On Fri, Dec 17, 2021 at 10:51 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > I've attached updated patches. The first patch just moves common > function for index bulk-deletion and cleanup to vacuum.c. And the > second patch moves parallel vacuum code to vacuumparallel.c. The > comments I got so far are incorporated into these patches. Please > review them. >
Thanks, it is helpful for the purpose of review. Few comments: ============= 1. + * dead_items stores TIDs whose index tuples are deleted by index vacuuming. + * Each TID points to an LP_DEAD line pointer from a heap page that has been + * processed by lazy_scan_prune. Also needed by lazy_vacuum_heap_rel, which + * marks the same LP_DEAD line pointers as LP_UNUSED during second heap pass. */ - LVDeadItems *dead_items; /* TIDs whose index tuples we'll delete */ + VacDeadItems *dead_items; /* TIDs whose index tuples we'll delete */ Isn't it better to keep these comments atop the structure VacDeadItems declaration? 2. What is the reason for not moving lazy_vacuum_one_index/lazy_cleanup_one_index to vacuum.c so that they can be called from vacuumlazy.c and vacuumparallel.c? Without this refactoring patch, I think both leader and workers set the same error context phase (VACUUM_ERRCB_PHASE_VACUUM_INDEX) during index vacuuming? Is it because you want a separate context phase for a parallel vacuum? The other thing related to this is that if we have to do the way you have it here then we don't need pg_rusage_init() in functions lazy_vacuum_one_index/lazy_cleanup_one_index. 3. @@ -3713,7 +3152,7 @@ update_index_statistics(LVRelState *vacrel) int nindexes = vacrel->nindexes; IndexBulkDeleteResult **indstats = vacrel->indstats; - Assert(!IsInParallelMode()); + Assert(!ParallelVacuumIsActive(vacrel)); I think we can retain the older Assert. If we do that then I think we don't need to define ParallelVacuumIsActive in vacuumlazy.c. -- With Regards, Amit Kapila.
