Hi all, On Tue, Jul 21, 2026 at 4:46 PM Ian Lawrence Barwick <[email protected]> wrote: > > 2026年7月21日(火) 12:38 Rithvika Devisetti <[email protected]>: > > > > 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 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
