Thread: doc issues in event-trigger-matrix.html
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
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 "<" <-> "<". -- Michael
Attachment
> 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
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?
> 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
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
> 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, alsomade a few corrections/additions there, based on inspecting the source code. And then removed the big matrix, whichdoesn'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 triggerthe sql_drop event. But this was already quite imprecise before, and I think also not quite correct. This mightneed 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
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;"
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.
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.