public inbox for [email protected]
help / color / mirror / Atom feedFrom: Bertrand Drouvot <[email protected]>
To: Peter Eisentraut <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: [email protected]
Subject: Re: Make use of unvolatize() in vac_truncate_clog()
Date: Wed, 25 Feb 2026 17:49:21 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<aZ3dthiAZ8nP5Cf7@nathan>
<[email protected]>
<[email protected]>
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
Attachments:
[text/x-diff] v2-0001-Reduce-the-scope-of-the-volatile-qualifier-in-vac.patch (1.8K, ../[email protected]/2-v2-0001-Reduce-the-scope-of-the-volatile-qualifier-in-vac.patch)
download | inline diff:
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
[text/x-diff] v2-0002-Reduce-the-scope-of-the-volatile-qualifier-in-vac.patch (1.7K, ../[email protected]/3-v2-0002-Reduce-the-scope-of-the-volatile-qualifier-in-vac.patch)
download | inline diff:
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
view thread (5+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Make use of unvolatize() in vac_truncate_clog()
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox