On Tue, Nov 28, 2023 at 12:28:50AM +0800, jian he wrote: > On Mon, Nov 27, 2023 at 6:58 PM Ajin Cherian <itsa...@gmail.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
signature.asc
Description: PGP signature