Re: POC: Parallel processing of indexes in autovacuum

Hi,

On Thu, Feb 26, 2026 at 6:59 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> For example, if users want to disable all parallel queries, they can do
> that by setting max_parallel_workers to 0. If parallel vacuum workers
> for autovacuums are taken from max_worker_processes pool (i.e.,
> without max_paralle_workers limit), users would need to set both
> max_parallel_workers and autovacuum_max_parallel_workers to 0.
>

This is kinda off-topic already, but I really want to clarify this question.

If parallel a/v workers are not limited by max_parallel_workers and the
user wants to disable all parallel operations, it is still enough to set
max_parallel_workers to 0. In this case parallel a/v could not acquire any
workers from bgworkers pool, and thus the user's goal is reached (and there
is no need to set autovacuum_max_parallel_workers to 0).

**Comments on the 0002 patch**

>
> + /* Worker usage stats for
> parallel autovacuum. */
> + appendStringInfo(&buf,
> +
> _("parallel index vacuum: %d workers were planned, %d workers were
> reserved and %d workers were launched in total\n"),
> +
> vacrel->workers_usage.vacuum.nplanned,
> +
> vacrel->workers_usage.vacuum.nreserved,
> +
> vacrel->workers_usage.vacuum.nlaunched);
>
> These log messages need to take care of plural forms but it seems to
> be too long if we use errmsg_plural() for each number. So how about
> something like:
>
> parallel workers: index: %d planned, %d reserved, %d launched in total
> parallel workers: cleanup %d planned, %d reserved, %d launched
>
> (Index cleanup is executed at most once so we don't need "in total" in
> the message.)

Oh, I forgot about plural form preservation. Agree with your suggestion.

**Comments on the 0003 patch**

>
> +typedef struct CostParamsData
> +{
> + double cost_delay;
> + int cost_limit;
> + int cost_page_dirty;
> + int cost_page_hit;
> + int cost_page_miss;
> +} CostParamsData;
>
> The name CostParamsData sounds too generic and I guess it could
> conflict with optimizer-related struct names in the future. How about
> renaming it to VacuumDelayParams?

I agree with the idea to rename this structure. But maybe we should rename
it to "VacuumCostParams"? This name conveys the contents of the structure
better, because enabling these parameters is called "VacuumCostActive".

> + SpinLockAcquire(&pv_shared_cost_params->mutex);
> +
> + shared_params_data = pv_shared_cost_params->params_data;
> +
> + VacuumCostDelay = shared_params_data.cost_delay;
> + VacuumCostLimit = shared_params_data.cost_limit;
> + VacuumCostPageDirty = shared_params_data.cost_page_dirty;
> + VacuumCostPageHit = shared_params_data.cost_page_hit;
> + VacuumCostPageMiss = shared_params_data.cost_page_miss;
> +
> + SpinLockRelease(&pv_shared_cost_params->mutex);
>
> If we copy the shared values in pv_shared_cost_params, we should
> release the spinlock earlier, i.e., before updating VacuumCostXXX
> variables. But I don't think we would even need to set these values in
> the local variables in this case as updating 4 local variables is
> fairly cheap.
>

Do you mean that we can release spinlock because we already copied the values
from the shared state to the local variable "shared_params_data"? I added this
variable as an alias for the long string "pv_shared_cost_params->params_data"
and I guess that compiler will get rid of it.

But now it doesn't seem like a good solution to me anymore. I'll get rid of
the local variable and copy the values directly from the shared state
(under spinlock).

> ---
> + FillCostParamsData(&local_params_data);
> + SpinLockAcquire(&pv_shared_cost_params->mutex);
> +
> + if (CostParamsDataEqual(pv_shared_cost_params->params_data,
> + local_params_data))
> + {
>
> IIUC it stores cost-based vacuum delay parameters into the
> local_params_data only for using CostParamsDataEqual() macro. I think
> it's better to directly compare values in pv_shared_cost_params and
> the cost-based vacuum delay parameters.

I agree.

> > > How about renaming it to use_shared_delay_params? I think it conveys
> > > better what the field is used for.
> >
> > I think that we should leave this name, because in the future some other
> > behavior differences may occur between manual VACUUM and autovacuum.
> > If so, we will already have an "am_autovacuum" field which we can use in
> > the code.
> > The existing logic with the "am_autovacuum" name is also LGTM - we should
> > use shared delay params only because we are running parallel autovacuum.
>
> It may occur but we can change the field name when it really comes.
>
> I'm slightly concerned that we've been using am_xxx variables in a
> different way. For instance, am_walsender is a global variable that is
> set to true only in wal sender processes. Also we have a bunch of
> AmXXProcess() macros that checks the global variable MyBackendType, to
> check the kinds of the current process. That is, the subject of 'am'
> is typically the process, I guess. On the other hand,
> am_parallel_autovacuum is stored in DSM space and indicates whether a
> parallel vacuum is invoked by manual VACUUM or autovacuum.

Yeah, I agree that "am_xxx" is not the best choice.
What about a simple "bool is_autovacuum"?

**Comments on the 0004 patch**

> If we write the log "%d parallel autovacuum workers have been
> released" in AutoVacuumReleaseParallelWorkres(), can we simplify both
> tests (4 and 5) further?
>

It won't help the 4th test, because ReleaseParallelWorkers is called
due to both ERROR and shmem_exit, but we want to be sure that
workers are released in the try/catch block (i.e. before the shmem_exit).
I thought that we could pass some additional info to the
"ReleaseAllParallelWorkers" such as "bool error_occured", but I decided
not to do so.

Also, I don't know whether the 5th test needs this log at all, because in
the end we are checking the number of free parallel workers. If a killed
a/v leader doesn't release parallel workers, we'll notice it.

> + if (nworkers > 0)
> +
> INJECTION_POINT("autovacuum-leader-before-indexes-processing", NULL);
>
> I think it's better to use #ifdef USE_INJECTION_POINTS here.
>

Agree. I'll also fix it in vacuumlazy.c

> +#ifdef USE_INJECTION_POINTS
> +/*
> + * Log values of the related to cost-based delay parameters. It is used for
>
> s/values of the related to/values related to/
>

OK

> + * testing purpose.
> + */
> +static void
> +parallel_vacuum_report_cost_based_params(void)
> +{
> + StringInfoData buf;
> +
> + /* Simulate config reload during normal processing */
> + pg_atomic_add_fetch_u32(VacuumActiveNWorkers, 1);
> + vacuum_delay_point(false);
> + pg_atomic_sub_fetch_u32(VacuumActiveNWorkers, 1);
>
> Calling vacuum_delay_point() here feels a bit arbitrary to me. Since
> parallel vacuum workers are calling
> parallel_vacuum_report_cost_based_params() after
> parallel_vacuum_process_safe_indexes(), I think we don't necessarily
> call vacuum_delay_point() here.
>

Sure! It is left from the previous implementation of the test. I'll remove
this call.

> + appendStringInfo(&buf, "Vacuum cost-based delay parameters of
> parallel worker:\n");
> + appendStringInfo(&buf, "vacuum_cost_limit = %d\n",vacuum_cost_limit);
> + appendStringInfo(&buf, "vacuum_cost_delay = %g\n", vacuum_cost_delay);
> + appendStringInfo(&buf, "vacuum_cost_page_miss = %d\n",
> VacuumCostPageMiss);
> + appendStringInfo(&buf, "vacuum_cost_page_dirty = %d\n",
> VacuumCostPageDirty);
> + appendStringInfo(&buf, "vacuum_cost_page_hit = %d\n",
> VacuumCostPageHit);
>
> I'd write these messages directly in elog() instead of using
> StringInfoData, which is simpler and can save palloc()/pfree().
>

OK

> + ereport(DEBUG2, errmsg("%s", buf.data));
>
> Let's use elog() instead of ereport().
>

I suppose this is suggested because we don't want to translate error
messages of DEBUG level. Did I understand you correctly?

> +# Create role with pg_signal_autovacuum_worker for terminating
> autovacuum worker.
> +$node->safe_psql('postgres', qq{
> + CREATE ROLE regress_worker_role;
> + GRANT pg_signal_autovacuum_worker TO regress_worker_role;
> + SET ROLE regress_worker_role;
> +});
> +
> +$node->safe_psql('postgres', qq{
> + SELECT pg_terminate_backend('$av_pid');
> +});
>
> These two safe_psql calls use separate connections, meaning that
> pg_terminate_backend() is executed by the superuser rather than
> regress_worker_role. I think we don't need to create the
> regrss_worker_role and we can use the superuser in this test case.
>

Hm, looks like another one piece of code from my previous attempts to
implement this test. I'll remove it.

> We would add more autovacuum related tests to the test_autovacuum
> directory in the future. Given that the 001_basic.pl focuses on
> parallel vacuum tests, how about renaming it to 001_parallel_vacuum.pl
> or something?
>

Agree, I'll rename it.

> > This time I'll try something experimental - besides the patches I'll also
> > post differences between corresponding patches from v20 and v21.
> > I.e. you can apply v20--v21-diff-for-0001 on the v20-0001 patch and
> > get the v21-0001 patch. There are a lot of changes, so I guess it will
> > help you during review. Please, let me know whether it is useful for you.
>
> It was helpful to easily see the changes from the previous version. Thank you!
>

I'm glad to hear it :) I will keep this tradition alive.

Thank you very much for the review!
Please, see updated set of patches and diffs between v21 and v22.

--
Best regards,
Daniil Davydov