Обсуждение: Make use of unvolatize() in vac_truncate_clog()

Hi hackers,

while reviewing [1], I noticed a remaining "cast discards ‘volatile’" outside of
c.h:

$ grep " warning: cast discards ‘volatile’" make.log
vacuum.c:1885:46: warning: cast discards ‘volatile’ qualifier from pointer target type [-Wcast-qual]
c.h:1263:10: warning: cast discards ‘volatile’ qualifier from pointer target type [-Wcast-qual]
c.h:1039:35: warning: cast discards ‘volatile’ qualifier from pointer target type [-Wcast-qual]
c.h:1039:35: warning: cast discards ‘volatile’ qualifier from pointer target type [-Wcast-qual]
c.h:1263:10: warning: cast discards ‘volatile’ qualifier from pointer target type [-Wcast-qual]
c.h:1263:10: warning: cast discards ‘volatile’ qualifier from pointer target type [-Wcast-qual]
c.h:1263:10: warning: cast discards ‘volatile’ qualifier from pointer target type [-Wcast-qual]

That indicated that unvolatize() is not being used in vacuum.c. Indeed, 481018f2804
introduced unvolatize() but its usage has been missed in c66a7d75e652.

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.

[1]: https://postgr.es/m/aZw4fcj1qBYgN41V%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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

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)