Hi, On Wed, Feb 25, 2026 at 02:33:29PM +0100, Peter Eisentraut wrote: > On 24.02.26 19:16, Bertrand Drouvot wrote: > > On Tue, Feb 24, 2026 at 11:19:50AM -0600, Nathan Bossart wrote: > > > On Tue, Feb 24, 2026 at 05:08:09PM +0000, Bertrand Drouvot wrote: > > > > This patch makes use of unvolatize() in vac_truncate_clog(). > > > > > > > > Note that it does not remove the warning but moves it to c.h (where > > > > unvolatize() > > > > is defined) but that's consistent with what 481018f2804 did too. > > > > > > Why is this preferable to marking the function parameter as volatile > > > > I think that that would sound misleading for the other callers that don't > > really > > need the volatile qualification. > > > > > or removing the volatile qualifier from the variable? > > > > That looks mandatory according to 2d2e40e3bef. > > Arguably, putting the volatile qualifier on the whole dbform is broader than > required. So you could imagine writing it something like this instead: > > FormData_pg_database *dbform = (Form_pg_database) GETSTRUCT(tuple); > volatile TransactionId *datfrozenxid_p; > volatile TransactionId *datminmxid_p; > > *datfrozenxid_p = dbform->datfrozenxid; > *datminmxid_p = dbform->datminmxid;
I think that looks like the best option as it also removes completely the volatile qual warning. That's done that way in 0001 (also moving back from FormData_pg_database to Form_pg_database as pre 2d2e40e3bef). Also, I'm using the same pattern in 0002 for vac_update_datfrozenxid() as f65ab862e3b: - was fixing the same kind of race as 2d2e40e3bef was fixing - added a comment for vac_update_datfrozenxid() mentioning the race in vac_truncate_clog(). So that's better if they both use the same pattern. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
>From c118ef37539a1f639b8c5fe27f67f16e7f201fc7 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Wed, 25 Feb 2026 16:12:42 +0000 Subject: [PATCH v2 1/2] Reduce the scope of the volatile qualifier in vac_truncate_clog() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit c66a7d75e652 introduced a new "cast discards ‘volatile’" warning (-Wcast-qual). Instead of making use of unvolatize(), let's remove the warning by reducing the scope of the volatile qualifier (added in 2d2e40e3bef) to only 2 fields. Suggested-by: Peter Eisentraut <[email protected]> Author: Bertrand Drouvot <[email protected]> Reviewed-by: Nathan Bossart <[email protected]> Discussion: https://postgr.es/m/aZ3a%2BV82uSfEjDmD%40ip-10-97-1-34.eu-west-3.compute.internal --- src/backend/commands/vacuum.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 100.0% src/backend/commands/ diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 03932f45c8a..7ff24600fcf 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -1869,9 +1869,11 @@ vac_truncate_clog(TransactionId frozenXID, while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { - volatile FormData_pg_database *dbform = (Form_pg_database) GETSTRUCT(tuple); - TransactionId datfrozenxid = dbform->datfrozenxid; - TransactionId datminmxid = dbform->datminmxid; + Form_pg_database dbform = (Form_pg_database) GETSTRUCT(tuple); + volatile TransactionId *datfrozenxid_p = &dbform->datfrozenxid; + volatile TransactionId *datminmxid_p = &dbform->datminmxid; + TransactionId datfrozenxid = *datfrozenxid_p; + TransactionId datminmxid = *datminmxid_p; Assert(TransactionIdIsNormal(datfrozenxid)); Assert(MultiXactIdIsValid(datminmxid)); -- 2.34.1
>From 2af207662215f2382ae0ce61a4f78c67c7ff6afc Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Wed, 25 Feb 2026 16:37:12 +0000 Subject: [PATCH v2 2/2] Reduce the scope of the volatile qualifier in vac_update_datfrozenxid() Commit xxx reduced the scope of the volatile qualifier in vac_truncate_clog(). Let's do the same for vac_update_datfrozenxid(), since the intent of f65ab862e3b was to prevent the same kind of race condition that 2d2e40e3bef was fixing. Suggested-by: Peter Eisentraut <[email protected]> Author: Bertrand Drouvot <[email protected]> Reviewed-by: Nathan Bossart <[email protected]> Discussion: https://postgr.es/m/aZ3a%2BV82uSfEjDmD%40ip-10-97-1-34.eu-west-3.compute.internal --- src/backend/commands/vacuum.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 100.0% src/backend/commands/ diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 7ff24600fcf..b9840637783 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -1665,9 +1665,11 @@ vac_update_datfrozenxid(void) while ((classTup = systable_getnext(scan)) != NULL) { - volatile FormData_pg_class *classForm = (Form_pg_class) GETSTRUCT(classTup); - TransactionId relfrozenxid = classForm->relfrozenxid; - TransactionId relminmxid = classForm->relminmxid; + Form_pg_class classForm = (Form_pg_class) GETSTRUCT(classTup); + volatile TransactionId *relfrozenxid_p = &classForm->relfrozenxid; + volatile TransactionId *relminmxid_p = &classForm->relminmxid; + TransactionId relfrozenxid = *relfrozenxid_p; + TransactionId relminmxid = *relminmxid_p; /* * Only consider relations able to hold unfrozen XIDs (anything else -- 2.34.1
