convert various variables to atomics

On 09.07.26 22:50, Nathan Bossart wrote:

The attached patch set converts various variables to atomics, thereby
allowing us to remove a handful of spinlocks and volatile qualifiers.
We've been slowly moving in this direction for a while already. I think
all of these are pretty straightforward and easy to reason about.

A number of these change signed integers to unsigned integers. Maybe
this doesn't matter in some cases, but it should be analyzed in more detail.

Here is an instance that seems obviously wrong:

  	/* Buffer id of the buffer that Startup process waits for pin on, or 
-1 */
-	int			startupBufferPinWaitBufId;
+	pg_atomic_uint32 startupBufferPinWaitBufId;

On Wed, Jul 22, 2026 at 02:31:49PM +0200, Peter Eisentraut wrote:

Here is an instance that seems obviously wrong:

/* Buffer id of the buffer that Startup process waits for pin on, or -1 */
-	int			startupBufferPinWaitBufId;
+	pg_atomic_uint32 startupBufferPinWaitBufId;

I may just be undercaffeinated, but what is wrong with this case? AFAICT
the casting should work as expected, and I see other examples that do
something similar, like avLauncherProc.

--
nathan

Hi,

On 2026-07-22 09:06:44 -0400, Nathan Bossart wrote:

On Wed, Jul 22, 2026 at 02:31:49PM +0200, Peter Eisentraut wrote:

Here is an instance that seems obviously wrong:

/* Buffer id of the buffer that Startup process waits for pin on, or -1 */
-	int			startupBufferPinWaitBufId;
+	pg_atomic_uint32 startupBufferPinWaitBufId;

I may just be undercaffeinated, but what is wrong with this case? AFAICT
the casting should work as expected, and I see other examples that do
something similar, like avLauncherProc.

The comment says -1, which doesn't really make sense for an unsigned variable.

Greetings,

Andres Freund

On Wed, Jul 22, 2026 at 09:19:39AM -0400, Andres Freund wrote:

On 2026-07-22 09:06:44 -0400, Nathan Bossart wrote:

/* Buffer id of the buffer that Startup process waits for pin on, or -1 */
-	int			startupBufferPinWaitBufId;
+	pg_atomic_uint32 startupBufferPinWaitBufId;

I may just be undercaffeinated, but what is wrong with this case? AFAICT
the casting should work as expected, and I see other examples that do
something similar, like avLauncherProc.

The comment says -1, which doesn't really make sense for an unsigned variable.

Ah. It looks like we could use 0 as the sentinel and simplify the call
sites. They subtract one before calling SetStartupBufferPinWaitBufId() and
add one after calling GetStartupBufferPinWaitBufId().

--
nathan

On Wed, Jul 22, 2026 at 09:31:49AM -0400, Nathan Bossart wrote:

Ah. It looks like we could use 0 as the sentinel and simplify the call
sites. They subtract one before calling SetStartupBufferPinWaitBufId() and
add one after calling GetStartupBufferPinWaitBufId().

I added a new prerequisite patch (v2-0004) that does this.

--
nathan

Hi Nathan,

I tested the complete v1 patch series on PostgreSQL 20devel.
The patches applied cleanly, and PostgreSQL built and installed
successfully. The server started without any issues after applying the
patches.
I performed functional testing for the areas affected by the patch
series, including shared invalidation, parallel bitmap heap scan,
parallel WAL state handling, startup process shared state, parallel
sort, SharedFileSet temporary file handling, parallel sequential scan,
and fast-path relation locking. For each patch, I compared the
behavior before and after applying the changes and observed no
functional differences. All tests behaved as expected, and I did not
encounter any crashes, assertion failures, or unexpected behavior.

I also ran the regression test suite using make check. All regression
tests passed successfully, and the generated regression.
Overall, I did not observe any functional regressions during testing.
The patch series looks good from my testing.

Thank you for working on this improvement.

Regards,
Solai

