Horton

Thread

Commits

  1. doc: Add better description for rewrite functions in event triggers

  1. Table rewrite supporting functions for event triggers

    Greg Sabino Mullane <htamfids@gmail.com> — 2024-09-02T21:15:03Z

    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)

Cheers,
Greg

  2. Re: Table rewrite supporting functions for event triggers

    Laurenz Albe <laurenz.albe@cybertec.at> — 2024-09-03T01:57:19Z

    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

  3. Re: Table rewrite supporting functions for event triggers

    Greg Sabino Mullane <htamfids@gmail.com> — 2024-09-03T15:10:57Z

    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

  4. Re: Table rewrite supporting functions for event triggers

    Greg Sabino Mullane <htamfids@gmail.com> — 2024-09-03T15:54:54Z

    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

  5. Re: Table rewrite supporting functions for event triggers

    Laurenz Albe <laurenz.albe@cybertec.at> — 2024-09-03T19:34:02Z

    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

  6. Re: Table rewrite supporting functions for event triggers

    Michael Paquier <michael@paquier.xyz> — 2024-09-11T06:00:31Z

    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

  7. Re: Table rewrite supporting functions for event triggers

    Greg Sabino Mullane <htamfids@gmail.com> — 2024-09-11T14:14:27Z

    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

  8. Re: Table rewrite supporting functions for event triggers

    Michael Paquier <michael@paquier.xyz> — 2024-09-12T03:17:28Z

    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

  9. Re: Table rewrite supporting functions for event triggers

    Greg Sabino Mullane <htamfids@gmail.com> — 2024-09-12T12:52:00Z

    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

  10. Re: Table rewrite supporting functions for event triggers

    Michael Paquier <michael@paquier.xyz> — 2024-09-12T22:39:13Z

    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

  11. Re: Table rewrite supporting functions for event triggers

    Michael Paquier <michael@paquier.xyz> — 2024-10-29T06:40:48Z

    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