Hi, On Mon, Jul 20, 2026 at 6:49 PM Ian Lawrence Barwick <[email protected]> wrote:
> Hi Jim > > 2026年7月21日(火) 0:13 Jim Jones <[email protected]>: > > > > Hi Ian > > > > Thanks for the patch! Here a few comments on v2: > > > > == shadow variable == > > > > The function pg_get_event_trigger_ddl_internal has a bool parameter > > named owner, and its body has a char* with the same name. > > Good catch - I note the compiler-level shadow check is only effective > for variables of the same type, which hadn't occurred to me before. > > > == trailing XXX (placeholder?) == > > > > + * CREATE EVENT TRIGGER statement; XXX > > Whoops, fixed. > > > == typo (2x the) == > > > > + is false, the the corresponding <literal>ENABLE</literal> clause is > > Classic typo invisible to the author, fixed. > > > === switch without default == > > > > I realise that "switch (evtForm->evtenabled)" already tests all possible > > values of evtenable, but I'm wondering if we really should let it > > silently finish the buffer like "ALTER EVENT TRIGGER foo ;" if evtenable > > ever gets a different value. I'd argue that an error message would be > > better than a malformed DDL. What do you think? > > I did wonder - the corresponding switch statement in pg_dump falls through > to > "ENABLE" as the default value, but only calls that if "evtenabled" is > not 'O' (the > default), so the addition of some new trigger firing state would result > in "ALTER EVENT TRIGGER foo ENABLE;" if the code was not updated, > which is easy to overlook with this kind of setup. On the other hand, I > can't > imagine what kind of new trigger firing state might be added, so maybe > it's more of a theoretical risk. In any case I added an error message, and > also updated the tests to check each existing firing state is > correctly rendered. While looking at that, I did update the comment in > "src/include/commands/trigger.h" > to note that the "TRIGGER_*" definitions also apply to pg_event_trigger. > Patch applies clean to me, and tests passed. I see a grammatical typo below, there is an additional "be". cannot be actually be used in a valid statement Thanks, Rithvika