A number of these change signed integers to unsigned integers. Maybe
this doesn't matter in some cases, but it should be analyzed in more detail.

I only see two other places (maxMsgNum and currentWorker/workersFinished), both seem to be a safe conversion to me, and generally the patch looks good.

On 23.07.26 21:56, Nathan Bossart wrote:

On Wed, Jul 22, 2026 at 09:31:49AM -0400, Nathan Bossart wrote:

Ah. It looks like we could use 0 as the sentinel and simplify the call
sites. They subtract one before calling SetStartupBufferPinWaitBufId() and
add one after calling GetStartupBufferPinWaitBufId().

I added a new prerequisite patch (v2-0004) that does this.

Maybe this is okay, but there are a bunch more places (not touched by
your patches) that mix unsigned atomics operations with actually signed
values. Stuff like PIDs and proc numbers. I think for better overall
hygiene and to simplify broader adoption, perhaps we should introduce
support for signed atomic variables.

Hi,

On 2026-08-04 16:07:58 +0200, Peter Eisentraut wrote:

On 23.07.26 21:56, Nathan Bossart wrote:

On Wed, Jul 22, 2026 at 09:31:49AM -0400, Nathan Bossart wrote:

Ah. It looks like we could use 0 as the sentinel and simplify the call
sites. They subtract one before calling SetStartupBufferPinWaitBufId() and
add one after calling GetStartupBufferPinWaitBufId().

I added a new prerequisite patch (v2-0004) that does this.

Maybe this is okay, but there are a bunch more places (not touched by your
patches) that mix unsigned atomics operations with actually signed values.
Stuff like PIDs and proc numbers. I think for better overall hygiene and to
simplify broader adoption, perhaps we should introduce support for signed
atomic variables.

I'm quite hesitant to do that, at least without a lot more clear cut examples
where it actually would make the code better. I think it's rarely a good idea
to use signed variables for atomics, because you get undefined behaviour on
overflow, there's problems with bit masking, etc. IME most data in atomically
modified should actually be unsigned and probably should have been unsigned
before the conversion to atomics.

Greetings,

Andres Freund

On 04.08.26 16:32, Andres Freund wrote:

Maybe this is okay, but there are a bunch more places (not touched by your
patches) that mix unsigned atomics operations with actually signed values.
Stuff like PIDs and proc numbers. I think for better overall hygiene and to
simplify broader adoption, perhaps we should introduce support for signed
atomic variables.

I'm quite hesitant to do that, at least without a lot more clear cut examples
where it actually would make the code better. I think it's rarely a good idea
to use signed variables for atomics, because you get undefined behaviour on
overflow, there's problems with bit masking, etc. IME most data in atomically
modified should actually be unsigned and probably should have been unsigned
before the conversion to atomics.

Yeah, using all unsigned would be cleaner.

I wonder what to do about this kind of suspicious-looking code that
mixes unsigned and signed:

Assert(pg_atomic_read_u32(&proc->clogGroupNext) == INVALID_PROC_NUMBER);

where

#define INVALID_PROC_NUMBER (-1)

and similarly this kind of thing

if (pg_atomic_read_u32(&slot->pss_pid) == pid)

(where pid is either pid_t or int).

We could make ProcNumber typedef'ed as unsigned instead and make
INVALID_PROC_NUMBER be UINT_MAX. That's what it effectively does now,
but that way it would be less mysterious.

(I suppose the PID stuff might go away/change significantly eventually
as part of thread stuff, but we'd probably still want an invalid/not-set
value.)

On 04/08/2026 17:32, Andres Freund wrote:

On 2026-08-04 16:07:58 +0200, Peter Eisentraut wrote:

On 23.07.26 21:56, Nathan Bossart wrote:

On Wed, Jul 22, 2026 at 09:31:49AM -0400, Nathan Bossart wrote:

Ah. It looks like we could use 0 as the sentinel and simplify the call
sites. They subtract one before calling SetStartupBufferPinWaitBufId() and
add one after calling GetStartupBufferPinWaitBufId().

