Re: parallel vacuum comments

On Tue, Dec 21, 2021 at 12:05 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Dec 20, 2021 at 6:29 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Mon, Dec 20, 2021 at 1:08 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > > >
> > > > > 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?
> >
>
> Right, but that also doesn't sound good to me.

Me too.

> I think it is better to
> keep a separate error context for parallel vacuum workers as you have
> in the patch. However, let's add some comments to indicate that if
> there is a change in one of the context functions, the other should be
> changed.

Agreed.

> BTW, if we go with that then we should set the correct phase
> for workers as well?

If we have separate error context for the leader (vacuumlazy.c) and
workers (vacuumparallel.c), workers don't necessarily need to have the
phases such as VACUUM_ERRCB_PHASE_VACUUM_INDEX and
VACUUM_ERRCB_PHASE_INDEX_CLEANUP. They can use PVIndVacStatus in the
error callback function as the patch does.

Regards,

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