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

How about something like this? I realize the other functions in that
section are not linked, but they all seem pretty self-explanatory compared
to the mysterious int return value of the linked one.

Cheers,
Greg

On Tue, 2024-09-03 at 11:54 -0400, Greg Sabino Mullane wrote:

How about something like this?

This patch looks good to me.

Yours,
Laurenz Albe

On Tue, Sep 03, 2024 at 09:34:02PM +0200, Laurenz Albe wrote:

On Tue, 2024-09-03 at 11:54 -0400, Greg Sabino Mullane wrote:

How about something like this?

This patch looks good to me.

-        Returns a code explaining the reason(s) for rewriting.  The exact
-        meaning of the codes is release dependent.
+        Returns a code explaining the reason(s) for rewriting. The value is
+        a bitmap built from the following values: 1 (the table has changed
+        persistence), 2 (a column has changed a default value), 4 (a column
+        has a new data type), and 8 (the table access method has changed).

Agreed that the user experience with this function is poor and that
the documentation should be improved. Still, I am not sure that this
is optimal. On top of the values, how about adding the variable names
and also mention that these are defined in event_trigger.h?

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.
--
Michael

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 10:14:27AM -0400, Greg Sabino Mullane wrote:

I dunno - so would we smush them together and return something like:

"ALTER_PERSISTENCE and COLUMN_REWRITE"

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

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. If this is like what we do in the regression tests and
print it in notice messages within a PL/pgSQL function, that's not
going to matter.

Or just have a separate function..

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.
--
Michael

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

On Thu, Sep 12, 2024 at 08:52:00AM -0400, Greg Sabino Mullane wrote:

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"
...

Okay. I am not going to be annoying with compatibility, then :D

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.)

I am not so sure, TBH. One example: I know these values in
tablecmds.c for some time because that's an area I tend to focus on
for bug fixes, but forgot entirely about the SQL function in event
triggers that feed on it until I found your post. A comment in
event_trigger.h to mention the doc update would work for me. That
would be impossible to miss.
--
Michael

On Fri, Sep 13, 2024 at 07:39:13AM +0900, Michael Paquier wrote:

I am not so sure, TBH. One example: I know these values in
tablecmds.c for some time because that's an area I tend to focus on
for bug fixes, but forgot entirely about the SQL function in event
triggers that feed on it until I found your post. A comment in
event_trigger.h to mention the doc update would work for me. That
would be impossible to miss.

My apologies for the delay in replying here. I've looked back at your
proposal in 0002 to mention the two functions in the main section for
event triggers and to add a description of the reson values in the
bitmap. After adding a comment in event_trigger.h to tell that the
documentation needs to be kept in sync, the result looked OK so I have
applied that down to 13.

1ad23335f36b got in the way for 12 with some conflicts. As it is
going to be EOL in a few days, it does not really matter much.
--
Michael