Re: POC: Parallel processing of indexes in autovacuum

Hi,

On Sat, Jan 17, 2026 at 5:20 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Looking at AutoVacuumReserveParallelWorkers(), it seems that we don't
> check the av_maxParallelWorkers() there. Is it possible that two more
> workers would be reserved even while the existing 2 workers are
> running?
>
>  /* Provide as many workers as we can. */
>  *nworkers = Min(AutoVacuumShmem->av_freeParallelWorkers, *nworkers);
>  AutoVacuumShmem->av_freeParallelWorkers -= *nworkers;
>

OK, I got it. You are right. I'll use the formula that you mentioned in the
previous letter. I'll also add a test for it.

>
> +       /* Release all the reserved parallel workers for autovacuum */
> +       if (AmAutoVacuumWorkerProcess() && pvs->pcxt->nworkers_launched > 0)
> +           AutoVacuumReleaseParallelWorkers(pvs->pcxt->nworkers_launched);
>
> Since we want to release all reserved workers here, I think it's clear
> if we use AutoVacuumReleaseAllParallelWorkers() and we add
> Assert(av_nworkers_reserved == 0) at the end of
> AutoVacuumReleaseAllParallelWorkers(). This way, we can ensure that
> all workers are released and it makes the codes more readable. What do
> you think?
>

Agree that this will be more clear for the readers.

> I've attached the patch proposing this change (please find
> v19-0001_masahiko.patch).

Thank you, I'll apply this patch. A few things in the patch that I changed :
1)
+ * The caller must call AutoVacuumReleaseParallelWorkers() to release the...
I think that we also should mention AutoVacuumReleaseAllParallelWorkers.
2)
+ * Similar to AutoVacuumReleaseParallelWorkers(), but this function releases...
If you don't mind, I'll leave the "same as above" formulation since this is
typical for the postgres code.

>
> +#autovacuum_max_parallel_workers = 2    # disabled by default and limited by
> +                                        # max_worker_processes
>
> It's odd to me that the comment says it's disabled by default while
> being set to 2. I think we can rewrite the comment to:
>
> +#autovacuum_max_parallel_workers = 2    # limited by max_worker_processes
>

Good catch. I forgot to change this comment.

> BTW it seems to me that this GUC should be capped by
> max_parallel_workers instead of max_worker_processes, no?
>

I explained my point about it here [1] and here [2]. What do you think?

> ---
> +  long_desc => 'This parameter is capped by "max_worker_processes"
> (not by "autovacuum_max_workers"!).',
>
> I'm concerned that this kind of description might not be appropriate
> to the description in long_desc. Looking at long_desc contents of
> other GUC parameters, we describe the detail of the parameters (e.g.,
> "0 means xxx" or detailed explanation of the effect). Probably we can
> remove this line.
>

Agree.