I added a new prerequisite patch (v2-0004) that does this.

Maybe this is okay, but there are a bunch more places (not touched by your
patches) that mix unsigned atomics operations with actually signed values.
Stuff like PIDs and proc numbers. I think for better overall hygiene and to
simplify broader adoption, perhaps we should introduce support for signed
atomic variables.

I'm quite hesitant to do that, at least without a lot more clear cut examples
where it actually would make the code better. I think it's rarely a good idea
to use signed variables for atomics, because you get undefined behaviour on
overflow, there's problems with bit masking, etc. IME most data in atomically
modified should actually be unsigned and probably should have been unsigned
before the conversion to atomics.

We could provide pg_atomic_read/write_i32() and
pg_atomic_compare_exchange_i32() but leave out fetch-and-add and other
such instructions that have overflow or bit masking issues.

- Heikki

Hi Nathan, Heikki,

This entry is currently Ready for Committer, but the last exchange seems
to leave the signed-atomic API question open. Andres objected to the
general form, and Heikki suggested exposing only read, write and CAS.

Is the current patchset still the intended design, or should it be
updated after that discussion? I have moved the entry back to Needs
Review until that is clear.

Thank you!

Best regards, Andrey Borodin.

On Sun, Aug 30, 2026 at 10:54:36PM +0500, Andrey Borodin wrote:

This entry is currently Ready for Committer, but the last exchange seems
to leave the signed-atomic API question open. Andres objected to the
general form, and Heikki suggested exposing only read, write and CAS.

Is the current patchset still the intended design, or should it be
updated after that discussion? I have moved the entry back to Needs
Review until that is clear.

My interpretation is that the signed atomics stuff would be a follow-up
effort. I'm still planning to commit the v2 patch set soon.

If you have questions about the status of one of my commitfest entries,
please ask before you change it.

--
nathan

Hi Nathan,

On Sun, Aug 30, 2026, Nathan Bossart wrote:

My interpretation is that the signed atomics stuff would be a follow-up
effort. I'm still planning to commit the v2 patch set soon.

Thanks, that answers my question.

If you have questions about the status of one of my commitfest entries,
please ask before you change it.

Of course, you can commit whatever you consider ready. My question was
about if the latest feedback had been resolved.

I did ask in the thread. I changed the status at the same time because
Ready for Committer did not seem accurate until that question had an
answer. There were six messages after v2 without a response from you:
two positive reviews, followed by the design discussion between Peter,
Andres, and Heikki.

Commitfest entries are there to collect feedback, and their statuses
should reflect whether that feedback has been addressed. What would be
the point of registering the entry otherwise?

You have now clarified that this is follow-up work, so Ready for
Committer accurately describes your intent.

Thank you!

Best regards, Andrey Borodin.

I committed v2-{0003,0004,0008,0009}, and I looked closer at the signed
versus unsigned mismatches and determined the following:

* v2-0001: We are changing a variable from signed to unsigned, but the code
goes out of its way to avoid negative values and signed integer overflow,
so I don't think there are any real problems here. The only atomic
arithmetic operation is in SICleanupQueue() where we subtract
MSGNUMWRAPAROUND, which IIUC should never produce a negative value. That
being said, I don't think it would be too disruptive to switch all relevant
variables to uint32 as a prerequisite patch. I don't see any particular
reason for those variables to be signed, anyway.

* v2-0002: The variable in question stores a value from the
SharedBitmapState enum. There's no atomic arithmetic involved: we just
write and compare-exchange. At a glance, I didn't see any existing
examples of using enum values for an atomic variable, but I think it's
fine. I believe the C standard guarantees the enum values will be 0, 1, 2,
etc., and even if we did set some enumeration constants to negative values,
it wouldn't matter because we aren't doing arithmetic with it (and are
probably unlikely to anytime soon). So, IMHO this one is fine as-is.

