Overview
This is a small, targeted cleanup patch proposed by Nathan Bossart (a PostgreSQL committer) against the atomics subsystem header src/include/port/atomics/arch-x86.h. The thread (as presented, a single message) flags two distinct but related issues and proposes removing dead code as the remedy.
The Core Problem
PostgreSQL's atomics layer (src/include/port/atomics/) contains per-architecture shims that provide the low-level memory barrier and atomic CAS primitives used throughout the backend (LWLocks, procarray, buffer header spinlocks, etc.). On x86, these shims historically needed a pg_spin_delay() implementation — emitting a PAUSE instruction (via rep; nop on GCC/Clang or the _mm_pause() intrinsic on MSVC) — so that busy-wait loops inside spinlock and atomic retry paths yield pipeline resources on hyperthreaded cores.
Nathan identified two problems with the current code in arch-x86.h:
1. Wrong MSVC 64-bit macro
The file contains:
#elif defined(_MSC_VER) && defined(__x86_64__)
__x86_64__ is a GCC/Clang-defined macro. MSVC does not define it; MSVC defines _M_X64 (and _M_AMD64) for 64-bit x86 targets. This means the branch is effectively dead on MSVC: any MSVC x86_64 build silently falls through to a different branch than intended. Thomas Munro had previously noticed the same class of mistake elsewhere in the tree (cited via the CA+hUKGL8Hs-phHPugrWM=5dAkcT897rXyazYzLw-Szxnzgx-rA message). This is a latent portability bug, but because the code gated by it is unused (see below), no runtime misbehavior results — only the illusion of MSVC-specific support.
2. The code is dead anyway
The pg_spin_delay() definitions in this header have been unused since commit b64d92f1a5 (the original introduction of the atomics abstraction by Andres Freund, back in 2014). The actual pg_spin_delay() callers in the spinlock code pull their definition from src/include/storage/s_lock.h, not from the atomics headers. The atomics-layer copy was presumably added defensively during the initial atomics design — perhaps anticipating that atomic retry loops would want their own delay primitive independent of s_lock — but that path was never wired up. It has sat as an implementation artifact for over a decade.
Proposed Solution
Rather than fixing the __x86_64__ → _M_X64 typo, Nathan proposes simply deleting the dead pg_spin_delay() block from arch-x86.h. This is the minimal and correct fix: there is no reason to maintain a carefully ported MSVC variant of code that nothing calls.
Why deletion is the right call
- Maintenance burden: keeping the code means continuing to debate whether it should use
_mm_pause(),YieldProcessor(), or__builtin_ia32_pause(), and ensuring the compiler-feature detection stays correct across MSVC, GCC, Clang, ICC, and any cross-compilation permutations. - No functional loss: spinlock delay on x86 is already provided by
s_lock.h. Atomics retry loops in PostgreSQL are short and don't currently need (or use) an architectural PAUSE hint from this layer. - Reduces confusion: future readers (like Nathan himself while auditing 32-bit code paths) won't waste time reasoning about an apparently-broken MSVC branch.
Architectural Context
This cleanup fits a broader ongoing effort in recent cycles to prune MSVC-specific cruft and consolidate platform detection macros. With the MSVC build system having been removed in favor of Meson (commit 1301c80b2), a lot of MSVC-conditional code in the tree has become either simpler to reason about or exposed as having been quietly wrong. Nathan's discovery is a symptom of that: when the MSVC build was less commonly exercised, a defined(__x86_64__) guard that never fires could live for years undetected.
The citation of Thomas Munro's prior observation suggests there may be a small cluster of similar __x86_64__-under-MSVC mistakes elsewhere. This patch addresses one instance by deletion; others would need the macro corrected to _M_X64 if the gated code is actually live.
Tradeoffs and Risks
- Risk: essentially zero. Removing unreferenced preprocessor-gated definitions cannot regress runtime behavior.
- Tradeoff: if a future atomics-layer feature wanted an architecture-specific delay, it would need to reintroduce this — but doing so correctly (with
_M_X64) would be trivial, and having a working s_lock.h version to crib from mitigates any loss of institutional knowledge.
Who Carries Weight Here
- Nathan Bossart is a committer and has done extensive work on low-level backend infrastructure (including atomics, spinlocks, and 32-bit cleanup). His judgment on what's dead code in this area is authoritative.
- Thomas Munro (committer, referenced indirectly) has deep portability expertise and originally flagged the macro-name class of bug.
- Andres Freund authored the atomics subsystem (b64d92f1a5) and would be the natural reviewer to confirm the code was never intended to be wired up — though the single-message thread shows no response yet.
Expected Resolution
Given the triviality and safety of the patch, this is the kind of change that typically goes in with minimal discussion after one reviewer confirms the "unused since b64d92f1a5" claim by grep. The more interesting follow-up would be a broader audit for other defined(__x86_64__) && defined(_MSC_VER) occurrences in the tree.