Re: parallel vacuum comments

On Mon, Dec 20, 2021 at 8:33 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Sat, Dec 18, 2021 at 3:38 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.