Thanks! I added the patch to the commitfest. TIL. > Hmm. What are the use cases you have in mind where you need to know the list of indexes listed by an event trigger like this?
Direct use case is general index health and rebuild. We had a recent upgrade go poorly (our fault) that left a bunch of indexes in a corrupted state. We ran a mass rebuild a few times. Would have been great to be able to observe changes happening in scripts. That left us wishing there was a way to monitor all changes around indexes. Figured I'd add support since my first thread seemed somewhat supportive of the idea. > We have a table_rewrite which is equivalent to check if a relfilenode has changed, so why not, I guess.. Yeah. I was actually debating if we need to add a new reindex event instead of building on top of the DDL start/end. You are right that the documentation does not include nonsql commands like reindex. Maybe I should consider going that direction again? > Assuming that ddl_command_start is not supported is incorrect. Also great callout. Using `SELECT * FROM pg_event_trigger_ddl_commands()` in the event does not yield anything. Bad assumption there. I'll add a test with my next patch version. Re: LOGSTMT_ALL Agree here. I think this is more for reference than anything else so I'm fine to call it LOGSTMT_ALL, but that goes back to the previous question around ddl events vs a new event similar to table_rewrite. P.S. Sorry for the double post. Sent from the wrong email so the mailing list didn't get the message. On Tue, Jul 25, 2023 at 12:55 AM Michael Paquier <mich...@paquier.xyz> wrote: > On Thu, Jul 20, 2023 at 10:47:00PM -0600, Garrett Thornburg wrote: > > Added my v1 patch to add REINDEX to event triggers. > > > > I originally built this against pg15 but rebased to master for the patch > > to hopefully make it easier for maintainers to merge. The rebase was > > automatic so it should be easy to include into any recent version. I'd > love > > for this to land in pg16 if possible. > > The development of Postgres 16 has finished last April and this > version is in a stabilization period. We are currently running the > first commit fest of PostgreSQL 17, though. I would recommend to > register your patch here so as it is not lost: > https://commitfest.postgresql.org/44/ > > > I added regression tests and they are passing. I also formatted the code > > using the project tools. Docs are included too. > > Hmm. What are the use cases you have in mind where you need to know > the list of indexes listed by an event trigger like this? Just > monitoring to know which indexes have been rebuilt? We have a > table_rewrite which is equivalent to check if a relfilenode has > changed, so why not, I guess.. > > Assuming that ddl_command_start is not supported is incorrect. For > example, with your patch applied, if I do the following setup: > create event trigger regress_event_trigger on ddl_command_start > execute procedure test_event_trigger(); > create function test_event_trigger() returns event_trigger as $$ > BEGIN > RAISE NOTICE 'test_event_trigger: % %', tg_event, tg_tag; > END > $$ language plpgsql; > > Then a REINDEX gives me that: > reindex table pg_class; > NOTICE: 00000: test_event_trigger: ddl_command_start REINDEX > > The first paragraphs of event-trigger.sgml describe the following: > The <literal>ddl_command_start</literal> event occurs just before the > execution of a <literal>CREATE</literal>, <literal>ALTER</literal>, > <literal>DROP</literal>, > <literal>SECURITY LABEL</literal>, > <literal>COMMENT</literal>, <literal>GRANT</literal> or > <literal>REVOKE</literal> > [...] > The <literal>ddl_command_end</literal> event occurs just after the > execution of > this same set of commands. To obtain more details on the > <acronym>DDL</acronym> > > So it would need a refresh to list REINDEX. > > case T_ReindexStmt: > - lev = LOGSTMT_ALL; /* should this be DDL? */ > + lev = LOGSTMT_DDL; > > This, unfortunately, may not be as right as it is simple because > REINDEX is not a DDL as far as I know (it is not in the SQL > standard). > -- > Michael >
