Re: POC: Parallel processing of indexes in autovacuum

Hi,

On Mon, Jul 14, 2025 at 2:10 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> ---
> -   shared->maintenance_work_mem_worker =
> -       (nindexes_mwm > 0) ?
> -       maintenance_work_mem / Min(parallel_workers, nindexes_mwm) :
> -       maintenance_work_mem;
> +
> +   if (AmAutoVacuumWorkerProcess())
> +       shared->maintenance_work_mem_worker =
> +           (nindexes_mwm > 0) ?
> +           autovacuum_work_mem / Min(parallel_workers, nindexes_mwm) :
> +           autovacuum_work_mem;
> +   else
> +       shared->maintenance_work_mem_worker =
> +           (nindexes_mwm > 0) ?
> +           maintenance_work_mem / Min(parallel_workers, nindexes_mwm) :
> +           maintenance_work_mem;
>
> Since we have a similar code in dead_items_alloc() I think it's better
> to follow it:
>
>     int         vac_work_mem = AmAutoVacuumWorkerProcess() &&
>         autovacuum_work_mem != -1 ?
>         autovacuum_work_mem : maintenance_work_mem;
>
> That is, we calculate vac_work_mem first and then calculate
> shared->maintenance_work_mem_worker. I think it's more straightforward
> as the formula of maintenance_work_mem_worker is the same whereas the
> amount of memory used for vacuum and autovacuum varies.
>

I was confused by the fact that initially maintenance_work_mem was used
for calculations, not vac_work_mem. I agree that we should better use
already calculated vac_work_mem value.

> ---
> +   nlaunched_workers = pvs->pcxt->nworkers_launched; /* remember this value */
>     DestroyParallelContext(pvs->pcxt);
> +
> +   /* Release all launched (i.e. reserved) parallel autovacuum workers. */
> +   if (AmAutoVacuumWorkerProcess())
> +       ParallelAutoVacuumReleaseWorkers(nlaunched_workers);
> +
>
> Why don't we release workers before destroying the parallel context?
>

Destroying parallel context includes waiting for all workers to exit (after
which, other operations can use them).
If we first call ParallelAutoVacuumReleaseWorkers, some operation can
reasonably request all released workers. But this request can fail,
because there is no guarantee that workers managed to finish.

Actually, there's nothing wrong with that, but I think releasing workers
only after finishing work is a more logical approach.

