Re: parallel vacuum comments

On Fri, Dec 17, 2021 at 10:51 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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?

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

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.

--
With Regards,
Amit Kapila.