Re: Table rewrite supporting functions for event triggers

On Wed, Sep 11, 2024 at 11:17 PM Michael Paquier <michael@paquier.xyz>
wrote:

> If multiple are set, let's just make it text[], then.
>

Hmmm...I guess it's better than an integer, in some ways, but I'm still a
weak -1.

> That would be a step backwards for anyone possibly using that integer
> > programatically to (for example) give a pretty user-facing message about
> > why the event was triggered.
>
> I don't know either how much people are relying on these numbers in
> applications.


I don't know either - it's one of those problems with our open source -
there could be literally hundreds of people using it, or it could be just
me! :)

I do like the simplicity of the bitmap:

if (reason & 1)
  print "Table has changed from logged to unlogged"
if (reason & 2)
  print "Default has been changed"

versus with text[]:

foreach reason in tablereason[]
  if reason.match_exact("ALTER_PERSISTENCE")
    print "Table has changed from logged to unlogged"
  if reason.match_regex("DEFAULT")
    print "Default has been changed"
...


> Do you have a comment about mentioning the variables or the header in the
> docs for the stable branches?  I'm aware that this is a rare
> practice, but so is this function's design.  My argument is greppability
> between the code and the docs, mainly, to not miss an update of the docs if
> more reasons are added.  That would be unlikely, but a backpatch of a
> reason is not impossible ABI-wise.
>

My initial reaction was that this is indeed a rare case, and to avoid
putting that level of code detail in the docs. Your argument is a good one,
but it still feels wrong to put that there. Yes, this puts a little more
onus on future developers, but updating the docs is already a core
requirement for patches.

(On reflection, maybe reverse it - put a code comment in event_trigger.h
reminding people to also update the docs? But again, that's seems like
something obvious anyway for someone making that change.)

Cheers,
Greg