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