Re: parallel vacuum comments

On Mon, Dec 20, 2021 at 1:08 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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.

Yeah, but if we expose VacErrPhase and vacuum_error_callback(), we
need to also expose LVRelState and vacuumparallel.c also uses it,
which seems not a good idea. So we will need to introduce a new struct
dedicated to the error callback function. Is that right?

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/