Horton

Thread

Commits

  1. doc: Remove event trigger firing matrix

  1. doc issues in event-trigger-matrix.html

    jian he <jian.universality@gmail.com> — 2024-03-19T00:00:00Z

    hi.
I think the "X" and "-" mean in this matrix [0] is not very intuitive.
mainly because "X" tends to mean negative things in most cases.
we can write a sentence saying "X"  means this, "-" means that.

or maybe Check mark [1] and Cross mark [2]  are more universal.
and we can use these marks.

"Only for local objects"
is there any reference explaining "local objects"?
I think local object means objects that only affect one single database?



[0] https://www.postgresql.org/docs/current/event-trigger-matrix.html
[1] https://en.wikipedia.org/wiki/Check_mark
[2] https://en.wikipedia.org/wiki/X_mark

  2. Re: doc issues in event-trigger-matrix.html

    Michael Paquier <michael@paquier.xyz> — 2024-03-19T01:14:10Z

    On Tue, Mar 19, 2024 at 08:00:00AM +0800, jian he wrote:
> I think the "X" and "-" mean in this matrix [0] is not very intuitive.
> mainly because "X" tends to mean negative things in most cases.
> we can write a sentence saying "X"  means this, "-" means that.
> 
> or maybe Check mark [1] and Cross mark [2]  are more universal.
> and we can use these marks.
> 
> "Only for local objects"
> is there any reference explaining "local objects"?
> I think local object means objects that only affect one single database?

It is true that in Japan the cross mark refers to a negation, and
that's the opposite in France: I would put a cross on a table in the
case where something is supported.  I've never seen anybody complain
about the format of these tables, FWIW, but if these were to be
changed, the update should happen across the board for all the tables
and not only one.

Using ASCII characters also has the advantage to make the maintenance
clightly easier, in my opinion, because there is no translation effort
between these special characters and their XML equivalent, like "&lt"
<-> "<".
--
Michael

  3. Re: doc issues in event-trigger-matrix.html

    Daniel Gustafsson <daniel@yesql.se> — 2024-03-19T09:34:32Z

    > On 19 Mar 2024, at 02:14, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Tue, Mar 19, 2024 at 08:00:00AM +0800, jian he wrote:
>> I think the "X" and "-" mean in this matrix [0] is not very intuitive.
>> mainly because "X" tends to mean negative things in most cases.
>> we can write a sentence saying "X"  means this, "-" means that.
>> 
>> or maybe Check mark [1] and Cross mark [2]  are more universal.
>> and we can use these marks.
>> 
>> "Only for local objects"
>> is there any reference explaining "local objects"?
>> I think local object means objects that only affect one single database?

That's a bigger problem than the table representation, we never define what
"local object" mean anywhere in the EVT docs.  EV's are global for a database,
but not a cluster, so I assume what this means is that EVs for non-DDL commands
like COMMENT can only fire for a specific relation they are attached to and not
database wide?

> It is true that in Japan the cross mark refers to a negation, and
> that's the opposite in France: I would put a cross on a table in the
> case where something is supported.  I've never seen anybody complain
> about the format of these tables, FWIW, but if these were to be
> changed, the update should happen across the board for all the tables
> and not only one.

AFAICT we only have one other table with "X" denoting support, the "Conflicting
Lock Modes" table under Concurrency Control chapter, and there we simply leave
the "not supported" column empty instead of using a dash.  Maybe the simple fix
here is to make these tables consistent by removing the dash from the event
trigger firing matrix?

As a sidenote, the table should gain a sentence explaining why the login column
is missing to avoid confusion.

--
Daniel Gustafsson

  4. Re: doc issues in event-trigger-matrix.html

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-21T21:47:17Z

    On 19.03.24 10:34, Daniel Gustafsson wrote:
>>> "Only for local objects"
>>> is there any reference explaining "local objects"?
>>> I think local object means objects that only affect one single database?
> That's a bigger problem than the table representation, we never define what
> "local object" mean anywhere in the EVT docs.  EV's are global for a database,
> but not a cluster, so I assume what this means is that EVs for non-DDL commands
> like COMMENT can only fire for a specific relation they are attached to and not
> database wide?

I think we could replace this whole table by a few definitions:

- "Local objects" are everything except "global objects".

- "Global objects", for the purpose of event triggers, are databases, 
tablespaces, roles, role memberships, and parameter ACLs.

- DDL commands are all commands except SELECT, INSERT, UPDATE, DELETE, 
MERGE.

