On Wed, Sep 11, 2024 at 11:17 PM Michael Paquier <mich...@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
