Re: PATCH: Add REINDEX tag to event triggers

On Mon, Nov 27, 2023 at 11:00 AM jian he <jian.universality@gmail.com> wrote:
>
> On Fri, Nov 24, 2023 at 10:44 AM Michael Paquier <michael@paquier.xyz> wrote:
> hi.
> v5-0001. changed the REINDEX command tag from event_trigger_ok: false
> to event_trigger_ok: true.
> Move ReindexStmt moves from standard_ProcessUtility to ProcessUtilitySlow.
> By default ProcessUtilitySlow will call trigger related functions.
> So the event trigger facility can support reindex statements.
>
> v5-0002. In GetCommandLogLevel, change T_ReindexStmt from lev =
> LOGSTMT_ALL to lev = LOGSTMT_DDL. so log_statement (enum) >= 'ddl'
> will make the reindex statement be logged.
>
> v5-0003. Refactor the following functions: {ReindexIndex,
> ReindexTable, ReindexMultipleTables,
> ReindexPartitions,ReindexMultipleInternal
> ,ReindexRelationConcurrently, reindex_relation,reindex_index} by
> adding `const ReindexStmt *stmt` as their first argument.
> This is for event trigger support reindex. We need to pass both the
> newly generated indexId and the ReindexStmt to
> EventTriggerCollectSimpleCommand right after the newly index gets
> their lock. To do that, we have to refactor related functions.
>
> v5-0004. Event trigger support reindex command implementation,
> documentation, regress test, helper function pass reindex info to
> function EventTriggerCollectSimpleCommand.

I just started reviewing the patch. Some minor comments:
In patch 0001:
In standard_ProcessUtility(), since you are unconditionally calling
ProcessUtilitySlow() in case of T_ReindexStmt, you really don't need
the case statement for T_ReindexStmt just like all the other commands
which have event trigger support. It will call ProcessUtilitySlow() as
default.

In patch 0004:
No need to duplicate reindex_event_trigger_collect in indexcmds.c
since it is already present in index.c. Add it to index.h and make the
function extern so that it can be accessed in both index.c and
indexcmds.c

regards,
Ajin Cherian
Fujitsu Australia