On Mon, May 11, 2020 at 11:30 PM Michael Paquier <michael@paquier.xyz> wrote:
The second point about the check with (!currentEventTriggerState) in EventTriggerTableRewrite() and EventTriggerDDLCommandEnd() shows that both comments share the same first sentence, but there is enough different context to just keep them as separate IMO.
Went back and looked this over - the comment differences in the check for currentEventTriggerState boil down to:
the word "required" vs "important" - either works for both.
the fact that the DDLCommandEnd function probably wouldn't crash absent the check - which while I do not know whether DDLTriggerRewrite would crash for certain (hence the "required") as a practical matter it is required (and besides if keeping note of which of these would crash or not is deemed important that can be commented upon specifically in each - both DDLCommandStart (which lacks the check altogether...) and SQLDrop both choose not to elaborate on that point at all.
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.