On Mon, Dec 20, 2021 at 8:33 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Sat, Dec 18, 2021 at 3:38 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > 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. >
Okay, that makes sense. > > > > 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. > How about exposing it via heapam.h? We have already exposed a few things via heapam.h (see /* in heap/vacuumlazy.c */). In the current proposal, we need to have separate callbacks and phases for index vacuuming so that it can be used by both vacuumlazy.c and vacuumparallel.c which might not be a good idea. -- With Regards, Amit Kapila.