Re: Event trigger code comment duplication

On Tuesday, May 12, 2020, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Tue, May 12, 2020 at 06:48:51PM -0700, David G. Johnston wrote:
> > Whether its a style thing, or some requirement of the C-language, I found
> > it odd that the four nearly identical checks were left inline in the
> > functions instead of being pulled out into a function. I've attached a
> > conceptual patch that does just this and more clearly presents on my
> > thoughts on the topic. In particular I tried to cleanup the quadruple
> > negative sentence (and worse for the whole paragraph) as part of the
> > refactoring of the currentEventTriggerState comment.
>
> You may want to check that your code compiles first :)

I said It was a conceptual patch...the inability to write correct C code
doesn’t wholly impact opinions of general code form.

> + /*
> + * See EventTriggerDDLCommandStart for a discussion about why event
> + * triggers are disabled in single user mode.
> + */
> + if (!IsUnderPostmaster)
> + return false;
> And here I am pretty sure that you don't want to remove the
> explanation why event triggers are disabled in standalone mode.

The full comment should have remained in the common function...so it moved
but wasn’t added or removed so not visible...in hindsight diff mode may
have been a less than ideal choice here. Or I may have fat-fingered the
copy-paste...

>
> Note the reason why we don't expect a state being set for
> ddl_command_start is present in EventTriggerBeginCompleteQuery():
> /*
> * Currently, sql_drop, table_rewrite, ddl_command_end events are the
> only
> * reason to have event trigger state at all; so if there are none,
> don't
> * install one.
> */

Thanks

>
> Even with all that, I am not sure that we need to complicate further
> what we have here. An empty currentEventTriggerState gets checks in
> three places, and each one of them has a slight different of the
> reason why we cannot process further, so I would prefer applying my
> previous, simple patch if there are no objections to remove the
> duplication about event triggers with standalone mode, keeping the
> explanations local to each event trigger type, and call it a day.
>

I’ll defer at this point - though maybe keep/improve the fix for the
quadruple negative and related commentary.

David J.