Replace pg_atomic_flag with pg_atomic_bool
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t252994psql -h localhost -U postgresBuilt from patchset v1 (message #1), September 20, 2026 at 06:47 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t252994_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t252994_1 && git checkout t252994_1Patchset v1 (message #1) is on t252994_1
Hi,
I've always found the pg_atomic_flag functions counterintuitive. I
remember by now pretty well how the pg_atomic_u32/u64 functions work,
but any time I have to look at the pg_atomic_flag functions, I feel
not-like I'm in a foreign land.
I think that's because I think of pg_atomic_flag as an atomic version of
"bool". I'd assume there to be functions similar to the u32/u64
functions, like pg_atomic_read_bool() and pg_atomic_write_bool(). But
alas, neither of those functions exist. To read the value, you have to
use pg_atomic_unlocked_test_flag(), and remember that when the flag is
set, it returns *false*, which feels completely backwards to me. It
makes more sense with pg_atomic_test_set_flag(), which returns 'false'
if the flag was already set and hence was not set again, but it
nevertheless just feels wrong to me.
I propose that we replace pg_atomic_flag with pg_atomic_bool, which
behaves more like pg_atomic_u32/u64.
We currently only use pg_atomic_bool in two places: in the shared memory
structs of pg_stash_advise and autovacuum. I actually considered just
removing pg_test_flag altogether and replacing those two uses directly
with pg_atomic_u32, but it's still nice to have a distinct boolean type.
It's worth noting that the current usage is not very performance
critical (not that I'd expect the u32 functions to be any slower).
Thomas's patches to switch to standard C <stdatomic.h> at [1]/messages/by-id/CA+hUKGKFvu3zyvv3aaj5hHs9VtWcjFAmisOwOc7aOZNc5AF3NA@mail.gmail.com is also
touching this. They keep the the application-facing functions unchanged,
but changes the implementation to use the same atomic-exchange
instructions as for u32/u64. This is complementary and an independent
topic to discuss, although the patches will conflict.
[1]: /messages/by-id/CA+hUKGKFvu3zyvv3aaj5hHs9VtWcjFAmisOwOc7aOZNc5AF3NA@mail.gmail.com
/messages/by-id/CA+hUKGKFvu3zyvv3aaj5hHs9VtWcjFAmisOwOc7aOZNc5AF3NA@mail.gmail.com
- Heikki
On Mon, Jul 6, 2026 at 3:57 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I think that's because I think of pg_atomic_flag as an atomic version of
"bool". I'd assume there to be functions similar to the u32/u64
functions, like pg_atomic_read_bool() and pg_atomic_write_bool(). But
alas, neither of those functions exist. To read the value, you have to
use pg_atomic_unlocked_test_flag(), and remember that when the flag is
set, it returns *false*, which feels completely backwards to me. It
makes more sense with pg_atomic_test_set_flag(), which returns 'false'
if the flag was already set and hence was not set again, but it
nevertheless just feels wrong to me.
I am using pg_atomic_test_set_flag() as a non-wait locking, lighter
than LWLock mechanism in shared buffer resizing patches. That might
change in future. But I think there's use for TAS kind of semantics.
But I have also got confused when looking at it as pg_atomic_bool. I
think we need both.
--
Best Wishes,
Ashutosh Bapat
On 06/07/2026 18:05, Ashutosh Bapat wrote:
On Mon, Jul 6, 2026 at 3:57 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I think that's because I think of pg_atomic_flag as an atomic version of
"bool". I'd assume there to be functions similar to the u32/u64
functions, like pg_atomic_read_bool() and pg_atomic_write_bool(). But
alas, neither of those functions exist. To read the value, you have to
use pg_atomic_unlocked_test_flag(), and remember that when the flag is
set, it returns *false*, which feels completely backwards to me. It
makes more sense with pg_atomic_test_set_flag(), which returns 'false'
if the flag was already set and hence was not set again, but it
nevertheless just feels wrong to me.I am using pg_atomic_test_set_flag() as a non-wait locking, lighter
than LWLock mechanism in shared buffer resizing patches. That might
change in future. But I think there's use for TAS kind of semantics.
But I have also got confused when looking at it as pg_atomic_bool. I
think we need both.
The functionality is still there with my patch, as the
pg_atomic_exchange_bool() function. Would that work for you?
- Heikki
On Mon, Jul 6, 2026 at 9:31 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 06/07/2026 18:05, Ashutosh Bapat wrote:
On Mon, Jul 6, 2026 at 3:57 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I think that's because I think of pg_atomic_flag as an atomic version of
"bool". I'd assume there to be functions similar to the u32/u64
functions, like pg_atomic_read_bool() and pg_atomic_write_bool(). But
alas, neither of those functions exist. To read the value, you have to
use pg_atomic_unlocked_test_flag(), and remember that when the flag is
set, it returns *false*, which feels completely backwards to me. It
makes more sense with pg_atomic_test_set_flag(), which returns 'false'
if the flag was already set and hence was not set again, but it
nevertheless just feels wrong to me.I am using pg_atomic_test_set_flag() as a non-wait locking, lighter
than LWLock mechanism in shared buffer resizing patches. That might
change in future. But I think there's use for TAS kind of semantics.
But I have also got confused when looking at it as pg_atomic_bool. I
think we need both.The functionality is still there with my patch, as the
pg_atomic_exchange_bool() function. Would that work for you?
I think I can use it by replacing pg_atomic_test_set_flag(ab) by
!pg_atomic_exchange_bool(ab, true). I haven't tried it. Maybe just
define pg_atomic_test_set_flag(ab) as !pg_atomic_exchange_bool(ab,
true) to be readable? Right now, none of the callers of
pg_atomic_test_set_flag() check its return value (thus use it as TAS).
So maybe we can add pg_atomic_test_set_flag() as a separate patch.
--
Best Wishes,
Ashutosh Bapat