Cleanup of s_lock.h: API Contract Clarification for Spinlocks

Context and Motivation

src/include/storage/s_lock.h is one of the oldest and most platform-sensitive headers in the PostgreSQL tree. It defines the low-level spinlock primitives (S_LOCK, S_UNLOCK, S_INIT_LOCK, SPIN_DELAY, TAS, TAS_SPIN) that underpin LWLocks and virtually every other synchronization primitive in the backend. Because spinlocks must be implemented with platform-specific atomic instructions, the header historically contained large blocks of inline assembly per CPU/compiler combination, with fallback paths to out-of-line assembly files (tas.s) or to a C implementation in s_lock.c.

Over the last several release cycles the set of supported platforms and fallback mechanisms has been pruned aggressively:

• Commit 718aa43a4e removed the last default C implementation of tas() from s_lock.c.
• Commit 25f36066dd removed the last remaining tas.s files.

The net result is that the surviving code paths always satisfy TAS via an inline macro (typically built on GCC's __sync_lock_test_and_set intrinsic or hand-rolled inline asm), yet the header still advertises — in comments and via an extern int tas(volatile slock_t *lock) declaration — a fallback that no longer exists. This is dead code that also misleads porters.

The Cleanup Nathan Bossart Proposed

The initial patch set had two independent goals:

1. 0001: Remove the stale extern int tas(...) declaration and the unconditional #define TAS(lock) tas(lock) that followed it, since no implementation is ever linked in.
2. 0002: Remove HAS_TEST_AND_SET entirely. Nathan's observation was that HAS_TEST_AND_SET is defined in essentially every branch where TAS is defined and nowhere else — so it's a redundant alias for "is TAS defined?". Direct #ifdef TAS checks would be clearer.

Tom Lane pushed back on (2) but engaged constructively with (1), suggesting the declaration be replaced by an explicit #error "must provide a spinlock implementation" guard so that a misconfigured build fails loudly at preprocessing rather than at link time with a missing-symbol error. He also proposed consolidating the HAS_TEST_AND_SET definitions, which are currently scattered across every per-platform block, into a single location at the end of the header:

#if defined(TAS)
#define HAS_TEST_AND_SET
#else
#error "must provide a spinlock implementation"
#endif

The Deeper Issue Tom Surfaced: A Broken API Contract

After a round of exchanges about comment wording, Tom identified what is really the central architectural defect. The header comment currently states:

Usually, S_LOCK() is implemented in terms of even lower-level macros TAS() and TAS_SPIN()

Because every supported platform today happens to follow this layering, the codebase has drifted into treating TAS() as a mandatory porting interface. Tom's point is that this inverts the abstraction: the contract of s_lock.h should be "you must provide S_LOCK()", and TAS() is merely the conventional building block we use to construct it. A hypothetical platform that has a very good native mutex or a pthread_spin_lock-style primitive should be free to supply S_LOCK() directly without faking a TAS().

This reframing has concrete consequences for the cleanup:

• The #error should fire on the absence of S_LOCK, not the absence of TAS.
• s_lock.c's s_lock() helper (the contention/backoff routine called from the S_LOCK macro when the fast TAS fails) is only meaningful if TAS/TAS_SPIN exist. Tom's proposal: don't compile s_lock() at all when TAS is undefined — a platform supplying its own S_LOCK is responsible for its own slow path.
• HAS_TEST_AND_SET's semantics become genuinely ambiguous under this contract: should a platform providing S_LOCK but not TAS set it? Since the macro has no external consumers (Tom verified it's referenced nowhere outside s_lock.h), Tom reversed position and agreed with Nathan that it should just be removed.

Tradeoffs and Design Decisions

• Fail-fast vs. graceful degradation: Replacing a dangling extern with #error trades a linker error (which might be cryptic or deferred) for a compile-time diagnostic. Given that spinlocks are non-optional for a working backend, fail-fast is strictly better.
• Preserving HAS_TEST_AND_SET for ABI/third-party compatibility: Tom initially worried that removing it could break external code. He relented once he confirmed it has no in-tree consumers; the risk to out-of-tree code is judged low because HAS_TEST_AND_SET is an internal porting marker, not a public extension API.
• Header comment obsolescence: The closing remark about "Equivalent OS-supplied mutex routines could be used too" dates from an era when POSIX threads were a novelty. Tom suggests rewording to "Equivalent compiler intrinsics are another popular option," which better reflects that GCC/Clang __sync_* and __atomic_* intrinsics are now the dominant implementation vehicle on modern platforms.
• Porter ergonomics: Nathan notes that a new-platform porter today primarily needs to define slock_t, TAS, S_UNLOCK, SPIN_DELAY, and optionally TAS_SPIN — not the full menagerie the comment lists. The comment's current framing gives an inflated sense of the porting burden.

Secondary Cleanup Surface

Kirill Reshke observed that tas.s still appears in meson.build, .gitignore, and src/backend/Makefile despite having no remaining producers. These references are vestigial and should be removed as part of the same cleanup for consistency with commit 25f36066dd.

Weight of Opinions

• Tom Lane is the decisive voice here: as a core committer with decades of history on this particular file, his framing of the API contract issue is what shifts the patch from a superficial cleanup to a meaningful clarification of a porting interface.
• Nathan Bossart (committer) is driving the work and owns the rewrite. His back-and-forth with Tom is notable for his willingness to reconsider the comment rewrite when he realized how intertwined it was with the API-contract question.
• Tristan Partin and Kirill Reshke contribute confirmatory review and spot additional stale references, respectively, but the technical direction is set by the Nathan/Tom exchange.

Expected Outcome

The converging design is:

1. Remove the bogus extern int tas(...) declaration.
2. Consolidate HAS_TEST_AND_SET handling, then likely remove it entirely since it has no consumers.
3. Re-anchor the mandatory-interface check on S_LOCK rather than TAS, with #error on absence.
4. Avoid compiling s_lock.c's s_lock() helper when TAS is undefined.
5. Rewrite the header comment to reflect that compiler intrinsics are now the mainstream implementation, and to give porters a realistic picture of which macros they actually need to supply.
6. Scrub remaining tas.s references from meson.build, .gitignore, and the legacy Makefile.