Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vighH-00D9GW-1F for pgsql-hackers@arkaria.postgresql.org; Wed, 21 Jan 2026 22:29:08 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1vighG-009m00-1a for pgsql-hackers@arkaria.postgresql.org; Wed, 21 Jan 2026 22:29:06 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vighG-009lzs-0P for pgsql-hackers@lists.postgresql.org; Wed, 21 Jan 2026 22:29:06 +0000 Received: from mail-lj1-x22d.google.com ([2a00:1450:4864:20::22d]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1vighE-001nSH-08 for pgsql-hackers@lists.postgresql.org; Wed, 21 Jan 2026 22:29:06 +0000 Received: by mail-lj1-x22d.google.com with SMTP id 38308e7fff4ca-385b9be0759so3048451fa.3 for ; Wed, 21 Jan 2026 14:29:03 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1769034541; cv=none; d=google.com; s=arc-20240605; b=SPzKzaIe26kjAeBV0XEjyp2Gtq5EKBKaWKf7WnC4DICh7MWyR6EJ//HfR1cHibS5Gh flwBg+9diycEUgTPAutEh7HP1a9zJzy3sbNMzAtcdEf3cInR0P0im2yels7XRzxRAeEa veDCeFm78M7AoBwhmD1hum5lyheXDiuGozWK5mPsz4HdRkMGOH+y3TiNSf0hFVMCWXde /f4CA38PBvNIVpReUvFFOP9aDdDbjVVjsl2UupxNyZt5Lw6cLugmlfXTZJuDf7rjsc+P s2zkq26GNHwINL2F4QHxIA5Kz9FDGKsEwB5a6sS4ulHhcf6XM+W7hlfxCHEJPuwTOGNx lbrg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=o74SWA0/7p+DOU5d5LVphm3ElnPgFZjhqMoDFfie5Qk=; fh=MmFkhVyF0eHggJ3BJ6Hg7vB3pb+2ZlcGVMtgjUtLbCY=; b=dFDkd1rbTrAD2mpVY/mWTAMJLR6AEObD8MmObvVeX+i5LmuB58WkfU1bDoz1JQe4Ej 8O+WVKX/++NIPVsiymFkWJA9oqcgwHVdpEKS6fN26TYBc139Gdqbn67skVqp6v3TDVfH 8/ZHsv95l25mKdkScfFwFOTJI1d902yU3dIL+ocz9H0G5L46taRLviP/hAm7qO3BIzai VFbIhAW3ZZfJXepVzhKbfn7Rj8BapK2D5La7kX/BilkU4ZwoC6TXUm4kuzux7wWvoFRa 0Izr4+u1BRZtF5/QIPrWUupuis06EglkV2jsjuwFzrDgCkyeRIeisfTIUITv83d/hSAP O7pA==; darn=lists.postgresql.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1769034541; x=1769639341; darn=lists.postgresql.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=o74SWA0/7p+DOU5d5LVphm3ElnPgFZjhqMoDFfie5Qk=; b=cPUHzYo0RG3zI/9jmk/9GNHjIRucHFfE9bIqzp0YHFwHwxla+t/sZVeKeUULGyzBmJ 8tGRlbaWP4qAjAkpkybqxp/MtwXNtL/SIiw2rgbYNrvB3fsz/aJBw+U91p8dxU8ZRsE0 cAPkZd3zLVBbuTLdDVhPcIX4ffM226BET3dn7wzJC3qxhFbECvq+IBoHFQAr/9l2PZ6m g8gquqsVzJ9U14B/lYw4OXAb0L9+ZCFNu8O9sdC1I72shuonXJ4bjapmKyHHclpCFs5E nU2fqgGOYuQEip3+M3wRRZab0R2Dnnohg6KiOH4iTHI2O94MJnN5XsDENnFLdqf2sJO0 mtBw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1769034541; x=1769639341; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=o74SWA0/7p+DOU5d5LVphm3ElnPgFZjhqMoDFfie5Qk=; b=I7nWmH57neVQPODEcpvEHkNlKxvVV3M2u6t72BkF0oA0jcPEOqbHt9w3ZX7Bx7NHuc 7xcKbL75+JcLrrkTrCyv3mrVtV8n0IeX/z0U5RAWRnBFNydRVscHiWtaYeGtjxAN/Wtu fdWL0Gcw3JadFWenzJd1WZdHkPKWvPKT9lm8OIAVpaZe/z4sVtUX7Ozio1iLOOseAbYT WToJsVAU1bvGfVgnQpJ53w5HvZt0JJtJacuAz5Qfw5VE0C1DzPRTSMtXhEG81TAqVHX7 GFmnvE2UKKYOWFKfEHITq2AeTrdfu/ZBU3ku4NpEdWJRDb6QpwX0aTtueuTMf7jysfB/ SUtw== X-Forwarded-Encrypted: i=1; AJvYcCVulQ6VQw7XH4tripMzy73CjSdXjGO9+iIu2fvRHjU9blOo1BYGdyJwJC01U5eIcvdHsm2pzer7lwRFBH5U@lists.postgresql.org X-Gm-Message-State: AOJu0Ywl5fC3m1Z5fUPclZulpKhobTO08uwW5jwnjGp0hP6xJsEv3p+6 9jt3ry/rpJKyzgle70MMRrTyHPBA8E9bHjYg1MuUA5IJUDCkL+ElWCErGAMdKTkH1odCuWIkO7b LlOtrFl17CjRgXPoxvxx1otHKBKnSzyM= X-Gm-Gg: AZuq6aJj15TWgUmeVtpxlBrmuLAsM/LTQUwZZ10uzIjr+/rkLCe50nLAIEWt/8MsI3G /ICHEVjhXgZqar1dEfLscoCzqLkIaXPdZvaZqK/dpvYY/dg7uZCIx3PYWdPsp9MtXAGI4e1ZH26 9N/Sddk9qFk3yM9FuBYcPsMUCW1D406IQM2G2ykMY79S+Tk6LUmTnKsgrow44ogl62yOSavbCP0 PIZO2Vh5s1EtZ1TVdYDLPsQ+TFIXeg1rDGqYPnjyIpxpJD+aKYQoDRRAOqVnverygsLuUEl X-Received: by 2002:a05:651c:221b:b0:336:bd8c:5e53 with SMTP id 38308e7fff4ca-385a53b6c01mr21609491fa.5.1769034541043; Wed, 21 Jan 2026 14:29:01 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Masahiko Sawada Date: Wed, 21 Jan 2026 14:28:24 -0800 X-Gm-Features: AZwV_QhZN0WmQ9KNjI6IFcB8EkVTcysP-Pf47gA-jYrO0jn7ePnHUu6o460KT8k Message-ID: Subject: Re: POC: Parallel processing of indexes in autovacuum To: Daniil Davydov <3danissimo@gmail.com> Cc: Sami Imseih , Alexander Korotkov , Matheus Alcantara , Maxim Orlov , Postgres hackers Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk On Sat, Jan 17, 2026 at 6:52=E2=80=AFAM Daniil Davydov <3danissimo@gmail.co= m> wrote: > > > > > 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 change= d : > 1) > + * The caller must call AutoVacuumReleaseParallelWorkers() to release th= e... > I think that we also should mention AutoVacuumReleaseAllParallelWorkers. > 2) > + * Similar to AutoVacuumReleaseParallelWorkers(), but this function rele= ases... > If you don't mind, I'll leave the "same as above" formulation since this = is > typical for the postgres code. > Agreed. > > > 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? I agree that autovacuum_max_parallel_workers should not be capped by other GUC parametres when setting a value. However, these messages seem not explain why this parameter is limited by max_worker_processes instead of max_parallel_workers. You mentioned: > I will keep the 'max_worker_processes' limit, so autovacuum will not > waste time initializing a parallel context if there is no chance that > the request will succeed. > But it's worth remembering that actually the > 'autovacuum_max_parallel_workers' parameter will always be implicitly > capped by 'max_parallel_workers'. It doesn't make sense to me that we limit autovacuum_max_parallel_workers by max_worker_processes TBH. When users want to have more parallel vacuum workers for autovacuum and the VACUUM command, they would have to consider max_worker_processes, max_parallel_workers, and max_parallel_maintenance_workers separately. Given that max_parallel_workers is controlling the number of max_worker_processes that can be used in parallel operations, I believe that parallel vacuum workers for autovacuum should also be taken from that pool. > > > --- > > + * Note, that this function has no effect if we are non-autovac= uum > > + * 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 nee= d 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. I still could not figure out why it should be mentioned here instead of at the comment of parallel_vacuum_update_shared_delay_params(). Readers can notice that calling parallel_vacuum_update_shared_delay_params() for parallel vacuum worker for the VACUUM command has no effect when reading the function. In my opinion, we should mention here why we call parallel_vacuum_update_shared_delay_params() but should not mention what the called function does because it should have been described in that function. BTW can we expose pv_shared_cost_params so that we can check it in vacuum_delay_point() before trying to call parallel_vacuum_update_shared_delay_params()? > > > > > IIUC autovacuum parallel workers seems to update their > > > > vacuum_cost_{delay|limit} every vacuum_delay_point(), which seems n= ot > > > > good. Can we somehow avoid unnecessary updates? > > > > > > More precisely, parallel worker *reads* leader's parameters every del= ay_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 ge= t 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 curre= nt 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 varia= ble > 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 > The 0001 patch looks mostly good to me except for the above comment (max_worker_processes vs. max_parallel_workers) and the following point: + nfree_workers =3D + autovacuum_max_parallel_workers - prev_max_parallel_workers + + AutoVacuumShmem->av_freeParallelWorkers; + + /* + * Cap or increase number of free parallel workers according to the + * parameter change. + */ + AutoVacuumShmem->av_freeParallelWorkers =3D Max(nfree_workers, 0); + + /* + * Don't allow number of free workers to become less than zero if the + * patameter was decreased. + */ + AutoVacuumShmem->av_freeParallelWorkers =3D + Max(AutoVacuumShmem->av_freeParallelWorkers, 0); Why does it do Max(x, 0) twice? * 0002 patch: + if (vacrel->workers_usage.nplanned > 0 && + AmAutoVacuumWorkerProcess()) + { + /* Worker usage stats for parallel autovacuum */ + appendStringInfo(&buf, + _("parallel index vacuum/cleanup: %d workers were planned, %d workers were reserved and %d workers were launched in total\n"), + vacrel->workers_usage.nplanned, + vacrel->workers_usage.nreserved, + vacrel->workers_usage.nlaunched); + } + else if (vacrel->workers_usage.nplanned > 0) + { + /* Worker usage stats for manual VACUUM (PARALLEL) */ + 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); + } Can we refactoring these codes to: if (vacrel->workers_usage.nplanned > 0) { if (AmAutoVacuumWorkerProcess()) appendStringInfo(...); else appendStringInfo(...); * 0003 patch: + if (!AmAutoVacuumWorkerProcess() && IsParallelWorker()) + { We can just check IsParallelWorker() here. --- +extern void parallel_vacuum_update_shared_delay_params(void); +extern void parallel_vacuum_propagate_cost_based_params(void); I think it's better to have similar names to these functions for consistency and readability. How about the following? parallel_vacuum_update_delay_params(); parallel_vacuum_propagate_delay_params(); --- + + params_generation =3D pg_atomic_read_u32(&pv_shared_cost_params->genera= tion); + + SpinLockAcquire(&pv_shared_cost_params->spinlock); + + pv_shared_cost_params->cost_delay =3D vacuum_cost_delay; + pv_shared_cost_params->cost_limit =3D vacuum_cost_limit; + pv_shared_cost_params->cost_page_dirty =3D VacuumCostPageDirty; + pv_shared_cost_params->cost_page_hit =3D VacuumCostPageHit; + pv_shared_cost_params->cost_page_miss =3D VacuumCostPageMiss; I think we can check if the new cost-based delay parameters are really changed before changing the shared values. If users don't change cost-based delay parameters, we don't need to increment the generation at all. --- + pg_atomic_write_u32(&pv_shared_cost_params->generation, + params_generation + 1); We can use pg_atomic_add_fetch_u32() instead. --- +/* + * Only autovacuum leader can reload config file. We use this structure in + * parallel autovacuum for keeping worker's parameters in sync with leader= 's + * parameters. + */ +typedef struct PVSharedCostParams I'd suggest writing the overall description first (e.g., what the struct holds and what the function does etc), and then describing the details and notes etc. For instance, readers might be confused when reading the first sentence "Only autovacuum leader can reload config file" as the struct definition is not related to the autovacuum implementation fact that autovacuum workers can reload the config file during the work. We would need to mention such detail somewhere in the comments but I think it should not be the first sentence. How about rewriting it to something like: +/* + * Struct for cost-based vacuum delay related parameters to share among an + * autovacuum worker and its parallel vacuum workers. + */ --- + slock_t spinlock; /* protects all fields below */ It's convention to name 'mutex' as a field name. --- +static PVSharedCostParams * pv_shared_cost_params =3D NULL; + +/* See comments for structure above for the explanation. */ +static uint32 shared_params_generation_local =3D 0; I think it's preferable to move these definitions of static variables right before the function prototypes. --- + /* + * If 'true' then we are running parallel autovacuum. Otherwise, we are + * running parallel maintenence VACUUM. + */ + bool am_parallel_autovacuum; How about renaming it to use_shared_delay_params? I think it conveys better what the field is used for. * 0004 patch: The patch introduces 5 injection points, which seems overkill to me for implementing the tests. IIUC we can implement the test2 with two injection points: 'autovacuum-start-parallel-vacuum' (set right before lazy_scan_heap() call) and 'autovacuum-leader-before-indexes-processing'. 1. stop the autovacuum worker at 'autovacuum-start-parallel-vacuum'. 2. change delay params and reload the conf. 3. let the autovacuum worker process tables (vacuum_delay_point() is called during the heap scan). 4. stop the autovacuum worker at 'autovacuum-leader-before-indexes-processi= ng'. 5. let parallel workers process indexes (vacuum_delay_point() is called during index vacuuming). For test3, I think we can write a DEBUG2 log in adjust_free_parallel_workers() and check it during the test instead of introducing the test-only function. For test4 and test5, we check the number of free workers using get_parallel_autovacuum_free_workers(). However, since autovacuum could retry to vacuum the table again, the test could fail. And here are some general comments and suggestions: +use warnings FATAL =3D> 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; We need comments to explain what we test with this test file. --- + $node->safe_psql('postgres', qq{ + UPDATE test_autovac SET col_1 =3D $test_number; + ANALYZE test_autovac; + }); Why do we need to execute ANALYZE as well? --- + $node->wait_for_log($expected_log); + truncate $node->logfile, 0 or die "truncate failed: $!"; +} Truncating all logs every after test would decrease the debuggability much. We can pass the offset as the start point to wait for the contents. --- +# Insert specified tuples num into the table +$node->safe_psql('postgres', qq{ + DO \$\$ + DECLARE + i INTEGER; + BEGIN + FOR i IN 1..$initial_rows_num LOOP + INSERT INTO test_autovac VALUES (i, i + 1, i + 2, i + 3); + END LOOP; + END \$\$; +}); We can use generate_series() here. And it's faster to load the data and then create indexes. --- +$node->psql('postgres', + "SELECT get_parallel_autovacuum_free_workers();", + stdout =3D> \$psql_out, +); Please use pgsql_safe() instead. Regards, --=20 Masahiko Sawada Amazon Web Services: https://aws.amazon.com