Re: POC: Parallel processing of indexes in autovacuum

Hi,

On Wed, Jun 18, 2025 at 5:37 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sun, May 25, 2025 at 10:22 AM Daniil Davydov <3danissimo@gmail.com> wrote:
> >
> > Thanks everybody for feedback! I attach a v4 patch to this letter.
> > Main features :
> > 1) 'parallel_autovacuum_workers' reloption - integer value, that sets
> > the maximum number of parallel a/v workers that can be taken from
> > bgworkers pool in order to process this table.
> > 2) 'max_parallel_autovacuum_workers' - GUC variable, that sets the
> > maximum total number of parallel a/v workers, that can be taken from
> > bgworkers pool.
> > 3) Parallel autovacuum does not try to use thresholds like
>  > NUM_INDEXES_PER_PARALLEL_WORKER and AV_PARALLEL_DEADTUP_THRESHOLD.
> > 4) Parallel autovacuum now can report statistics like "planned vs. launched".
> > 5) For now I got rid of the 'reserving' idea, so now autovacuum
> > leaders are competing with everyone for parallel workers from the
> > bgworkers pool.
> >
> > What do you think about this implementation?
> >
>
> I think it basically makes sense to me. A few comments:
>
> ---
> The patch implements max_parallel_autovacuum_workers as a
> PGC_POSTMASTER parameter but can we make it PGC_SIGHUP? I think we
> don't necessarily need to make it a PGC_POSTMATER since it actually
> doesn't affect how much shared memory we need to allocate.
>

Yep, there's nothing stopping us from doing that. This is a usable
feature, I'll implement it in the v5 patch.

> ---
> I think it's better to have the prefix "autovacuum" for the new GUC
> parameter for better consistency with other autovacuum-related GUC
> parameters.
>
> ---
>  #include "storage/spin.h"
> @@ -514,6 +515,11 @@ ReinitializeParallelDSM(ParallelContext *pcxt)
>     {
>         WaitForParallelWorkersToFinish(pcxt);
>         WaitForParallelWorkersToExit(pcxt);
> +
> +       /* Release all launched (i.e. reserved) parallel autovacuum workers. */
> +       if (AmAutoVacuumWorkerProcess())
> +           ParallelAutoVacuumReleaseWorkers(pcxt->nworkers_launched);
> +
>         pcxt->nworkers_launched = 0;
>         if (pcxt->known_attached_workers)
>         {
> @@ -1002,6 +1008,11 @@ DestroyParallelContext(ParallelContext *pcxt)
>      */
>     HOLD_INTERRUPTS();
>     WaitForParallelWorkersToExit(pcxt);
> +
> +   /* Release all launched (i.e. reserved) parallel autovacuum workers. */
> +   if (AmAutoVacuumWorkerProcess())
> +       ParallelAutoVacuumReleaseWorkers(pcxt->nworkers_launched);
> +
>     RESUME_INTERRUPTS();
>
> I think that it's better to release workers in vacuumparallel.c rather
> than parallel.c.
>

Agree with both comments.

Thanks for the review! Please, see v5 patch :
1) GUC variable and field in autovacuum shmem are renamed
2) ParallelAutoVacuumReleaseWorkers call moved from parallel.c to
vacuumparallel.c
3) max_parallel_autovacuum_workers is now PGC_SIGHUP parameter
4) Fix little bug (ParallelAutoVacuumReleaseWorkers in autovacuum.c:735)

--
Best regards,
Daniil Davydov