Re: parallel vacuum comments

On Sat, Dec 18, 2021 at 3:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Dec 17, 2021 at 10:51 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached updated patches. The first patch just moves common
> > function for index bulk-deletion and cleanup to vacuum.c. And the
> > second patch moves parallel vacuum code to vacuumparallel.c. The
> > comments I got so far are incorporated into these patches. Please
> > review them.
> >
>
> Thanks, it is helpful for the purpose of review.
>
> 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.

>
> 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.

> The other thing related to this is that if we have to
> do the way you have it here then we don't need pg_rusage_init() in
> functions lazy_vacuum_one_index/lazy_cleanup_one_index.

Right. It should be removed.

>
> 3.
> @@ -3713,7 +3152,7 @@ update_index_statistics(LVRelState *vacrel)
>   int nindexes = vacrel->nindexes;
>   IndexBulkDeleteResult **indstats = vacrel->indstats;
>
> - Assert(!IsInParallelMode());
> + Assert(!ParallelVacuumIsActive(vacrel));
>
> I think we can retain the older Assert. If we do that then I think we
> don't need to define ParallelVacuumIsActive in vacuumlazy.c.

Right, will fix in the next version patch.

Regards,

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