On Mon, Jul 20, 2026 at 6:49 PM Ian Lawrence Barwick <barwick@gmail.com> wrote:
Hi Jim
2026年7月21日(火) 0:13 Jim Jones <jim.jones@uni-muenster.de>: > > 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.
> == 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,