* v2-0005: Since 0004 is committed, startupBufferPinWaitBuf is now a
Buffer. Buffer is still a signed integer, but since we don't set
startupBufferPinWaitBuf to a local buffer (only to a shared buffer or
InvalidBuffer (0)), it'll always be >= 0. Furthermore, we don't do any
sort of atomic arithmetic with this variable; it's hidden behind setter and
getter functions. I think this one is fine.

* v2-0006: The variables in this one are only ever incremented by 1, and
they track the number of workers for a given operation, which I can't
imagine approaches anything even close to overflowing an integer. Not to
mention that we're using signed integers for all the relevant variables
today... I don't see any risk here, but I'll try to switch the relevant
variables to unsigned as a prerequisite and see how it looks. If it's too
invasive, it's probably not worth worrying about.

* v2-0007: I think this one already does all the work to avoid any signed
versus unsigned mismatches. The Assert() in SharedFileSetOnDetach() looks
bogus, though, so I'll fix that. I guess there could be some risk of
overflow in the "refcnt + 1" in SharedFileSetAttach(), but we don't handle
that at all today, so I don't think we need to worry about it. (In theory
this patch actually reduces the overflow risk by switching to unsigned,
anyway.)

--
nathan

On Thu, Aug 06, 2026 at 11:37:46AM +0300, Heikki Linnakangas wrote:

On 04/08/2026 17:32, Andres Freund wrote:

I'm quite hesitant to do that, at least without a lot more clear cut examples
where it actually would make the code better. I think it's rarely a good idea
to use signed variables for atomics, because you get undefined behaviour on
overflow, there's problems with bit masking, etc. IME most data in atomically
modified should actually be unsigned and probably should have been unsigned
before the conversion to atomics.

We could provide pg_atomic_read/write_i32() and
pg_atomic_compare_exchange_i32() but leave out fetch-and-add and other such
instructions that have overflow or bit masking issues.

I would do both of these, i.e., first try switching to unsigned, and if
that's not an option for whatever reason, use signed atomics. If those
existed, I'd use them for v2-0002, which uses an atomic variable for an
enum value, and v2-0005, which uses an atomic variable for a Buffer.
Neither needs to do any sort of atomic arithmetic on the value, so the lack
of fetch-and-add, etc., isn't a problem.

That being said, adding signed atomics just for these small patches seems
rather extreme, so unless we see ourselves using them quite a bit more down
the road, my feeling is that the juice isn't worth the squeeze. I'm
curious how others feel about this.

--
nathan

On Tue, Sep 08, 2026 at 12:00:47PM -0500, Nathan Bossart wrote:

* v2-0001: We are changing a variable from signed to unsigned, but the code
goes out of its way to avoid negative values and signed integer overflow,
so I don't think there are any real problems here. The only atomic
arithmetic operation is in SICleanupQueue() where we subtract
MSGNUMWRAPAROUND, which IIUC should never produce a negative value. That
being said, I don't think it would be too disruptive to switch all relevant
variables to uint32 as a prerequisite patch. I don't see any particular
reason for those variables to be signed, anyway.

v3-0001 is the prerequisite patch. This requires some new clamping logic
in SICleanupQueue() for minsig and lowbound, since the subtractions can
produce negative values. I believe this retains the existing behavior, but
need to double-check.

* v2-0006: The variables in this one are only ever incremented by 1, and
they track the number of workers for a given operation, which I can't
imagine approaches anything even close to overflowing an integer. Not to
mention that we're using signed integers for all the relevant variables
today... I don't see any risk here, but I'll try to switch the relevant
variables to unsigned as a prerequisite and see how it looks. If it's too
invasive, it's probably not worth worrying about.

Yeah, this looks far too invasive. I left it alone.

* v2-0007: I think this one already does all the work to avoid any signed
versus unsigned mismatches. The Assert() in SharedFileSetOnDetach() looks
bogus, though, so I'll fix that. I guess there could be some risk of
overflow in the "refcnt + 1" in SharedFileSetAttach(), but we don't handle
that at all today, so I don't think we need to worry about it. (In theory
this patch actually reduces the overflow risk by switching to unsigned,
anyway.)

