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
