Re: PATCH: Add REINDEX tag to event triggers

On Fri, Nov 24, 2023 at 08:00:00AM +0800, jian he wrote:
> not sure this is the right approach. But now the reindex event
> collection is after we call index_open.
> same static function (reindex_event_trigger_collect) in
> src/backend/commands/indexcmds.c and src/backend/catalog/index.c for
> now.

Knowing that there are two calls of reindex_relation() and four
callers of reindex_index(), passing the stmt looks acceptable to me to
be able to get more context from the reindex statement where a given
index has been rebuilt.

Anyway, as a whole, I am still confused by the shape of the patch..
This relies on pg_event_trigger_ddl_commands(), that reports a list of
DDL commands, but the use case you are proposing is to report a list
of the *indexes* rebuilt. So, in its current shape, we'd report the
equivalent of N DDL commands matching the N indexes rebuilt in a
single command. Shouldn't we use a new event type and a dedicated SQL
function to report the list of indexes newly rebuilt to get a full
list of the work that has happened?

> given that ProcessUtilitySlow supports the event triggers facility.
> so probably no need for another REINDEXOPT_ option?

Right, perhaps we don't need that if we just supply the stmt. If
there is no requirement for it, let's avoid that, then.

- ReindexMultipleTables(stmt->name, stmt->kind, &params);
+ ReindexMultipleTables(stmt, stmt->name, stmt->kind, &params);

If we begin including the stmt, there is no need for these extra
arguments in the extra routines of indexcmds.c.

As far as I can see, this patch is doing too much as presented. Could
you split the patch into more pieces, please? Based on v4 you have
sent, there are refactoring and basic piece parts like:
- Patch to make event triggers ddl_command_start and ddl_command_stop
react on ReindexStmt, so as a command is reported in
pg_event_trigger_ddl_commands().
- Patch for GetCommandLogLevel() to switch ReindexStmt from
LOGSTMT_ALL to LOGSTMT_DDL. True that this could be switched.
- Patch to refactor the routines of indexcmds.c and index.c to use the
ReindexStmt as argument, as a preparation of the next patch...
- Patch to add a new event type with a new SQL function to return a
list of the indexes rebuilt, with the ReindexStmt involved when the
index OID was trapped by the collection.

1) and 2) are a minimal feature in themselves, which may be enough to
satisfy your original case, and where you'd know when a REINDEX has
been run in event triggers. 3) and 4) are what you are trying to
achieve, to get the list of the indexes rebuilt knowing the context of
a given command.
--
Michael