Upon closer inspection, the Assert() looks fine. I'm not sure why I
thought it was bogus.

--
nathan

08.09.2026 22:49, Nathan Bossart пишет:

On Tue, Sep 08, 2026 at 12:00:47PM -0500, Nathan Bossart wrote:

* v2-0001: We are changing a variable from signed to unsigned, but the code
goes out of its way to avoid negative values and signed integer overflow,
so I don't think there are any real problems here. The only atomic
arithmetic operation is in SICleanupQueue() where we subtract
MSGNUMWRAPAROUND, which IIUC should never produce a negative value. That
being said, I don't think it would be too disruptive to switch all relevant
variables to uint32 as a prerequisite patch. I don't see any particular
reason for those variables to be signed, anyway.

v3-0001 is the prerequisite patch. This requires some new clamping logic
in SICleanupQueue() for minsig and lowbound, since the subtractions can
produce negative values. I believe this retains the existing behavior, but
need to double-check.

Personally, I don't like current implementation of
pg_atomic_read_membarrier_u32 because it writes into shared variable.

That is why in [1]/messages/by-id/attachment/174633/v3-0001-sinvaladt.c-use-atomic-operations-on-maxMsgNum.patch (thread [2]/messages/by-id/30aa0030-f694-44ef-a19d-6ef7ddb69374@postgrespro.ru) I used explicit pg_memory_barrier before
and pg_read_barrier after reading segP->maxMsgNum. (pg_memory_barrier
writes onto stack - process's private memory, and pg_read_barrier does
nothing on x86_64).

[1]: /messages/by-id/attachment/174633/v3-0001-sinvaladt.c-use-atomic-operations-on-maxMsgNum.patch
/messages/by-id/attachment/174633/v3-0001-sinvaladt.c-use-atomic-operations-on-maxMsgNum.patch
[2]: /messages/by-id/30aa0030-f694-44ef-a19d-6ef7ddb69374@postgrespro.ru
/messages/by-id/30aa0030-f694-44ef-a19d-6ef7ddb69374@postgrespro.ru

--
regards
Yura Sokolov aka funny-falcon

On Fri, Sep 11, 2026 at 05:43:22PM +0300, Yura Sokolov wrote:

Personally, I don't like current implementation of
pg_atomic_read_membarrier_u32 because it writes into shared variable.

I think your dislike of the membarrier implementation is misguided. The
write is important and helps reduce the cognitive load of reading the code.

A spinlock guarantees that whoever takes the lock sees everything the
previous holder did before releasing the lock. The membarrier functions
keep that guarantee because every access is a read-modify-write, i.e.,
whoever touches the variable second must read what the first one wrote.
Take the following example:

/* thread A */
x = 1;
z = pg_atomic_read_membarrier_u32(&y);

/* thread B */
pg_atomic_write_membarrier_u32(&y, 1);
x = 2;

Let's say thread A's read of "y" returns 0. That must mean that thread A
wrote "x" before thread B did, which is same as what you'd get with a
spinlock. If the read was just a plain load behind a barrier, we can't
know the order of the writes to "x" on non-TSO architectures.

That is why in [1] (thread [2]) I used explicit pg_memory_barrier before
and pg_read_barrier after reading segP->maxMsgNum. (pg_memory_barrier
writes onto stack - process's private memory, and pg_read_barrier does
nothing on x86_64).

My patch is intended to be a straightforward spinlock-to-atomics
conversion, so I'd like to keep the membarrier accessors for now. Further
optimizations should be handled in their own threads. Two that come to
mind are an x86-specific implementation of pg_atomic_read_membarrier_u32()
(since it _is_ a TSO architecture), and something like your patch for
sinvaladt.c, i.e., using explicit barriers for that code.

--
nathan