On Sat, Dec 18, 2021 at 3:38 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > 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?
I think LP_DEAD and LP_UNUSED stuff are specific to heap. Given moving VacDeadItems to vacuum.c, I thought it's better to keep it as generic TID storage. > > 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? Since the phases defined as VacErrPhase like VACUUM_ERRCB_PHASE_SCAN_HEAP and VACUUM_ERRCB_PHASE_VACUUM_HEAP etc. and error callback function, vacuum_error_callback(), are specific to heap, I thought it'd not be a good idea to move lazy_vacuum/cleanup_one_index() so that both vacuumlazy.c and vacuumparallel.c can use the phases and error callback function. > 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. Right. It should be removed. > > 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. Right, will fix in the next version patch. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
