Re: [PATCH] Add pg_get_event_trigger_ddl() function

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.

Regards

Ian Barwick