Re: [PATCH] Add pg_get_event_trigger_ddl() function

Hi all,

On Tue, Jul 21, 2026 at 4:46 PM Ian Lawrence Barwick <barwick(at)gmail(dot)com> wrote:
>
> 2026年7月21日(火) 12:38 Rithvika Devisetti <devisettirithvika(at)gmail(dot)com>:
> >
> > Hi,
> >
> > On Mon, Jul 20, 2026 at 6:49 PM Ian Lawrence Barwick <barwick(at)gmail(dot)com> wrote:
> >>
> >> 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.
> >
> >
> > 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 for checking! Revised version attached, with that and a couple of
> minor text tweaks.
>

Thank you for the updated patch. I reviewed and tested this patch on
my tree. The patch applied cleanly and the regression test completed
successfully without any failures. I performed functional testing of
the new pg_get_event_trigger_ddl() function and verified: the function
is installed and registered correctly, the reconstruction of CREATE
EVENT TRIGGER statements, output formats, reconstruction of single and
multiple WHEN TAG clauses, preservation of event trigger enable
states, optional owner and enable parameters, schema-qualified
function names in the generated DDL, handling of quoted event trigger
names, expected error reporting for non-existent event triggers and
also confirmed that the generated DDL is consistent with the metadata
stored in pg_event_trigger. However I did not observe any functional
issues during testing and behaved as intended. The patch looks good to
me.

Regards,
Solai