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.