On Mon, Dec 20, 2021 at 6:29 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Dec 20, 2021 at 1:08 PM Amit Kapila <amit.kapila16@gmail.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. 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. BTW, if we go with that then we should set the correct phase
for workers as well?
--
With Regards,
Amit Kapila.