> ---
> @@ -558,7 +576,9 @@ parallel_vacuum_compute_workers(Relation *indrels,
> int nindexes, int nrequested,
>      * We don't allow performing parallel operation in standalone backend or
>      * when parallelism is disabled.
>      */
> -   if (!IsUnderPostmaster || max_parallel_maintenance_workers == 0)
> +   if (!IsUnderPostmaster ||
> +       (autovacuum_max_parallel_workers == 0 && AmAutoVacuumWorkerProcess()) ||
> +       (max_parallel_maintenance_workers == 0 && !AmAutoVacuumWorkerProcess()))
>         return 0;
>
>     /*
> @@ -597,15 +617,17 @@ parallel_vacuum_compute_workers(Relation
> *indrels, int nindexes, int nrequested,
>     parallel_workers = (nrequested > 0) ?
>         Min(nrequested, nindexes_parallel) : nindexes_parallel;
>
> -   /* Cap by max_parallel_maintenance_workers */
> -   parallel_workers = Min(parallel_workers, max_parallel_maintenance_workers);
> +   /* Cap by GUC variable */
> +   parallel_workers = AmAutoVacuumWorkerProcess() ?
> +       Min(parallel_workers, autovacuum_max_parallel_workers) :
> +       Min(parallel_workers, max_parallel_maintenance_workers);
>
>     return parallel_workers;
>
> How about calculating the maximum number of workers once and using it
> in the above both places?
>

Agree. Good idea.

> ---
> +   /* Check how many workers can provide autovacuum. */
> +   if (AmAutoVacuumWorkerProcess() && nworkers > 0)
> +       nworkers = ParallelAutoVacuumReserveWorkers(nworkers);
> +
>
> I think it's better to move this code to right after setting "nworkers
> = Min(nworkers, pvs->pcxt->nworkers);" as it's a more related code.
>
> The comment needs to be updated as it doesn't match what the function
> actually does (i.e. reserving the workers).
>

You are right, I'll fix it.

> ---
>         /* Reinitialize parallel context to relaunch parallel workers */
>         if (num_index_scans > 0)
> +       {
>             ReinitializeParallelDSM(pvs->pcxt);
>
> +           /*
> +            * Release all launched (i.e. reserved) parallel autovacuum
> +            * workers.
> +            */
> +           if (AmAutoVacuumWorkerProcess())
> +               ParallelAutoVacuumReleaseWorkers(pvs->pcxt->nworkers_launched);
> +       }
>
> Why do we need to release all workers here? If there is a reason, we
> should mention it as a comment.
>

Hm, I guess it was left over from previous patch versions. Actually
we don't need to release workers here, as we will try to launch them
immediately. It is a bug, thank you for noticing it.

> ---
> @@ -706,16 +751,16 @@
> parallel_vacuum_process_all_indexes(ParallelVacuumState *pvs, int
> num_index_scan
>
>         if (vacuum)
>             ereport(pvs->shared->elevel,
> -                   (errmsg(ngettext("launched %d parallel vacuum
> worker for index vacuuming (planned: %d)",
> -                                    "launched %d parallel vacuum
> workers for index vacuuming (planned: %d)",
> +                   (errmsg(ngettext("launched %d parallel %svacuum
> worker for index vacuuming (planned: %d)",
> +                                    "launched %d parallel %svacuum
> workers for index vacuuming (planned: %d)",
>                                      pvs->pcxt->nworkers_launched),
> -                           pvs->pcxt->nworkers_launched, nworkers)));
> +                           pvs->pcxt->nworkers_launched,
> AmAutoVacuumWorkerProcess() ? "auto" : "", nworkers)));
>
> The "%svacuum" part doesn't work in terms of translation. We need to
> construct the whole sentence instead.
> But do we need this log message
> change in the first place? IIUC autovacuums write logs only when the
> execution time exceed the log_autovacuum_min_duration (or its
> reloption). The patch unconditionally sets LOG level for autovacuums
> but I'm not sure it's consistent with other autovacuum logging
> behavior:
>
> +   int         elevel = AmAutoVacuumWorkerProcess() ||
> +       vacrel->verbose ?
> +       INFO : DEBUG2;
>
>

This log level is used only "for messages about parallel workers launched".
I think that such logs relate more to the parallel workers module than
autovacuum itself. Moreover, if we emit log "planned vs. launched" each
time, it will simplify the task of selecting the optimal value of
'autovacuum_max_parallel_workers' parameter. What do you think?

About "%svacuum" - I guess we need to clarify what exactly the workers
were launched for. I'll add errhint to this log, but I don't know whether such
approach is acceptable.


> - *   Support routines for parallel vacuum execution.
> + *   Support routines for parallel [auto]vacuum execution.
>
> The patch includes the change of "vacuum" -> "[auto]vacuum" in many
> places. While I think we need to mention that vacuumparallel.c
> supports autovacuums I'm not sure we really need all of them. If we
> accept this style, we would require for all subsequent changes to
> follow it, which could increase maintenance costs.
>

Agree. I'll leave a comment which says that vacuumparallel also supports
parallel autovacuum. All other changes like "[auto]vacuum" will be deleted.

> ---
> @@ -299,6 +301,7 @@ typedef struct
>     WorkerInfo  av_startingWorker;
>     AutoVacuumWorkItem av_workItems[NUM_WORKITEMS];
>     pg_atomic_uint32 av_nworkersForBalance;
> +   uint32 av_available_parallel_workers;
>
> Other field names seem to have consistent naming rules; 'av_' prefix
> followed by name in camel case. So how about renaming it to
> av_freeParallelWorkers or something along those lines?
>
> ---
> +int
> +ParallelAutoVacuumReserveWorkers(int nworkers)
> +{
>
> Other exposed functions have "AutoVacuum" prefix, so how about
> renaming it to AutoVacuumReserveParallelWorkers() or something along
> those lines?
>

Agreeing with both comments, I'll rename the structure field and functions.

> ---
> +   if (AutoVacuumShmem->av_available_parallel_workers < nworkers)
> +   {
> +       /* Provide as many workers as we can. */
> +       can_launch = AutoVacuumShmem->av_available_parallel_workers;
> +       AutoVacuumShmem->av_available_parallel_workers = 0;
> +   }
> +   else
> +   {
> +       /* OK, we can provide all requested workers. */
> +       can_launch = nworkers;
> +       AutoVacuumShmem->av_available_parallel_workers -= nworkers;
> +   }
>
> Can we simplify this logic as follows?
>
>  can_launch = Min(AutoVacuumShmem->av_available_parallel_workers, nworkers);
>  AutoVacuumShmem->av_available_parallel_workers -= can_launch;
>

Sure, I'll simplify it.

---
Thank you very much for your comments! Please, see v7 patch :
1) Rename few functions and variables + get rid of comments like
"[auto]vacuum" in vacuumparallel.c
2) Simplified logic in 'parallel_vacuum_init' and
'AutoVacuumReserveParallelWorkers' functions
3) Refactor and bug fix in 'parallel_vacuum_process_all_indexes' function
4) Change "planned vs. launched" logging, so it can be translated
5) Rebased on newest commit in master branch

--
Best regards,
Daniil Davydov