> >
> > Previously I used the NULL value of this pointer as a flag that we don't need
> > to log workers usage. Now I'll add boolean flag for this purpose (IIUC,
> > "nplanned > 0" condition is not enough to determine whether we should log
> > workers usage, because VACUUM PARALLEL can be called without VERBOSE).
>
> Can't we simply not report the worker usage if nplanned is 0?
>
> >
> > As I wrote above - we don't need to log workers usage if the VERBOSE option
> > is not specified (even if nplanned > 0). Am I missing something?
>
> No. My point is that even when the VERBOSE option is specified, we can
> skip reporting the worker usage if the parallel vacuum is not even
> planned. That is, I think we can do like:
>
>     if (vacrel->workers_usage.nplanned > 0)
>         appendStringInfo(&buf,
>                          _("parallel index vacuum/cleanup: %d workers
> were planned and %d workers were launched in total\n"),
>                          vacrel->workers_usage.nplanned,
>                          vacrel->workers_usage.nlaunched);
>

Sorry, I forgot that we accumulate the messages for logging only if
"instrument == true". It will cut off the case when manual vacuum is
called without the VERBOSE option.

>
> Thank you for updating the patch. I think that we need the explanation
> of what nlaunched and nplanned actually mean in the PVWorkersUsage
> definition
>
> +typedef struct PVWorkersUsage
> +{
> +   int         nlaunched;
> +   int         nplanned;
> +} PVWorkersUsage;
>
> I'm concerned that readers might be confused that nplanned is not the
> number of parallel workers we actually planned to launch.
>
> Or it might make sense to track these three values: planned, reserved,
> and launched. For example, suppose max_worker_processes = 10 and
> autovacuum_max_parallel_workers = 5, if two autovacuum workers try to
> reserve 3 workers each, one worker can reserve and launch 3 and
> another worker can reserve and launch 2. The autovacuum logs would be
> "3 planned and 3 launched" and "3 planned and 2 launched". Users can
> deal with the shortage of parallel workers by increasing
> autovacuum_max_parallel_workers. On the other hand, if some bgworkers
> are being used by other components (.e.g, parallel queries, logical
> replication etc.) and there are only 2 free bgworkers, the autovacuum
> worker can reserve 3 but can launch only 2, and other worker can
> reserve 2 but cannot launch any workers. The autovacuum logs would be
> "3 planned and 2 launched" and "3 planned and 0 launched". Here
> increasing autovacuum_max_parallel_workers resolves the shortage of
> parallel workers, but users would have to increase
> max_worker_processes instead. If we can report the worker usage like
> "3 planned, 3 reserved, and 2 launched" and "3 planned, 2 reserved,
> and 0 launched", users would realize the need to increase
> max_worker_processes. Of course, the "xxx reserved" information would
> not be necessary for VACUUM VERBOSE logs.
>

Hm, I think that reporting of "nreserved" would make it easier for the user to
understand what is going on. Thanks for the detailed explanation, I'll
implement it.

> >
> > We already have such check inside the "fix_cost_based" function :
> > /* Check whether we are running parallel autovacuum */
> > if (pv_shared_cost_params == NULL)
> >     return false;
> >
> > We also have this comment:
> > * If we are autovacuum parallel worker, check whether cost-based
> > * parameters had changed in leader worker.
> >
> > As an alternative, I'll add comment explicitly saying that process will
> > immediately return if it not parallel autovacuum participant.
>
> Why don't we add IsInParallelMode() or IsParallelWorker() check before
> calling parallel_vacuum_update_shared_delay_params()?

Considering your suggestion below, I will add this check.

>
> +bool
> +parallel_vacuum_update_shared_delay_params(void)
> +{
> +   /* Check whether we are running parallel autovacuum */
> +   if (pv_shared_cost_params == NULL)
> +       return false;
> +
> +   Assert(IsParallelWorker() && !AmAutoVacuumWorkerProcess());
>
> These codes are a bit odd to me in two points:
>
> 1. A process can never be both a parallel worker and an autovacuum worker.
>
> 2. If pv_shared_cost_parame == NULL, even autovacuum workers and
> non-parallel workers can call this function, but it seems to be
> unexpected function call given the subsequent assertion. If we want to
> have an assertion to ensure that a function is called only by
> processes we expect or allow, I think we should add an assertion to
> the beginning of function. How about rewriting these parts to:
>
>     Assert(IsParallelWorker());
>
>     /* Check whether we are running parallel autovacuum */
>     if (pv_shared_cost_params == NULL)
>         return false;
>

Agree, it will be much more clear.

> ---
> +        * Note, that this function has no effect if we are non-autovacuum
> +        * parallel worker.
> +        */
>
> I don't think this kind of comment should be noted here since if we
> change the parallel_vacuum_update_shared_delay_params() behavior in
> the future, such comments would get easily out-of-sync.
>

If behavior will be changed, then all comments for this function will need to
be changed, actually. Don't get me wrong - I just think that this Note is
important for the readers. But if you doubt its usefulness, I don't
mind deleting it.

> > > IIUC autovacuum parallel workers seems to update their
> > > vacuum_cost_{delay|limit} every vacuum_delay_point(), which seems not
> > > good. Can we somehow avoid unnecessary updates?
> >
> > More precisely, parallel worker *reads* leader's parameters every delay_point.
> > Obviously, this does not mean that the parameters will necessarily be updated.
> >
> > But I don't see anything wrong with this logic. We just every time get the most
> > relevant parameters from the leader. Of course we can introduce some
> > signaling mechanism, but it will have the same effect as in the current code.
>
> Although the parameter propagation itself is working correctly, the
> current implementation seems suboptimal performance-wise. Acquiring an
> additional spinlock and updating the local variables for every block
> seems too costly to me. IIUC we would end up incurring these costs
> even when vacuum delays are disabled. I think we need to find a better
> way.
>
> For example, we can have a generation of these parameters. That is,
> the leader increments the generation (stored in PVSharedCostParams)
> whenever updating them after reloading the configuration file, and
> workers maintain its generation of the parameters currently used. If
> the worker's generation < the global generation, it updates its
> parameters along with its generation. I think we can implement the
> generation using pg_atomic_u32, making the check for parameter updates
> lock-free. There might be better ideas, though.
>

OK, I see your point. Considering that we need to check some shared state (in
order to understand whether we should update our params), an atomic variable
seem to be the best solution.


Thank you for the review! Please, see v20 patches. Main changes :
1) Add new formula for freeParallelWorkers computation
2) Add 'nreserved' logging for parallel autovacuum
3) Add atomic variable to speed up checking shared params state change
4) New test for autovacuum_max_parallel_workers parameter change
5) Fully get rid of "custom" injection points in tests

BTW, I think that we need more fixes for documentation, so I'll
take a look at it in the near future.

[1] https://www.postgresql.org/message-id/CAJDiXgiYiX%2BazuR76DcVx8fZn57m_4v6cB14-GW34mWa%3DqudFQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAJDiXgjX%2BbO%3DdEZxpnsh588N3BsQ%3D7MHX3YQSJS6FxqGq4zMqQ%40mail.gmail.com

--
Best regards,
Daniil Davydov