Re: PATCH: Add REINDEX tag to event triggers

On Tue, Nov 28, 2023 at 12:28:50AM +0800, jian he wrote:
> On Mon, Nov 27, 2023 at 6:58 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>> Ajin Cherian

Dammit. I've just noticed that I missed mentioning you in the commit
log as a reviewer. Sorry about that.

> The above 2 suggestions are good, now the code is more neat.

Right, the extra bits for the default case of
standard_ProcessUtility() was not necessary, because we don't need to
apply any object-level checks to see as we just collect a list of
indexes.

Anyway, I've been working on the patch for the last few days, and
applied it after tweaking a bit its style, code and comments.

Mainly, I did not see a need for reindex_event_trigger_collect() as
wrapper for EventTriggerCollectSimpleCommand() as it is only used in
two code paths across two files. I have come back to the regression
tests, and trimmed them down to a minimum as event_trigger.sql cannot
be run in parallel so this eats in run time, concluding that knowing
the list of indexes rebuilt is also something that we track with
relfilenode change tracking in the other test suites (including the
SCHEMA, toast and partition cases), so doing that again had limited
impact.

One thing that I have been unable to conclude is if we should change
GetCommandLogLevel() for ReindexStmt. I agree that there's a good
argument for it, but I am going to raise a different thread about that
to raise awareness, and this does not prevent the feature to work
(even if it feels inconsistent with pg_event_trigger_ddl_commands()).
--
Michael