Re: [PATCH] Add pg_get_event_trigger_ddl() function

Hi Jim

2026年7月21日(火) 0:13 Jim Jones <jim(dot)jones(at)uni-muenster(dot)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.

> == 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.

Regards

Ian Barwick