Make use of unvolatize() in vac_truncate_clog()

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 or
removing the volatile qualifier from the variable?

--
nathan

Hi,

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.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

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;

Alternatively, we should document why applying unvolatize() is
acceptable, like

/* ... unvolatize() is ok because datconnlimit is not concurrently
updated (see above) ...

Or just write it directly like

if (dbform->datconnlimit == DATCONNLIMIT_INVALID_DB)

This violates the abstraction layer, but the variant with the comment
kind of does too.

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

On 25.02.26 18:49, Bertrand Drouvot wrote:

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.

Committed (squashed into one patch)