- Events triggers are supported for all DDL commands on all local objects.

Is this table saying anything else?

Is there any way to check if it's even correct?  For example, it shows 
that the event "sql_​drop" can fire for a few ALTER commands, but how is 
this determined?  If tomorrow someone changes ALTER DOMAIN to possibly 
do a table rewrite, will they remember to update this table?

  5. Re: doc issues in event-trigger-matrix.html

    Daniel Gustafsson <daniel@yesql.se> — 2024-03-21T22:10:37Z

    > On 21 Mar 2024, at 22:47, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> On 19.03.24 10:34, Daniel Gustafsson wrote:
>>>> "Only for local objects"
>>>> is there any reference explaining "local objects"?
>>>> I think local object means objects that only affect one single database?
>> That's a bigger problem than the table representation, we never define what
>> "local object" mean anywhere in the EVT docs.  EV's are global for a database,
>> but not a cluster, so I assume what this means is that EVs for non-DDL commands
>> like COMMENT can only fire for a specific relation they are attached to and not
>> database wide?
> 
> I think we could replace this whole table by a few definitions:

Simply extending the "Overview of Event Trigger Behavior" section slightly
might even be enough?

> If tomorrow someone changes ... will they remember to update this table?

Highly unlikely.

--
Daniel Gustafsson

  6. Re: doc issues in event-trigger-matrix.html

    jian he <jian.universality@gmail.com> — 2024-03-22T03:58:51Z

    On Fri, Mar 22, 2024 at 5:47 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 19.03.24 10:34, Daniel Gustafsson wrote:
> >>> "Only for local objects"
> >>> is there any reference explaining "local objects"?
> >>> I think local object means objects that only affect one single database?
> > That's a bigger problem than the table representation, we never define what
> > "local object" mean anywhere in the EVT docs.  EV's are global for a database,
> > but not a cluster, so I assume what this means is that EVs for non-DDL commands
> > like COMMENT can only fire for a specific relation they are attached to and not
> > database wide?
>
> I think we could replace this whole table by a few definitions:
>
> - "Local objects" are everything except "global objects".
>
> - "Global objects", for the purpose of event triggers, are databases,
> tablespaces, roles, role memberships, and parameter ACLs.
>
> - DDL commands are all commands except SELECT, INSERT, UPDATE, DELETE,
> MERGE.
>
> - Events triggers are supported for all DDL commands on all local objects.
>
> Is this table saying anything else?
>
> Is there any way to check if it's even correct?  For example, it shows

comparing  these two html files:
https://www.postgresql.org/docs/devel/sql-commands.html
https://www.postgresql.org/docs/devel/event-trigger-matrix.html

summary:
all commands begin with "CREATE"
except the following two are not supported  by event trigger.
CREATE TRANSFORM
CREATE EVENT TRIGGER

