Thread: Re: Table rewrite supporting functions for event triggers
On Mon, 2024-09-02 at 17:15 -0400, Greg Sabino Mullane wrote: > While looking over the event trigger docs, I noticed that the sample code references > the two special table rewrite functions (returning oid and reason for the rewrite), > but the event trigger page itself does not mention them, although it does mention > the functions available for the other types of event triggers (e.g. pg_event_trigger_ddl_commands). > Please find attached a patch to remedy this, including the meaning of the int values > (which, while subject to change, seems worth documenting here rather than hand-waving > it away as func.sgml does) I think that it would be better to add a reference to https://www.postgresql.org/docs/16/functions-event-triggers.html#FUNCTIONS-EVENT-TRIGGER-TABLE-REWRITE than to repeat that information. If you feel that "The exact meaning of the codes is release dependent" is unnecessarily vague, that sentence should be changed. Your proposed description leaves me a bit clueless: + To find the OID of the table that was rewritten, use the function + <literal>pg_event_trigger_table_rewrite_oid()</literal>. To discover the + reason for the rewrite, use the function + <literal>pg_event_trigger_table_rewrite_reason()</literal>. This function returns + an integer representing a bitmap of reasons for the rewrite. The current values + are 1 (the table has changed persistence), 2 (a column has changed a default value), + 3 (a column has a new data type), and 4 (the table access method has changed). A "bitmap of reasons" to me would mean that each reason is a bit, and if two reasons apply at the same time, both bits are set. But that's clearly not what you mean, because "a column has a new data type" is not the same as "the table has changed persistence" and at the same time "a column has changed a default value". Perhaps "a bitmap of reasons" should simply become "the reason". Yours, Laurenz Albe
On Mon, Sep 2, 2024 at 9:57 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
I think that it would be better to add a reference to
https://www.postgresql.org/docs/16/functions-event-triggers.html#FUNCTIONS-EVENT-TRIGGER-TABLE-REWRITE
than to repeat that information.
If you feel that "The exact meaning of the codes is release dependent" is unnecessarily
vague, that sentence should be changed.
Okay, that's a good point, will do so in the next patch.
A "bitmap of reasons" to me would mean that each reason is a bit, and if two reasons
apply at the same time, both bits are set. But that's clearly not what you mean, because
"a column has a new data type" is not the same as "the table has changed persistence"
and at the same time "a column has changed a default value".
Perhaps "a bitmap of reasons" should simply become "the reason".
No, it really is a bitmap, as you can get a reason of "5" for example, if you change the persistence and a data type in the same ALTER TABLE command, e.g. alter table foo set unlogged, alter id type bigint; But I wrote it wrong: they should be 1,2,4,8.
Cheers,
Greg
On Wed, Sep 11, 2024 at 2:00 AM Michael Paquier <michael@paquier.xyz> wrote:
Putting the documentation change aside for a bit, could it be better
to redesign this function and return a text value rather than an
integer? We could directly return the names, minus "AT_REWRITE_", for
instance.
I dunno - so would we smush them together and return something like:
"ALTER_PERSISTENCE and COLUMN_REWRITE"
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.
Cheers,
Greg
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