Starting a new thread from [0]. On Wed, Feb 18, 2026 at 02:17:34AM +0200, Heikki Linnakangas wrote: > On 18/02/2026 01:11, Andres Freund wrote: >> I think the spinlock functions should also assert this. > > +1
While working on adding assertions that we are not in a signal handler to the spinlock functions, I figured I'd take the opportunity to convert the spin.h macros to static inline functions. This was previously discussed [1], but AFAICT there was debate over whether to remove S_LOCK and friends altogether (which doesn't seem to have happened). But IIUC nobody was horribly opposed to $subject, which I think makes sense to do as prerequisite work for the aforementioned assertions. However, as soon as I did this, I got a bunch of build failures because various parts of the code still use volatile qualifiers quite liberally. It looks like most of these (e.g., see code from commits 2487d872e0, 966fb05b58, 4bc15a8bfb, and 4db3744f1f) predate making spinlocks compiler barriers (commit 0709b7ee72) or were cargo-culted from code that predated it. So, IIUC, it's probably safe to remove these volatile qualifiers now. We could alternatively add volatile qualifiers to the new static inline function parameters, but that seems like it might just encourage continued unnecessary use. I also noticed that SpinLockFree() is unused (and apparently never was). There seems to have been agreement back in 2020 to remove it [2], but it still exists. I added a prerequisite patch for this, too. Thoughts? [0] https://postgr.es/m/2f25aa74-990d-4412-a032-c876dbcff667%40iki.fi [1] https://postgr.es/m/flat/20200617183354.pm3biu3zbmo2pktq%40alap3.anarazel.de#160ac60fddc7934777a0d20b9183d840 [2] https://postgr.es/m/flat/20200608225338.m5zho424w6lpwb2d%40alap3.anarazel.de -- nathan
>From 9917d563397ffac4b41af2e9d58dbf99efbf0924 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 18 Feb 2026 10:31:24 -0600 Subject: [PATCH v1 1/3] Remove unnecessary volatile qualifiers. --- src/backend/replication/syncrep.c | 19 ++++++++----------- src/backend/storage/ipc/procsignal.c | 4 ++-- src/backend/storage/lmgr/lock.c | 2 +- src/test/modules/test_shm_mq/setup.c | 4 ++-- src/test/modules/test_shm_mq/worker.c | 2 +- 5 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c index 7ea6001e9ad..045fd35786a 100644 --- a/src/backend/replication/syncrep.c +++ b/src/backend/replication/syncrep.c @@ -473,7 +473,6 @@ SyncRepInitConfig(void) void SyncRepReleaseWaiters(void) { - volatile WalSndCtlData *walsndctl = WalSndCtl; XLogRecPtr writePtr; XLogRecPtr flushPtr; XLogRecPtr applyPtr; @@ -548,19 +547,19 @@ SyncRepReleaseWaiters(void) * Set the lsn first so that when we wake backends they will release up to * this location. */ - if (walsndctl->lsn[SYNC_REP_WAIT_WRITE] < writePtr) + if (WalSndCtl->lsn[SYNC_REP_WAIT_WRITE] < writePtr) { - walsndctl->lsn[SYNC_REP_WAIT_WRITE] = writePtr; + WalSndCtl->lsn[SYNC_REP_WAIT_WRITE] = writePtr; numwrite = SyncRepWakeQueue(false, SYNC_REP_WAIT_WRITE); } - if (walsndctl->lsn[SYNC_REP_WAIT_FLUSH] < flushPtr) + if (WalSndCtl->lsn[SYNC_REP_WAIT_FLUSH] < flushPtr) { - walsndctl->lsn[SYNC_REP_WAIT_FLUSH] = flushPtr; + WalSndCtl->lsn[SYNC_REP_WAIT_FLUSH] = flushPtr; numflush = SyncRepWakeQueue(false, SYNC_REP_WAIT_FLUSH); } - if (walsndctl->lsn[SYNC_REP_WAIT_APPLY] < applyPtr) + if (WalSndCtl->lsn[SYNC_REP_WAIT_APPLY] < applyPtr) { - walsndctl->lsn[SYNC_REP_WAIT_APPLY] = applyPtr; + WalSndCtl->lsn[SYNC_REP_WAIT_APPLY] = applyPtr; numapply = SyncRepWakeQueue(false, SYNC_REP_WAIT_APPLY); } @@ -767,8 +766,7 @@ SyncRepGetCandidateStandbys(SyncRepStandbyData **standbys) n = 0; for (i = 0; i < max_wal_senders; i++) { - volatile WalSnd *walsnd; /* Use volatile pointer to prevent code - * rearrangement */ + WalSnd *walsnd; SyncRepStandbyData *stby; WalSndState state; /* not included in SyncRepStandbyData */ @@ -905,7 +903,6 @@ SyncRepGetStandbyPriority(void) static int SyncRepWakeQueue(bool all, int mode) { - volatile WalSndCtlData *walsndctl = WalSndCtl; int numprocs = 0; dlist_mutable_iter iter; @@ -920,7 +917,7 @@ SyncRepWakeQueue(bool all, int mode) /* * Assume the queue is ordered by LSN */ - if (!all && walsndctl->lsn[mode] < proc->waitLSN) + if (!all && WalSndCtl->lsn[mode] < proc->waitLSN) return numprocs; /* diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c index 5d33559926a..97791f979ef 100644 --- a/src/backend/storage/ipc/procsignal.c +++ b/src/backend/storage/ipc/procsignal.c @@ -283,7 +283,7 @@ CleanupProcSignalState(int status, Datum arg) int SendProcSignal(pid_t pid, ProcSignalReason reason, ProcNumber procNumber) { - volatile ProcSignalSlot *slot; + ProcSignalSlot *slot; if (procNumber != INVALID_PROC_NUMBER) { @@ -394,7 +394,7 @@ EmitProcSignalBarrier(ProcSignalBarrierType type) */ for (int i = NumProcSignalSlots - 1; i >= 0; i--) { - volatile ProcSignalSlot *slot = &ProcSignal->psh_slot[i]; + ProcSignalSlot *slot = &ProcSignal->psh_slot[i]; pid_t pid = pg_atomic_read_u32(&slot->pss_pid); if (pid != 0) diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index e1168ad3837..718de1b5e98 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -309,7 +309,7 @@ typedef struct uint32 count[FAST_PATH_STRONG_LOCK_HASH_PARTITIONS]; } FastPathStrongRelationLockData; -static volatile FastPathStrongRelationLockData *FastPathStrongRelationLocks; +static FastPathStrongRelationLockData *FastPathStrongRelationLocks; /* diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c index 579e5933d28..a1ec13d66b9 100644 --- a/src/test/modules/test_shm_mq/setup.c +++ b/src/test/modules/test_shm_mq/setup.c @@ -36,7 +36,7 @@ static worker_state *setup_background_workers(int nworkers, dsm_segment *seg); static void cleanup_background_workers(dsm_segment *seg, Datum arg); static void wait_for_workers_to_become_ready(worker_state *wstate, - volatile test_shm_mq_header *hdr); + test_shm_mq_header *hdr); static bool check_worker_status(worker_state *wstate); /* value cached, fetched from shared memory */ @@ -256,7 +256,7 @@ cleanup_background_workers(dsm_segment *seg, Datum arg) static void wait_for_workers_to_become_ready(worker_state *wstate, - volatile test_shm_mq_header *hdr) + test_shm_mq_header *hdr) { bool result = false; diff --git a/src/test/modules/test_shm_mq/worker.c b/src/test/modules/test_shm_mq/worker.c index 368e4f3f234..14972f7e7cd 100644 --- a/src/test/modules/test_shm_mq/worker.c +++ b/src/test/modules/test_shm_mq/worker.c @@ -50,7 +50,7 @@ test_shm_mq_main(Datum main_arg) shm_toc *toc; shm_mq_handle *inqh; shm_mq_handle *outqh; - volatile test_shm_mq_header *hdr; + test_shm_mq_header *hdr; int myworkernumber; PGPROC *registrant; -- 2.50.1 (Apple Git-155)
>From 5033d7108d4203a34fb7eb84f793494ded735803 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 18 Feb 2026 11:03:01 -0600 Subject: [PATCH v1 2/3] Remove SpinLockFree() and S_LOCK_FREE(). --- src/backend/storage/lmgr/s_lock.c | 24 ------------------------ src/include/storage/s_lock.h | 8 -------- src/include/storage/spin.h | 6 ------ 3 files changed, 38 deletions(-) diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c index 5b79556bc9c..6df568eccb3 100644 --- a/src/backend/storage/lmgr/s_lock.c +++ b/src/backend/storage/lmgr/s_lock.c @@ -262,12 +262,6 @@ main() return 1; } - if (!S_LOCK_FREE(&test_lock.lock)) - { - printf("S_LOCK_TEST: failed, lock not initialized\n"); - return 1; - } - S_LOCK(&test_lock.lock); if (test_lock.pad1 != 0x44 || test_lock.pad2 != 0x44) @@ -276,12 +270,6 @@ main() return 1; } - if (S_LOCK_FREE(&test_lock.lock)) - { - printf("S_LOCK_TEST: failed, lock not locked\n"); - return 1; - } - S_UNLOCK(&test_lock.lock); if (test_lock.pad1 != 0x44 || test_lock.pad2 != 0x44) @@ -290,12 +278,6 @@ main() return 1; } - if (!S_LOCK_FREE(&test_lock.lock)) - { - printf("S_LOCK_TEST: failed, lock not unlocked\n"); - return 1; - } - S_LOCK(&test_lock.lock); if (test_lock.pad1 != 0x44 || test_lock.pad2 != 0x44) @@ -304,12 +286,6 @@ main() return 1; } - if (S_LOCK_FREE(&test_lock.lock)) - { - printf("S_LOCK_TEST: failed, lock not re-locked\n"); - return 1; - } - printf("S_LOCK_TEST: this will print %d stars and then\n", NUM_DELAYS); printf(" exit with a 'stuck spinlock' message\n"); printf(" if S_LOCK() and TAS() are working.\n"); diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h index 3d9070e79d4..89d28ac40c9 100644 --- a/src/include/storage/s_lock.h +++ b/src/include/storage/s_lock.h @@ -21,10 +21,6 @@ * void S_UNLOCK(slock_t *lock) * Unlock a previously acquired lock. * - * bool S_LOCK_FREE(slock_t *lock) - * Tests if the lock is free. Returns true if free, false if locked. - * This does *not* change the state of the lock. - * * void SPIN_DELAY(void) * Delay operation to occur inside spinlock wait loop. * @@ -671,10 +667,6 @@ spin_delay(void) (TAS(lock) ? s_lock((lock), __FILE__, __LINE__, __func__) : 0) #endif /* S_LOCK */ -#if !defined(S_LOCK_FREE) -#define S_LOCK_FREE(lock) (*(lock) == 0) -#endif /* S_LOCK_FREE */ - #if !defined(S_UNLOCK) /* * Our default implementation of S_UNLOCK is essentially *(lock) = 0. This diff --git a/src/include/storage/spin.h b/src/include/storage/spin.h index 0f160d0e395..78c95ae538b 100644 --- a/src/include/storage/spin.h +++ b/src/include/storage/spin.h @@ -18,10 +18,6 @@ * void SpinLockRelease(volatile slock_t *lock) * Unlock a previously acquired lock. * - * bool SpinLockFree(slock_t *lock) - * Tests if the lock is free. Returns true if free, false if locked. - * This does *not* change the state of the lock. - * * Callers must beware that the macro argument may be evaluated multiple * times! * @@ -60,6 +56,4 @@ #define SpinLockRelease(lock) S_UNLOCK(lock) -#define SpinLockFree(lock) S_LOCK_FREE(lock) - #endif /* SPIN_H */ -- 2.50.1 (Apple Git-155)
>From e757c04f3da5729400d466ffa8a0bf91f8cf272f Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 18 Feb 2026 10:32:26 -0600 Subject: [PATCH v1 3/3] Convert SpinLock* macros to static inline functions. --- src/include/storage/spin.h | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/src/include/storage/spin.h b/src/include/storage/spin.h index 78c95ae538b..5b680ff5dd8 100644 --- a/src/include/storage/spin.h +++ b/src/include/storage/spin.h @@ -5,22 +5,19 @@ * * * The interface to spinlocks is defined by the typedef "slock_t" and - * these macros: + * these functions: * - * void SpinLockInit(volatile slock_t *lock) + * void SpinLockInit(slock_t *lock) * Initialize a spinlock (to the unlocked state). * - * void SpinLockAcquire(volatile slock_t *lock) + * void SpinLockAcquire(slock_t *lock) * Acquire a spinlock, waiting if necessary. * Time out and abort() if unable to acquire the lock in a * "reasonable" amount of time --- typically ~ 1 minute. * - * void SpinLockRelease(volatile slock_t *lock) + * void SpinLockRelease(slock_t *lock) * Unlock a previously acquired lock. * - * Callers must beware that the macro argument may be evaluated multiple - * times! - * * Load and store operations in calling code are guaranteed not to be * reordered with respect to these operations, because they include a * compiler barrier. (Before PostgreSQL 9.5, callers needed to use a @@ -29,9 +26,9 @@ * Keep in mind the coding rule that spinlocks must not be held for more * than a few instructions. In particular, we assume it is not possible * for a CHECK_FOR_INTERRUPTS() to occur while holding a spinlock, and so - * it is not necessary to do HOLD/RESUME_INTERRUPTS() in these macros. + * it is not necessary to do HOLD/RESUME_INTERRUPTS() in these functions. * - * These macros are implemented in terms of hardware-dependent macros + * These functions are implemented in terms of hardware-dependent macros * supplied by s_lock.h. There is not currently any extra functionality * added by this header, but there has been in the past and may someday * be again. @@ -49,11 +46,22 @@ #include "storage/s_lock.h" +static inline void +SpinLockInit(slock_t *lock) +{ + S_INIT_LOCK(lock); +} -#define SpinLockInit(lock) S_INIT_LOCK(lock) - -#define SpinLockAcquire(lock) S_LOCK(lock) +static inline void +SpinLockAcquire(slock_t *lock) +{ + S_LOCK(lock); +} -#define SpinLockRelease(lock) S_UNLOCK(lock) +static inline void +SpinLockRelease(slock_t *lock) +{ + S_UNLOCK(lock); +} #endif /* SPIN_H */ -- 2.50.1 (Apple Git-155)