generally, one "CREATE" corresponds to one "DROP" and one "ALTER".
but I found there is more to "CREATE" than  "ALTER".  (i didn't bother why)
there is one more "DROP" than "CREATE",
because of "DROP ROUTINE" and "DROP OWNED"
also
"CREATE TABLE"
"CREATE TABLE AS"
corresponds to one "DROP TABLE"

other command not begin with "CREATE" supported by event trigger (per
event-trigger-matrix) are:
COMMENT
GRANT Only for local objects
IMPORT FOREIGN SCHEMA
REFRESH MATERIALIZED VIEW
REINDEX
REVOKE
SECURITY LABEL
SELECT INTO

all commands
that is not begin with "CREATE" | "DROP", "ALTER" (per sql-commands.html) are:
ABORT
ANALYZE
BEGIN
CALL
CHECKPOINT
CLOSE
CLUSTER
COMMENT
COMMIT
COMMIT PREPARED
COPY
DEALLOCATE
DECLARE
DELETE
DISCARD
DO
END
EXECUTE
EXPLAIN
FETCH
GRANT
IMPORT FOREIGN SCHEMA
INSERT
LISTEN
LOAD
LOCK
MERGE
MOVE
NOTIFY
PREPARE
PREPARE TRANSACTION
REASSIGN OWNED
REFRESH MATERIALIZED VIEW
REINDEX
RELEASE SAVEPOINT
RESET
REVOKE
ROLLBACK
ROLLBACK PREPARED
ROLLBACK TO SAVEPOINT
SAVEPOINT
SECURITY LABEL
SELECT
SELECT INTO
SET
SET CONSTRAINTS
SET ROLE
SET SESSION AUTHORIZATION
SET TRANSACTION
SHOW
START TRANSACTION
TRUNCATE
UNLISTEN
UPDATE
VACUUM
VALUES

  7. Re: doc issues in event-trigger-matrix.html

    Peter Eisentraut <peter@eisentraut.org> — 2024-10-29T11:54:03Z

    I made a patch for this.  I have expanded the narrative discussion on 
what commands are supported for event triggers, also made a few 
corrections/additions there, based on inspecting the source code.  And 
then removed the big matrix, which doesn't provide any additional 
information, I think.

I think this is sufficient and covers everything.  The only hand-wavy 
thing I can see is exactly which ALTER commands trigger the sql_drop 
event.  But this was already quite imprecise before, and I think also 
not quite correct.  This might need a separate investigation.

In any case, we can use this as a starting point to iterate on the right 
wording etc.

  8. Re: doc issues in event-trigger-matrix.html

    Daniel Gustafsson <daniel@yesql.se> — 2024-10-29T14:53:43Z

    > On 29 Oct 2024, at 12:54, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> I made a patch for this.  I have expanded the narrative discussion on what commands are supported for event triggers, also made a few corrections/additions there, based on inspecting the source code.  And then removed the big matrix, which doesn't provide any additional information, I think.
> 
> I think this is sufficient and covers everything.  The only hand-wavy thing I can see is exactly which ALTER commands trigger the sql_drop event.  But this was already quite imprecise before, and I think also not quite correct.  This might need a separate investigation.
> 
> In any case, we can use this as a starting point to iterate on the right wording etc.

+1, I think this is a net improvement.

The only thing I would change on top of this is move the reference to section
9.30 under table_rewrite to be at the end after both supporting functions since
the link is relevant to both of them. Something like:

-    <literal>pg_event_trigger_table_rewrite_oid()</literal> (see
-    <xref linkend="functions-event-triggers"/>). To discover the reason(s)
-    for the rewrite, use the function
-    <literal>pg_event_trigger_table_rewrite_reason()</literal>.
+    <literal>pg_event_trigger_table_rewrite_oid()</literal> To discover the reason(s)
+    for the rewrite, use the function <literal>pg_event_trigger_table_rewrite_reason()</literal>
+    (see <xref linkend="functions-event-triggers"/>).

--
Daniel Gustafsson

  9. Re: doc issues in event-trigger-matrix.html

    Michael Paquier <michael@paquier.xyz> — 2024-10-29T22:33:57Z

    On Tue, Oct 29, 2024 at 03:53:43PM +0100, Daniel Gustafsson wrote:
> +1, I think this is a net improvement.

Agreed.  I have spent some time looking in the past few years looking
at patches that tweaked this table, and it was always hard to figure
out if it was completely right.

> The only thing I would change on top of this is move the reference to section
> 9.30 under table_rewrite to be at the end after both supporting functions since
> the link is relevant to both of them. Something like:
> 
> -    <literal>pg_event_trigger_table_rewrite_oid()</literal> (see
> -    <xref linkend="functions-event-triggers"/>). To discover the reason(s)
> -    for the rewrite, use the function
> -    <literal>pg_event_trigger_table_rewrite_reason()</literal>.
> +    <literal>pg_event_trigger_table_rewrite_oid()</literal> To discover the reason(s)
> +    for the rewrite, use the function <literal>pg_event_trigger_table_rewrite_reason()</literal>
> +    (see <xref linkend="functions-event-triggers"/>).

Fine by me to tweak this paragraph like that with the link at the end.
--
Michael

  10. Re: doc issues in event-trigger-matrix.html

    jian he <jian.universality@gmail.com> — 2024-10-30T12:31:36Z

    On Tue, Oct 29, 2024 at 7:54 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> I made a patch for this.  I have expanded the narrative discussion on
> what commands are supported for event triggers, also made a few
> corrections/additions there, based on inspecting the source code.  And
> then removed the big matrix, which doesn't provide any additional
> information, I think.
>
> I think this is sufficient and covers everything.  The only hand-wavy
> thing I can see is exactly which ALTER commands trigger the sql_drop
> event.  But this was already quite imprecise before, and I think also
> not quite correct.  This might need a separate investigation.
>
> In any case, we can use this as a starting point to iterate on the right
> wording etc.

hi. I have some minor issue.

   <para>
     An event trigger fires whenever the event with which it is associated
     occurs in the database in which it is defined.
</para>
is possible to rewrite this sentence, two "which" is kind of not easy
to understand?


create role alice;
create role bob;
grant alice to bob;
   <para>
     As an exception, this event does not occur for DDL commands targeting
     shared objects:
     <itemizedlist>
      <listitem><para>databases</para></listitem>
      <listitem><para>roles</para></listitem>
      <listitem><para>tablespaces</para></listitem>
      <listitem><para>parameter privileges</para></listitem>
      <listitem><para><command>ALTER SYSTEM</command></para></listitem>
     </itemizedlist>
     This event also does not occur for commands targeting event triggers
     themselves.
   </para>

not 100% sure this description
"      <listitem><para>roles</para></listitem>"
cover case like "grant alice to bob;"
Here "targeting  shared objects" is "role related meta information".
maybe a new item like
<listitem><para>roles privileges</para></listitem>.


so we can more easily distinguish
"grant select on t1 to alice;"
and
"grant alice to bob;"

  11. Re: doc issues in event-trigger-matrix.html

    Peter Eisentraut <peter@eisentraut.org> — 2024-11-06T12:48:50Z

    On 29.10.24 23:33, Michael Paquier wrote:
> On Tue, Oct 29, 2024 at 03:53:43PM +0100, Daniel Gustafsson wrote:
>> +1, I think this is a net improvement.
> 
> Agreed.  I have spent some time looking in the past few years looking
> at patches that tweaked this table, and it was always hard to figure
> out if it was completely right.
> 
>> The only thing I would change on top of this is move the reference to section
>> 9.30 under table_rewrite to be at the end after both supporting functions since
>> the link is relevant to both of them. Something like:
>>
>> -    <literal>pg_event_trigger_table_rewrite_oid()</literal> (see
>> -    <xref linkend="functions-event-triggers"/>). To discover the reason(s)
>> -    for the rewrite, use the function
>> -    <literal>pg_event_trigger_table_rewrite_reason()</literal>.
>> +    <literal>pg_event_trigger_table_rewrite_oid()</literal> To discover the reason(s)
>> +    for the rewrite, use the function <literal>pg_event_trigger_table_rewrite_reason()</literal>
>> +    (see <xref linkend="functions-event-triggers"/>).
> 
> Fine by me to tweak this paragraph like that with the link at the end.

Committed with the suggested changes.

  12. Re: doc issues in event-trigger-matrix.html

    Peter Eisentraut <peter@eisentraut.org> — 2024-11-06T12:50:00Z

    On 30.10.24 13:31, jian he wrote:
> On Tue, Oct 29, 2024 at 7:54 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> I made a patch for this.  I have expanded the narrative discussion on
>> what commands are supported for event triggers, also made a few
>> corrections/additions there, based on inspecting the source code.  And
>> then removed the big matrix, which doesn't provide any additional
>> information, I think.
>>
>> I think this is sufficient and covers everything.  The only hand-wavy
>> thing I can see is exactly which ALTER commands trigger the sql_drop
>> event.  But this was already quite imprecise before, and I think also
>> not quite correct.  This might need a separate investigation.
>>
>> In any case, we can use this as a starting point to iterate on the right
>> wording etc.
> 
> hi. I have some minor issue.
> 
>     <para>
>       An event trigger fires whenever the event with which it is associated
>       occurs in the database in which it is defined.
> </para>
> is possible to rewrite this sentence, two "which" is kind of not easy
> to understand?

I couldn't think of anything simpler that wouldn't be weirdly nested in 
some other way.  This wasn't really related to this patch, so I didn't 
touch it.  But suggestions are welcome.

> create role alice;
> create role bob;
> grant alice to bob;
>     <para>
>       As an exception, this event does not occur for DDL commands targeting
>       shared objects:
>       <itemizedlist>
>        <listitem><para>databases</para></listitem>
>        <listitem><para>roles</para></listitem>
>        <listitem><para>tablespaces</para></listitem>
>        <listitem><para>parameter privileges</para></listitem>
>        <listitem><para><command>ALTER SYSTEM</command></para></listitem>
>       </itemizedlist>
>       This event also does not occur for commands targeting event triggers
>       themselves.
>     </para>
> 
> not 100% sure this description
> "      <listitem><para>roles</para></listitem>"
> cover case like "grant alice to bob;"
> Here "targeting  shared objects" is "role related meta information".
> maybe a new item like
> <listitem><para>roles privileges</para></listitem>.

Yeah, I added a clarification in the committed version.