Re: PATCH: Add REINDEX tag to event triggers

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