Conversation
📜 Change history & discussion (Agora / pg.ddx.io)I have enough confirmed material. The C11 baseline is referenced in Munro's opening message ("since we pulled the trigger on C11"). I won't fabricate a specific commit hash for that. Final report. 🧵 Related discussion
🔗 Related commits / prior art
📋 Commitfest
🧭 Context for reviewers
Generated by pg-history via the Agora MCP server (pg.ddx.io). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 OCR found 45 issue(s).
- 25 inline, 20 in summary (inline capped at 25)
⚠️ 4 warning(s) during review
📄 src/backend/storage/lmgr/README
There is no pg_atomic_unlocked_read_* function in the API. Only pg_atomic_unlocked_write_u32/u64 exist (see atomics.h and stdatomic_impl.h); no pg_atomic_unlocked_read_* is defined. The {read,write} brace expansion references a non-existent primitive and will mislead readers. Change to pg_atomic_unlocked_write_u32/u64 (or drop "read,").
💡 Suggested change
Before:
contract. The pg_atomic_unlocked_{read,write}_*
variants also use relaxed ordering, consistent with their "no guarantees"
contract.
After:
contract. The pg_atomic_unlocked_write_*
variants also use relaxed ordering, consistent with their "no guarantees"
contract.
📄 src/include/port/atomics.h (L46-L54)
This is a floating essay-style comment: it sits between #define INSIDE_ATOMICS_H and #include <limits.h> but documents nothing that immediately follows it (no volatile-qualified signature appears here). PostgreSQL comment discipline is that a comment explains the code it sits above. A cross-cutting design note like this belongs at the definition of the API/_impl signatures (or a single design paragraph in the file header), not wedged before an unrelated #include. Also, the prose ('technically redundant', 'for compatibility with existing callers', discussion of what GCC/Clang do) reads like mailing-list rationale rather than in-tree documentation -- trim to what a future reader needs. (moderate confidence)
📄 src/include/port/spin_delay.h (L67-L67)
__arm64__ is not a standard predefined macro on aarch64 toolchains; GCC and Clang define __aarch64__. __arm64__ was only ever emitted by some older Apple/LLVM configurations. Combined with __aarch64__ it is effectively dead here and just adds noise. Drop it unless there is a concrete toolchain in the support matrix that defines only __arm64__.
💡 Suggested change
Before:
#elif defined(__aarch64__) || defined(__arm64__)
After:
#elif defined(__aarch64__)
📄 src/test/benchmarks/atomics_analyzer.py (L43-L45)
Division by zero on legitimate benchmark data. traditional_ms can be exactly 0.0: atomics_workload.py records elapsed = 0.0 when a measurement query fails, and that value becomes traditional_ms in results.json. This unguarded division then raises ZeroDivisionError and aborts loading the entire report. Also, all fields are accessed via direct subscript (r["traditional_ms"], etc.), so any results file missing a key raises a bare KeyError with no context. Guard the divisor and/or use .get() with validation. (high confidence)
💡 Suggested change
Before:
diff_pct = (
(r["traditional_ms"] - r["stdatomic_ms"]) / r["traditional_ms"] * 100
)
After:
traditional_ms = r["traditional_ms"]
if traditional_ms == 0:
diff_pct = 0.0
else:
diff_pct = (
(traditional_ms - r["stdatomic_ms"]) / traditional_ms * 100
)
📄 src/test/benchmarks/atomics_analyzer.py (L154-L154)
Empty results file crashes with an unhandled KeyError. When self.comparisons is empty, analyze_overall() returns {} (see the if not self.comparisons: return {} guard), and this line then does overall['total_scenarios'] on that empty dict, raising KeyError. run_full_suite produces an empty results list (and {} summary) whenever every scenario fails, so this path is reachable and yields an unhelpful traceback instead of a graceful message. Add an early check for empty comparisons in generate_report. (high confidence)
💡 Suggested change
Before:
lines.append(f"Total scenarios: {overall['total_scenarios']}")
After:
if not overall:
lines.append("No results to analyze.")
return "\n".join(lines)
lines.append(f"Total scenarios: {overall['total_scenarios']}")
📄 src/test/benchmarks/atomics_analyzer.py (L156-L156)
Non-ASCII characters (±) in source violate PostgreSQL's ASCII-only rule for source and diffs. Replace with +/-. This appears on both this line and the within_noise line below. (high confidence)
💡 Suggested change
Before:
lines.append(f"Mean speedup: {overall['mean_speedup']:.3f}x ± {overall['stdev_speedup']:.3f}")
After:
lines.append(f"Mean speedup: {overall['mean_speedup']:.3f}x +/- {overall['stdev_speedup']:.3f}")
📄 src/test/benchmarks/atomics_analyzer.py (L164-L164)
Non-ASCII ± character violates the ASCII-only source rule; use +/-. (high confidence)
💡 Suggested change
Before:
lines.append(f"Within measurement noise: {overall['within_noise']} (±{2.0:.1f}%)")
After:
lines.append(f"Within measurement noise: {overall['within_noise']} (+/-{2.0:.1f}%)")
📄 src/test/benchmarks/atomics_benchmark.py (L341-L341)
Division-by-zero, high confidence. std_result.elapsed_seconds can legitimately be 0.0: in atomics_workload.py _run_single, a query that raises during measurement sets elapsed = 0.0 and appends it, and the median can therefore be 0.0 (also possible for sub-timer-resolution queries). This raises ZeroDivisionError, which the broad except Exception in run_full_suite swallows, silently dropping the entire scenario and corrupting the summary. Guard against a zero denominator (e.g. skip the comparison or record a sentinel) before dividing.
💡 Suggested change
Before:
speedup = trad_result.elapsed_seconds / std_result.elapsed_seconds
After:
if std_result.elapsed_seconds == 0.0:
logger.warning(
"Skipping comparison for pattern %s: stdatomic elapsed time is 0",
pattern,
)
continue
speedup = trad_result.elapsed_seconds / std_result.elapsed_seconds
📄 src/test/benchmarks/atomics_benchmark.py (L500-L500)
Incorrect median for even-length input, moderate confidence. sorted(speedups)[len(speedups) // 2] returns the upper-middle element for even N rather than the mean of the two central values. statistics.median (already imported in atomics_analyzer.py) computes this correctly and is DRY.
💡 Suggested change
Before:
"median_speedup": sorted(speedups)[len(speedups) // 2],
After:
"median_speedup": statistics.median(speedups),
📄 src/test/benchmarks/atomics_benchmark.py (L506-L506)
neutral_count is effectively dead, low confidence. Measured floating-point timing ratios will essentially never be exactly 1.0, so s == 1.0 is almost always false. If a neutral bucket is intended, compare against a tolerance band (e.g. math.isclose or 0.99 <= s <= 1.01); otherwise drop the field.
📄 src/test/benchmarks/atomics_benchmark.py (L254-L257)
Unchecked pool dereference, moderate confidence. execute/fetch/fetchrow use self.pool.acquire() without verifying self.pool is not None. If a caller invokes these before start() (pool not yet created) or after stop() (which sets self.pool = None), this raises an opaque AttributeError instead of a clear error. A guard or explicit assertion would give a better failure mode.
📄 src/test/benchmarks/atomics_benchmark.py (L436-L438)
pg_total_relation_size('{table_name}') interpolates the table name into a SQL string literal via f-string. Table names are config-derived constants today (safe), but this is a footgun: any name containing an apostrophe corrupts the query. Prefer passing the identifier as a parameter (pg_total_relation_size($1) with table_name as a bind value) or quoting it via a regclass cast.
📄 src/test/benchmarks/atomics_config.py (L260-L260)
The default data_dir values hardcode POSIX-only absolute paths under /tmp. This is non-portable (Windows/MSVC has no /tmp) and on shared/CI hosts multiple runs collide on the same fixed path. Prefer a platform-neutral default derived from tempfile.gettempdir() (or leave data_dir=None and let the caller/CLI supply it, since the CLI already provides these defaults). Confidence: moderate.
📄 src/test/benchmarks/atomics_workload.py (L236-L239)
A failed measurement is recorded as elapsed = 0.0 and included in timings, which then feeds the median. A partially-failing pattern reports an artificially low median latency, and a fully-failing query yields a median of 0.0. In atomics_benchmark.py the caller computes speedup = trad_result.elapsed_seconds / std_result.elapsed_seconds, so a 0.0 stdatomic median raises ZeroDivisionError; a 0.0 traditional median silently reports a speedup of 0. Failed iterations should be excluded from timing statistics (or the query counted as failed), not injected as zero-time samples.
Confidence: high.
📄 src/test/benchmarks/atomics_workload.py (L241-L243)
timings can be empty (e.g. measure_iterations <= 0, which is user-controllable via --iterations with no validation, or if every iteration is somehow skipped), making timings[len(timings) // 2] raise IndexError. Guard against an empty timing list before indexing.
Confidence: high.
💡 Suggested change
Before:
# Use median timing
timings.sort()
median_time = timings[len(timings) // 2]
After:
# Use median timing
timings.sort()
median_time = timings[len(timings) // 2] if timings else 0.0
📄 src/test/benchmarks/atomics_workload.py (L140-L140)
nextval('seq_concurrent_insert') references a sequence that is never created anywhere in the benchmark suite (no CREATE SEQUENCE exists for it, and _create_table in atomics_benchmark.py does not create one). Every CONCURRENT_INSERT measurement will fail with 'relation does not exist' and, combined with the 0.0-on-failure handling above, be silently recorded as zero-time, polluting results. Create the sequence during table setup or generate ids differently.
Confidence: high.
📄 src/test/benchmarks/atomics_workload.py (L291-L291)
implementation is passed through as result.implementation, which run_workload sets to "" ('Set by caller'). The caller in atomics_benchmark.py (_run_on_instance/run_benchmark) never assigns workload_result.implementation, so every QueryResult carries an empty implementation label. This is the key attribution field for the traditional-vs-stdatomic comparison; leaving it empty makes per-result records unattributable. Either set it here from a parameter or ensure the caller populates it.
Confidence: high.
📄 src/test/benchmarks/atomics_cli.py (L209-L212)
--rows is silently ignored when --full-matrix is also passed. build_config sets config.row_counts = args.rows, but AtomicsBenchmarkConfig.get_row_counts() returns the hardcoded ROW_COUNTS list and ignores row_counts entirely whenever full_matrix is True (see atomics_config.py:272-275). So --rows 100000 --full-matrix discards the user's explicit --rows without any warning. This is a footgun (POLA violation). Either make full_matrix respect an explicit --rows, or reject/warn on the conflicting combination.
📄 src/test/benchmarks/atomics_cli.py (L4-L5)
The documented invocation python -m src.test.benchmarks.atomics_cli is inconsistent with the README (python -m atomics_cli) and does not work as written: this module uses relative imports (from .atomics_config import ...), yet there are no __init__.py files under src/test/benchmarks (nor is src a package). Running it as atomics_cli fails with 'attempted relative import with no known parent package'. Pick one supported invocation and make it actually runnable (add package markers or switch to absolute imports), and keep the docstring and README in sync.
📄 src/test/regress/pg_regress.c (L1246-L1246)
This execl -> execlp change is a functional no-op and should be dropped. shellprog is always SHELLPROG, which resolves to an absolute path in both build systems ($(SHELL) in GNUmakefile, "/bin/sh" in meson.build). execlp only performs a PATH search when the program name contains no slash; an absolute path always contains one, so execlp behaves identically to execl here. The diff therefore adds no behavior and violates the minimal-diff rule (touching code not required by the change). Furthermore, if shellprog could ever become a bare name, switching to a PATH search would be a footgun: the test harness would run whatever sh appears first on $PATH rather than the shell chosen at configure time, which is both non-deterministic and a security concern. Revert this line. (high confidence)
💡 Suggested change
Before:
execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
After:
execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
| source_up | ||
| watch_file flake.nix | ||
| use flake | ||
| use project_steering postgresql | ||
| use project_mcp postgresq | ||
| use aws |
There was a problem hiding this comment.
This .envrc is a personal direnv/Nix developer environment file and must not be part of a PostgreSQL patch. It references private tooling (project_steering, project_mcp, use aws) and machine-local configuration that is meaningless to the community and does not belong in the tree. Committing it violates the minimal-diff discipline (it is entirely unrelated to the stated change) and would also add a repo-wide dotfile with no documentation or build-system integration. Note also the typo use project_mcp postgresq (missing trailing l). Remove this file from the patch and keep it in your local (gitignored) environment instead.
| # Local development ignores (not tracked in .gitignore) | ||
| # To enable: git config core.excludesFile .local-gitignore | ||
| .local-gitignore |
There was a problem hiding this comment.
This entire file is a personal developer workspace artifact and must not be committed. PostgreSQL patches destined for pgsql-hackers must contain only the minimal changes needed for the stated purpose. A .local-gitignore intended for local use (as its own comment says, enabled via git config core.excludesFile) is by definition not tracked, and it references personal paths (build/, install-asan/, .direnv/, NOTES) and even a stray mailing-list Message-Id filename (CA+hUKGKFvu3zyvv3aaj5hHs9VtWcjFAmisOwOc7aOZNc5AF3NA@mail.gmail.com). Committing local ignore rules pollutes the tree and will be rejected. Drop this file from the patch entirely; if any of these ignore rules are genuinely needed for the project they belong in the appropriate tracked .gitignore.
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This .clangd file is personal editor/LSP configuration, not part of the change's stated purpose (the atomics/spin work in the rest of the diff). Committing personal tooling config into the tree is a top rejection reason on pgsql-hackers: it is unrelated churn, bloats the patch, and causes needless merge conflicts. The author's own .local-gitignore (which lists build/, .direnv/, .cache/, NOTES) and .github/docs/pristine-master-policy.md confirm these dev-setup files (.clangd, .envrc, .gdbinit, pg-aliases.sh, flake.nix, .vscode/) are meant for a separate personal branch, not master. Keep the patch minimal: drop this file from the change (add it to a local excludesFile instead).
| @@ -0,0 +1,156 @@ | |||
| # HOT Indexed Updates — GDB breakpoints for code review | |||
There was a problem hiding this comment.
This entire file is a personal debugging artifact and should not be committed. Beyond violating the minimal-diff discipline (a .gdbinit has no build/test/doc value and is unrelated to the functional change), it is a footgun: GDB auto-loads .gdbinit from the current working directory, so anyone running gdb from the repo root would silently execute these breakpoint commands. Drop it from the patch and keep it in a personal/global gitignore instead.
| @@ -0,0 +1,156 @@ | |||
| # HOT Indexed Updates — GDB breakpoints for code review | |||
There was a problem hiding this comment.
ASCII-only violation: this line (and the section headers on lines 15, 38, 52, 81, 98, 106, 117, 140, 148) use a non-ASCII em-dash (U+2014). PostgreSQL source and diffs must be ASCII only. Use a plain hyphen - instead.
| # HOT Indexed Updates — GDB breakpoints for code review | |
| +# HOT Indexed Updates - GDB breakpoints for code review |
| #include <atomic> | ||
|
|
||
| /* Map to C++ atomic types */ | ||
| #define pg_atomic(T) std::atomic<T> |
There was a problem hiding this comment.
The C++ path maps pg_atomic(T) to std::atomic<T>, but all the _impl signatures take volatile pg_atomic_* * and call the C11 generic macros (atomic_load_explicit, atomic_store_explicit, atomic_fetch_*_explicit, atomic_init) on those volatile-qualified atomics. In C++ pre-23, these C11 macros are provided via <atomic> mapping and the interaction of volatile std::atomic<T> with them is not guaranteed to compile cleanly across the supported compiler matrix (MSVC-as-C++, clang, gcc). Since this is the sole atomics implementation, a compile failure here blocks the entire build on a supported platform. This needs explicit verification/testing on the MSVC C++ and clang/gcc C++ builds, not just an assurance comment in atomics.h. Confidence: moderate.
| static inline uint32 | ||
| pg_atomic_add_fetch_u32_impl(volatile pg_atomic_uint32 *ptr, int32 add_) | ||
| { | ||
| return atomic_fetch_add_explicit(ptr, add_, memory_order_seq_cst) + add_; |
There was a problem hiding this comment.
pg_atomic_add_fetch_u32_impl passes int32 add_ into atomic_fetch_add_explicit on a _Atomic(uint32), then adds add_ (int32) to the returned uint32. The addition mixes signed/unsigned and relies on well-defined unsigned wraparound to produce the correct new value for negative add_. This is correct for two's-complement wraparound but subtle; it matches the public pg_atomic_add_fetch_u32(..., int32 add_) signature (atomics.h:368) so behavior is consistent, but worth a brief comment noting the reliance on unsigned wraparound. Confidence: moderate.
| #if !(defined(_MSC_VER) && !defined(__cplusplus)) | ||
| StaticAssertDecl(ATOMIC_CHAR_LOCK_FREE == 2, "8-bit atomics are not lock-free"); |
There was a problem hiding this comment.
Skipping the lock-free static assertion for MSVC-as-C leaves a supported platform (PostgreSQL's Windows backend is built as C) with neither a compile-time nor a run-time guarantee that atomics are truly lock-free. The comment justifies this by asserting MSVC's <stdatomic.h> reports 1 spuriously, but there is no compensating atomic_is_lock_free() run-time check anywhere in the tree. If the assumption is ever wrong for any width, non-lock-free atomics fall back to process-local (libatomic-style) locks and silently corrupt cross-process shared memory -- precisely the hazard this assertion exists to prevent. Add an Assert(atomic_is_lock_free(...)) run-time guard for the MSVC-as-C path (or otherwise substantiate the claim) rather than leaving it unchecked. Confidence: moderate.
| * A cleaner long-term fix is to leave this primitive relaxed (matching the | ||
| * documented contract) and give the specific hash-join consumer the ordering | ||
| * it needs -- e.g. an acquire read at the call site or | ||
| * dsa_pointer_atomic_read_membarrier(). That is a change to core executor / | ||
| * dsa code and is intentionally out of scope for the atomics port; it is | ||
| * noted as follow-up work rather than folded in here. |
There was a problem hiding this comment.
This header comment editorializes and carries follow-up/aspirational narrative ('A cleaner long-term fix ...', 'noted as follow-up work rather than folded in here', plus run-report anecdotes like '1 run in 4 on real rv64 hardware'). PostgreSQL header comments should concisely explain the current behavior and why, not carry TODO-style future-work notes or benchmarking anecdotes. Trim to the essential rationale for the seq_cst choice; if follow-up work is intended, it belongs in the commit message / -hackers thread, not the shipped header. Confidence: high.
| /* | ||
| * First a set of architecture specific files is included. | ||
| * | ||
| * These files can provide the full set of atomics or can do pretty much | ||
| * nothing if all the compilers commonly used on these platforms provide | ||
| * usable generics. | ||
| * | ||
| * Don't add an inline assembly of the actual atomic operations if all the | ||
| * common implementations of your platform provide intrinsics. Intrinsics are | ||
| * much easier to understand and potentially support more architectures. | ||
| * | ||
| * It will often make sense to define memory barrier semantics here, since | ||
| * e.g. generic compiler intrinsics for x86 memory barriers can't know that | ||
| * postgres doesn't need x86 read/write barriers do anything more than a | ||
| * compiler barrier. | ||
| * | ||
| * With the stdatomic.h implementation there is no longer any restriction on | ||
| * including atomics.h from frontend code, since stdatomic.h is a standard | ||
| * header with no backend-only dependencies. | ||
| */ | ||
| #if defined(__arm__) || defined(__aarch64__) | ||
| #include "port/atomics/arch-arm.h" | ||
| #elif defined(__i386__) || defined(__x86_64__) | ||
| #include "port/atomics/arch-x86.h" | ||
| #elif defined(__powerpc__) || defined(__powerpc64__) | ||
| #include "port/atomics/arch-ppc.h" | ||
| #endif | ||
| #define INSIDE_ATOMICS_H |
There was a problem hiding this comment.
This hunk removes the #ifdef FRONTEND guard and adds a comment declaring that including atomics.h from frontend code is now allowed. However, no frontend translation unit includes port/atomics.h -- every current includer is backend code (src/backend/...), backend-only headers (lwlock.h, buf_internals.h, spin.h, etc.), or the backend-loaded regress module. This is speculative scaffolding (YAGNI): it enables and documents a capability that has no caller, and it is unrelated to the stated purpose of porting atomics onto C11 stdatomic.h. Dropping a long-standing safety guard with no consumer is the kind of unrelated change that draws rejection on -hackers. Recommend keeping the #ifdef FRONTEND guard until a frontend consumer actually exists, and removing the accompanying comment. (moderate confidence)
Keeps gburd/postgres rebased hourly on postgres/postgres master with only .github/ changes on top (sync-upstream automatic + manual). Drops the bespoke Windows dependency-builder workflow: Windows is already built and tested in CI by upstream's pg-ci.yml (Visual Studio + MinGW meson jobs), so a separate dependency prebuild that nothing consumed was redundant.
The Open Code Review system: ocr-review and ocr-model-check workflows plus .github/ocr config (LiteLLM->Bedrock Claude Opus 4.8, rule.json, context.md, pg-history.py).
github-actions
Bot
force-pushed
the
master
branch
from
0b3e646 to
3ed29c4
Compare
There was a problem hiding this comment.
🔍 OCR found 75 issue(s).
- 25 inline, 50 in summary (inline capped at 25)
📄 .local-gitignore (L1-L3)
This entire file is a personal/local development artifact and must not be part of a patch destined for pgsql-hackers. A .local-gitignore documenting the author's private core.excludesFile workflow, local build directories (build-valgrind/, build-asan/, install/), direnv (.envrc/.direnv/), and — most tellingly — a Gmail Message-Id filename (CA+hUKGKFvu3zyvv3aaj5hHs9VtWcjFAmisOwOc7aOZNc5AF3NA@mail.gmail.com) and a NOTES file, all belong in the developer's private environment, not in the tree. Per the minimal-diff discipline, unrelated files like this are a top rejection reason on -hackers. Drop this file from the patch; if such ignores are truly needed they belong in the appropriate committed .gitignore after discussion. (high confidence)
📄 .github/workflows/sync-upstream.yml (L109-L114)
This step is continue-on-error: true, so the job never fails on this step; all downstream steps are gated solely on steps.merge.outputs.merge_status. But not every exit path writes that output. If git merge --ff-only (line 111), git rebase --abort (line 134), or the shell interpolation of an empty commits_* output aborts the script before a merge_status=... line runs, the output is left empty. Then both Push to origin (if == 'success') and Create issue on failure (if == 'conflict') evaluate false: the failure is silently swallowed with no push and no issue created. Ensure every terminal path writes a deterministic merge_status (e.g. default it to conflict up front, or trap ERR to record status), rather than relying on the happy-path echo lines being reached.
Moderate confidence.
📄 .github/workflows/sync-upstream.yml (L142-L142)
git checkout master then git push origin master --force-with-lease operates on the local master created at checkout, while all divergence math uses origin/master/upstream/master. --force-with-lease (no explicit expected-ref) compares against the remote-tracking ref that the runner fetched at job start. Because this job runs unattended every hour and can overlap with the sibling sync-upstream-manual.yml, another run pushing in between will update the real origin/master while this runner's cached remote-tracking ref is stale, so the lease check can still pass and overwrite the just-pushed commits. Pair this with the missing concurrency group flagged above; alone, --force-with-lease here does not protect the default branch from a concurrent sync run.
Moderate confidence.
📄 configure (L17754-L17755)
The 64-bit CAS probe is compile-only (ac_fn_c_try_compile), and this change adds no libatomic linkage anywhere in the build. On platforms where atomic_compare_exchange_strong on a 64-bit type is not lock-free and lowers to a libatomic call (e.g. 32-bit ARM, some RISC-V/PPC configurations), this probe compiles cleanly but the backend then fails to link against __atomic_compare_exchange_8. Because stdatomic.h is now the sole atomics implementation with no fallback, a compile-only check is insufficient to guarantee the build works. Use a link-time check (equivalent of AC_LINK_IFELSE) so the probe fails on the same platforms the real code would, and/or detect and add -latomic when required. Note: the actual fix belongs in configure.ac (this is generated output); the meson probe has the identical gap.
📄 pg-aliases.sh (L1-L1)
Scope / patch-hygiene violation: this entire file is a personal developer-environment script (658 lines of aliases and functions) placed at the repository root. It is unrelated to the substantive change in this PR (the lmgr/atomics/spinlock work) and would be rejected outright on pgsql-hackers as unrelated churn. The sibling .local-gitignore even lists build/, install/, test-db/, results/ etc. as "Local development ignores (not tracked in .gitignore)", and the added .gdbinit is tied to a completely different "HOT Indexed Updates patch series" -- strong evidence these files were committed by mistake and should be excluded (e.g. via a local core.excludesFile) rather than added to the tree. Recommend dropping pg-aliases.sh (and the sibling .clangd/.gdbinit/.envrc) from the patch to keep it minimal and atomic.
📄 pg-aliases.sh (L79-L79)
Footgun: destructive deletion with no guard against an empty/unset variable. If $PG_BUILD_DIR or $PG_INSTALL_DIR is empty (e.g. the script is sourced without the surrounding env being configured), this expands to trash "" "" / rm -rf "" "", and any relative fallback can operate on the wrong path. Add an explicit non-empty check before deleting, e.g. [ -n "$PG_BUILD_DIR" ] && [ -n "$PG_INSTALL_DIR" ] || { echo "env not set" >&2; return 1; }.
📄 pg-aliases.sh (L82-L82)
Footgun: pg-init unconditionally deletes $PG_DATA_DIR. If PG_DATA_DIR is unset/empty this becomes rm -rf "" (harmless in bash but the trash "" path and any downstream initdb "" behaviour is surprising), and if it accidentally points at a real directory it is wiped with no confirmation. Guard against an empty value before deleting.
📄 pg-aliases.sh (L16-L16)
Destructive branch in pg_clean_for_compiler: on a false-negative compiler comparison (the grep -o regex assumes a specific /…/bin/[gc]cc|/…/bin/clang path layout and otherwise yields unknown) the else-branch deletes the build dir via trash/rm -rf. Also basename $CC is unquoted and breaks if $CC contains spaces. Quote "$CC" and consider making the destructive rebuild opt-in rather than heuristic-driven.
📄 pg-aliases.sh (L7-L7)
local current_compiler="$(basename $CC)" -- $CC is unquoted inside the command substitution, so a compiler path containing spaces will word-split and produce a wrong result (which in turn feeds the destructive compiler-change branch above). Use basename "$CC".
📄 pg-aliases.sh (L619-L620)
find ... -mtime +$days leaves $days unquoted; if the caller passes an empty or non-numeric argument the find predicate silently misbehaves (or matches unexpectedly), and combined with -delete this can remove files you did not intend. Quote it (-mtime "+$days") and validate that $days is a non-negative integer.
📄 pg-aliases.sh (L422-L425)
Dead references: pg-flame-generate and pg-bench-run are used by these aliases/functions but are not defined in this file or anywhere else in the repository. Invoking pg-flame, pg-flame-30, pg-bench, pg-bench-quick, etc. will fail with 'command not found'. Either define these helpers or remove the aliases that depend on them (YAGNI).
📄 pg-aliases.sh (L98-L98)
Security footgun: gdb -x .gdbinit auto-sources a .gdbinit from the current working directory. Running these aliases inside an untrusted checkout can execute attacker-controlled gdb commands. Also $GDBINIT may be unset (the file does not define it), making -x "$GDBINIT" fail. Prefer an absolute, trusted path and avoid loading .gdbinit from CWD.
📄 src/backend/storage/lmgr/README (L61-L66)
This documents a use_stdatomic build option that does not exist. There is no use_stdatomic entry in meson_options.txt, no get_option('use_stdatomic') call anywhere, and nothing in configure.ac. The Meson build (meson.build lines ~732-742) unconditionally probes for stdatomic.h and hard-errors if it is missing; there is no yes/auto/no option to accept or reject. Running meson setup build -Duse_stdatomic=auto (or =yes) on this tree would fail with an "unknown option" error, so the documented commands are wrong (POLA violation / doc-code drift). Either remove this paragraph and the sample commands, or restore a real option in the build files. Note the accompanying src/test/benchmarks/README.md and atomics_cli.py still reference -Duse_stdatomic=no/=yes, which is separately stale.
📄 src/backend/storage/lmgr/s_lock.c (L94-L96)
The comment calls this a "relaxed pre-check", but pg_atomic_read_u32() in the new stdatomic implementation uses memory_order_seq_cst (see pg_atomic_read_u32_impl in stdatomic_impl.h), not a relaxed load. The stated purpose of a test-and-test-and-set pre-check is to avoid cache-line ownership transfer / bus traffic with a cheap read. PG_SPIN_TRY_RELAXED is enabled for x86/x86_64 and PPC. On x86 (TSO) a seq_cst load is a plain mov, so the intent holds; but on PPC a seq_cst load emits sync/isync fences, so this "relaxed" pre-check actually adds barrier instructions on every contended spin iteration -- the opposite of the intended optimization. Either use a genuinely relaxed load (e.g. pg_atomic_unlocked_test via a relaxed-load primitive) or fix the comment and reconsider enabling this path on PPC.
📄 src/include/port/atomics.h (L44-L44)
This removes the long-standing #ifdef FRONTEND / #error guard and asserts atomics.h is now frontend-safe. That claim is not fully substantiated by the change and is a real portability/API-scope hazard:
- The safety depends entirely on stdatomic_impl.h having no backend-only dependencies. It uses
StaticAssertDeclanduintNN(both frontend-safe via c.h), so the include itself is likely OK, but the guarantee now rests on discipline rather than the compile-time guard that previously enforced it. Any future backend-only construct pulled into the atomics stack will silently break frontend builds instead of erroring clearly here. - This is a deliberate expansion of the public API surface (atomics were backend-only by design). Such a scope change belongs in its own discussed patch, not folded into an implementation swap.
Recommend either keeping backend-only scope (retain the guard) unless frontend atomics use is actually needed and reviewed, or, if intentional, justify it explicitly and confirm frontend links (e.g. the spinlock fallback path in atomics.c is backend-only). Confidence: moderate.
📄 src/include/port/atomics.h (L59-L62)
Deleting the #error checks for PG_HAVE_ATOMIC_U32_SUPPORT, pg_compiler_barrier_impl, and pg_memory_barrier_impl removes the compile-time safety net that guaranteed a platform provides working 32-bit atomics and barriers. If a future compiler/target fails to define one of these _impl primitives, the failure will now surface as obscure errors deep in the public API (or worse, a missing symbol at link time) rather than a clear, early diagnostic. The entire backend's locking/lockless correctness depends on these primitives; keeping an explicit assertion is cheap insurance. Consider retaining equivalent #if !defined(...) #error checks after including stdatomic_impl.h. Confidence: moderate.
📄 src/include/port/atomics.h (L51-L53)
This rationale comment claims only GCC and Clang handle volatile _Atomic correctly and omits MSVC, which is a hard portability target for PostgreSQL. Two concrete concerns the comment does not address:
- MSVC (both as C and C++): the public API here passes
volatile pg_atomic_uint32 *into the_impllayer, i.e.volatile _Atomic/volatile std::atomic. Operations onvolatile std::atomic<T>were deprecated in C++20 and can trigger-Wdeprecated/ warnings-as-errors, and MSVC-as-C behavior is unstated here. - Retaining
volatileon_Atomicis a code smell that the comment justifies as 'simply ignored'; the C standard does not guarantee that, and mixing the two can degrade codegen or emit diagnostics on some toolchains.
At minimum, the comment should state the MSVC position (verified, not assumed) since the change makes stdatomic.h a hard requirement across all supported compilers. Confidence: moderate.
📄 src/include/port/atomics.h (L16-L18)
Comment hygiene (pgsql-hackers): this header block is largely commit-message / change-history narrative ('the former platform-specific implementations ... have been removed', 'is a hard build requirement'). Comments should describe what the code does now, not the migration that produced it; this text will read as stale once the change ships. Trim to describe the current design (atomics implemented via C11 stdatomic.h in stdatomic_impl.h) and move the removal rationale into the commit message. Confidence: high.
📄 src/include/port/spin_delay.h (L119-L122)
Comment-vs-code drift (moderate confidence). The comment says "no-op with compiler barrier" / "we need to prevent the compiler from optimizing away the spin loop", but (void) 0 is NOT a compiler barrier and does nothing to prevent reordering/elision. The GCC path (__asm__ __volatile__("":::"memory")) and the MSVC path (_ReadWriteBarrier()) both emit a real barrier here; this arm silently does not. Either emit a real barrier for consistency, or fix the comment to state that no barrier is emitted and explain why it is safe (the surrounding s_lock() loop already guards its condition with pg_atomic_read_u32/pg_atomic_exchange_u32, which carry their own barriers). As written the comment is factually false.
📄 src/include/port/spin_delay.h (L74-L74)
ARM64 barrier semantics diverge between toolchains (moderate confidence). The GCC/Clang path emits a bare isb (ISB with no operand defaults to the SY full-system option per the ARM ARM, so this happens to match), while the MSVC path passes _ARM64_BARRIER_SY explicitly. This is only equivalent because bare isb already implies SY; the comment claims they match but does not state that the bare form defaults to SY, which is the only reason the two are equivalent. Consider making the equivalence explicit in a comment, or writing isb sy in the asm, to avoid a future reader assuming they differ.
📄 src/include/port/spin_delay.h (L132-L132)
Fork-specific commit-hash reference in a header comment (low confidence). Referencing commit ae27a41e0c7 and asserting "was removed as unused" bakes fork history into a public header. Such hashes drift and mean nothing to a reader of the file who does not have that history; upstream PostgreSQL header comments do not cite bare commit hashes this way. Drop the hash and state the rationale directly (this header provides the public pg_spin_delay() entry point for the stdatomic spinlock path).
📄 src/include/port/spin_delay.h (L82-L85)
Missing CPU hint for supported architectures (moderate confidence). RISC-V, s390x, and PPC/POWER all fall through to the compiler-barrier-only branch and get no real spin-delay instruction. PostgreSQL's portability gate targets these platforms; PPC in particular traditionally uses a hint here. Note that PG_SPIN_TRY_RELAXED (below) IS enabled for PPC, so on PPC you take the relaxed-load fast path but with no CPU pause hint in the delay. Confirm this matches the intended behavior for PPC64/s390x/RISC-V rather than silently regressing their spin-delay behavior.
📄 src/include/port/atomics/stdatomic_impl.h (L69-L69)
Stale/misleading comment: it justifies the 32-bit flag width with "Because slock_t (a pg_atomic_flag) is frequently packed next to other fields". But slock_t is NOT a pg_atomic_flag in this change -- spin.h defines typedef pg_atomic_uint32 slock_t; and deliberately does not use pg_atomic_flag (see the note in spin.h). The RISC-V sub-word RMW rationale for making the flag 32-bit may still be valid, but the slock_t claim is factually wrong and should be corrected/removed so the comment describes what the code actually does. (high confidence)
📄 src/include/port/atomics/stdatomic_impl.h (L322-L326)
pg_atomic_read_u32 is documented in atomics.h as having "No barrier semantics", yet this _impl uses memory_order_seq_cst. The comment openly concedes this is stronger than the contract and that the "cleaner long-term fix" is to keep the primitive relaxed and fix the specific consumer (parallel hash join / dsa). Baking a full barrier into the general read primitive is a footgun: it silently masks missing barriers at other call sites across the tree and makes it impossible for a reader to reason about the documented weak-ordering contract. It also creates an odd asymmetry with the relaxed write below. Recommend implementing the documented relaxed read and giving the hash-join consumer the acquire it needs at the call site, rather than over-strengthening the primitive. (high confidence, design)
📄 src/include/port/atomics/stdatomic_impl.h (L498-L502)
Same seq_cst-vs-documented-relaxed deviation as pg_atomic_read_u32_impl: pg_atomic_read_u64 is documented as "no barrier semantics" but implemented with memory_order_seq_cst. See the note on the u32 read; the two should be consistent and match the documented contract. (high confidence)
📄 src/include/port/atomics/stdatomic_impl.h (L315-L320)
The performance-driven memory-ordering decisions here (seq_cst reads, relaxed writes, and the RISC-V/ARM regressions cited) are non-trivial and rest on benchmarks and observed bugs that are not reproducible from the diff. Per pgsql-hackers norms, such decisions need a reference to the design discussion / thread Message-Id. The comment also uses aspirational/future-tense phrasing ("cleaner long-term fix", "out of scope", "noted as follow-up work"), which the comment-accuracy discipline discourages -- comments should describe what the code does now, with any deferred work tracked outside the source. Consider trimming to the operative rationale and moving the follow-up notes to the commit message/thread. (moderate confidence)
📄 src/include/port/atomics/stdatomic_impl.h (L216-L216)
Style/pgindent: use a single space before an end-of-line block comment rather than two, to match tree conventions and avoid pgindent reflow churn. (low confidence)
💡 Suggested change
Before:
atomic_init(ptr, 1); /* 1 = unlocked */
After:
atomic_init(ptr, 1); /* 1 = unlocked */
📄 src/test/benchmarks/atomics_analyzer.py (L43-L45)
ZeroDivisionError risk: traditional_ms is written by the producer as trad_result.elapsed_seconds * 1000, which can be 0 for a sub-millisecond timing (or a degenerate/failed run). Dividing by it here aborts the entire analysis. Guard against a zero baseline before dividing.
Confidence: medium.
💡 Suggested change
Before:
diff_pct = (
(r["traditional_ms"] - r["stdatomic_ms"]) / r["traditional_ms"] * 100
)
After:
traditional_ms = r["traditional_ms"]
diff_pct = (
(traditional_ms - r["stdatomic_ms"]) / traditional_ms * 100
if traditional_ms
else 0.0
)
📄 src/test/benchmarks/atomics_analyzer.py (L150-L154)
Empty-results crash: analyze_overall() deliberately returns {} when self.comparisons is empty (see the if not self.comparisons: return {} guard above), but generate_report then unconditionally indexes overall['total_scenarios'], overall['median_speedup'], etc., raising KeyError. This is reachable: the producer (atomics_benchmark.run_full_suite) swallows per-scenario failures and can legitimately emit "results": [], so a resultless results.json crashes the report instead of reporting "no data". Handle the empty case (early-return with a message) before dereferencing overall.
Confidence: high.
📄 src/test/benchmarks/atomics_analyzer.py (L156-L156)
Non-ASCII character (U+00B1 ±) in an emitted report string. The project requires ASCII-only source. Use +/- instead.
Confidence: high.
💡 Suggested change
Before:
lines.append(f"Mean speedup: {overall['mean_speedup']:.3f}x ± {overall['stdev_speedup']:.3f}")
After:
lines.append(f"Mean speedup: {overall['mean_speedup']:.3f}x +/- {overall['stdev_speedup']:.3f}")
📄 src/test/benchmarks/atomics_analyzer.py (L164-L164)
Non-ASCII character (U+00B1 ±) in an emitted report string; ASCII-only is required. Use +/-.
Confidence: high.
💡 Suggested change
Before:
lines.append(f"Within measurement noise: {overall['within_noise']} (±{2.0:.1f}%)")
After:
lines.append(f"Within measurement noise: {overall['within_noise']} (+/-{2.0:.1f}%)")
📄 src/test/benchmarks/atomics_analyzer.py (L13-L13)
Tuple is imported but never used in this module. Remove it to keep the import list clean.
Confidence: high.
💡 Suggested change
Before:
from typing import Dict, List, Optional, Tuple
After:
from typing import Dict, List, Optional
📄 src/test/benchmarks/atomics_benchmark.py (L295-L296)
The benchmark database is created here but never used. PostgresManager's connection pool (_create_pool) connects with database="postgres", and all subsequent work (table creation in _create_table, data loading, workloads via mgr.pool, and metrics in _collect_metrics) runs through that same pool. Nothing reconnects to benchmark_db. As a result, all benchmark tables are created in the postgres database, the intended isolation never happens, and benchmark_db is dead. Either connect the pool to benchmark_db after creating it, or drop the CREATE DATABASE calls. Confidence: high.
📄 src/test/benchmarks/atomics_benchmark.py (L341-L341)
Division by std_result.elapsed_seconds with no zero guard. In atomics_workload.py, a failed measurement query records elapsed = 0.0, and the reported elapsed_seconds is the median of the timings list, so a 0.0 median is reachable. When that happens this raises ZeroDivisionError, which propagates up and is swallowed by the broad except Exception in run_full_suite, silently discarding the entire scenario. Guard against a zero denominator (e.g. skip or flag the comparison) before dividing. Confidence: high.
📄 src/test/benchmarks/atomics_benchmark.py (L15-L16)
Unused imports: shutil, signal, datetime, and field are imported but never referenced in this module (Path and QueryPattern below are likewise unused). Per YAGNI/minimal-diff discipline, remove the dead imports. Confidence: high.
📄 src/test/benchmarks/atomics_benchmark.py (L21-L21)
Path (imported here) and QueryPattern (imported from atomics_config below) are unused in this module. Remove them along with the other dead imports. Confidence: high.
📄 src/include/storage/spin.h (L105-L106)
SpinLockRelease relies on pg_write_barrier() + a relaxed store to provide release semantics, but the documented contract of pg_write_barrier() (see atomics.h: "a write barrier acts as a compiler barrier, and also orders stores") is store-store ordering only. It does not promise that prior loads in the critical section are ordered before the unlock store. A correct lock release requires full release semantics (all prior loads AND stores complete before the store is observed free); otherwise a load inside the critical section may be reordered after the unlock on weakly-ordered CPUs (ARM64/PPC64/RISC-V), breaking mutual exclusion.
This happens to work today only because this stdatomic build defines pg_write_barrier_impl() as atomic_thread_fence(memory_order_release), which does order prior loads. That is an implementation detail stronger than the public API contract; a future retuning of pg_write_barrier() to a true store-store barrier would silently break every spinlock. Use the primitive that documents release/barrier semantics for exactly this case instead: pg_atomic_write_membarrier_u32(lock, 0) (referenced by the write-path comment in stdatomic_impl.h and by atomics.h as the correct "ordered write"). If the seq_cst store's cost on ARM64 is the concern the comment raises, that is a justification to add a documented pg_atomic_write_release_u32 helper, not to depend on pg_write_barrier() being stronger than its contract. (high confidence)
💡 Suggested change
Before:
pg_write_barrier();
pg_atomic_write_u32(lock, 0);
After:
pg_atomic_write_membarrier_u32(lock, 0);
📄 src/include/storage/spin.h (L101-L103)
This comment calls pg_write_barrier() a "release fence", but pg_write_barrier() is documented in atomics.h as a store-store barrier ("orders stores"), not a release fence. __sync_lock_release() is a release operation that also orders prior loads, so the claimed equivalence is inaccurate against the public API contract. Please align the comment with whichever primitive is actually used (see the related correctness comment); as written it will mislead future readers into thinking pg_write_barrier() guarantees release ordering. (moderate confidence)
📄 src/test/benchmarks/atomics_cli.py (L172-L177)
The --distribution all branch expands to only RANDOM, CLUSTERED, and LOW_CARDINALITY, silently dropping HIGH_NULL even though high_null is advertised as a valid --distribution choice in the argparser. A user selecting all (the default) reasonably expects every advertised distribution to run, so this under-tests the high_null path and is a POLA violation. Either include DataDistribution.HIGH_NULL here or drop high_null from the --distribution choices. (moderate confidence)
💡 Suggested change
Before:
if args.distribution == "all":
distributions = [
DataDistribution.RANDOM,
DataDistribution.CLUSTERED,
DataDistribution.LOW_CARDINALITY,
]
After:
if args.distribution == "all":
distributions = [
DataDistribution.RANDOM,
DataDistribution.CLUSTERED,
DataDistribution.LOW_CARDINALITY,
DataDistribution.HIGH_NULL,
]
📄 src/test/benchmarks/atomics_config.py (L1-L2)
Scope/hygiene (high confidence): this file introduces a brand-new, non-standard src/test/benchmarks/ directory (no such directory existed before this change) containing ad-hoc developer benchmark scaffolding. None of these Python scripts are wired into any Makefile, meson.build, or test schedule. Bundling throwaway performance tooling with the substantive atomics/spinlock change (s_lock.c, atomics.h, stdatomic_impl.h, spin.h) violates the "one patch does one thing / minimal diff" discipline and will draw rejection on -hackers. Performance claims should be backed by a reproducible benchmark discussed on the list, not committed as tree scaffolding. Consider dropping this suite from the patch (or splitting it into its own clearly-separated proposal).
📄 src/test/benchmarks/atomics_config.py (L259-L260)
Hardcoded absolute paths (/tmp/pgdata-traditional, /tmp/pgdata-stdatomic) and fixed ports (5433/5434) are baked in as the effective defaults. This is a portability and collision hazard: /tmp is not appropriate/writable on all platforms (e.g. Windows/MSVC), the ports may already be in use, and two concurrent runs will clash on both the data dir and the port. PostgreSQL test infrastructure conventions forbid hardcoded ports/paths (TAP tests allocate ports dynamically). Prefer a temp dir via tempfile/PGDATA and a dynamically allocated / configurable port with no fixed default.
📄 src/test/benchmarks/atomics_config.py (L101-L105)
get_dsn builds the DSN by raw string concatenation with no quoting/escaping of the field values. If host/dbname/user/password ever contain a space or a special character (e.g. a symbol in a password), the resulting connection string is malformed or misparsed. Use a proper DSN builder (e.g. psycopg's make_dsn/keyword args) or single-quote and backslash-escape each value per libpq connection-string rules.
📄 src/test/benchmarks/atomics_workload.py (L236-L239)
On measurement failure this appends 0.0 to timings and continues. The median at line 243 then silently blends real timings with fabricated zeros, producing misleadingly fast numbers with no signal that the results are invalid. For a benchmark whose entire purpose is comparing 'traditional' vs 'stdatomic', reporting fabricated 0.0 timings on error is a correctness/validity hazard. Prefer skipping the failed iteration (don't append) and, if all iterations failed, mark the result as invalid rather than emitting a median of zeros.
Confidence: high.
📄 src/test/benchmarks/atomics_workload.py (L140-L140)
_concurrent_insert_query emits nextval('seq_concurrent_insert'), but nothing in this file or the rest of the benchmark harness creates the seq_concurrent_insert sequence (verified: this is the only reference in src/test/benchmarks/). When the CONCURRENT_INSERT pattern runs, every iteration fails with an undefined-sequence error; the broad except in _run_single swallows it and records 0.0 timings, yielding meaningless results for this pattern. Create the sequence during setup or generate the id differently.
Confidence: high.
📄 src/test/benchmarks/atomics_workload.py (L281-L282)
WorkloadResult is created with implementation="" and _run_single is invoked with implementation=result.implementation, which is always "" at this point. The caller in atomics_benchmark.py (run_workload at line 394) never assigns workload_result.implementation afterward, so every QueryResult.implementation stays an empty string. This breaks the traditional-vs-stdatomic labeling the whole benchmark depends on. Pass the implementation into run_workload as a parameter and set it on both WorkloadResult and the QueryResults.
Confidence: high.
📄 src/test/regress/pg_regress.c (L1246-L1246)
Switching execl -> execlp here is unjustified and introduces a security footgun. SHELLPROG is defined by the build system as $(SHELL) (GNUmakefile/ecpg) or "/bin/sh" (meson), i.e. an absolute path in every standard configuration. When the program name contains a slash, execlp behaves identically to execl and performs no PATH search, so this change is a no-op in all normal builds -- it achieves nothing the commit claims. The only case where it differs is if SHELLPROG were ever a bare name (no slash): then execlp resolves the shell via the PATH environment variable, letting an attacker-controlled PATH select which sh runs the test commands. That is strictly worse than the previous behavior and an unsafe default. Revert to execl unless there is a concrete, documented reason (and design-thread reference) for a PATH-based shell lookup. (high confidence)
💡 Suggested change
Before:
execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
After:
execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
📄 src/tools/pgindent/pgindent (L1-L1)
This shebang change diverges from the tree-wide convention: every other Perl script in the PostgreSQL source (e.g. src/tools/copyright.pl, src/tools/mark_pgdllimport.pl, src/backend/catalog/genbki.pl, and ~30 others) uses #!/usr/bin/perl. Making pgindent the sole exception with #!/usr/bin/env perl is an unrelated, inconsistent change that a committer will reject. Also note #!/usr/bin/env perl is not universally portable (some hardened/build environments restrict PATH-based interpreter lookup), which is why the project standardized on the absolute path. Revert to keep the diff minimal and consistent, or raise the convention change separately on -hackers for the whole tree.
💡 Suggested change
Before:
#!/usr/bin/env perl
After:
#!/usr/bin/perl
📄 src/test/benchmarks/data_generator.py (L352-L352)
Reproducibility/comparison-fairness defect (high confidence). This UUID expression uses random(), which is non-deterministic and unseeded per session. generate_server_side_insert is run separately on the two instances being compared (traditional vs stdatomic, atomics_benchmark.py:373 for each), and the harness derives a speedup by comparing query timings against those tables (atomics_benchmark.py:341). Because random() produces different values on each instance, any schema with UUID columns (MEDIUM_SCHEMA ref_uuid, WIDE_SCHEMA col_uuid_*) under RANDOM/CLUSTERED will get divergent data, making the comparison unfair and contradicting the module's stated "reproducible seeded" contract. Derive the UUID deterministically from g and seed_val (e.g. md5(g::text || '{seed_val}')::uuid) instead of random().
💡 Suggested change
Before:
return f"md5(g::text || '{seed_val}' || random()::text)::uuid"
After:
return f"md5(g::text || '{seed_val}')::uuid"
📄 src/test/benchmarks/data_generator.py (L398-L398)
Inconsistent SQL literal quoting (low confidence, latent footgun). Every other array-of-text branch builds SQL string literals via '{t}', but here repr(t) is used. For the current LOW_CARD_TEXT values this happens to yield valid single-quoted SQL, but repr() is Python literal syntax, not SQL: if any value ever contained a single quote, repr() would switch to double quotes and produce invalid SQL. Use the same '{t}' form as the other branches for consistency and safety.
💡 Suggested change
Before:
f"(ARRAY[{','.join(repr(t) for t in LOW_CARD_TEXT)}])"
After:
f"(ARRAY[{','.join(chr(39) + t + chr(39) for t in LOW_CARD_TEXT)}])"
📄 src/test/benchmarks/data_generator.py (L13-L13)
Optional is imported but never used anywhere in this module. Remove it to keep the import list clean.
💡 Suggested change
Before:
from typing import Any, List, Optional
After:
from typing import Any, List
| watch_file flake.nix | ||
| use flake | ||
| use project_steering postgresql | ||
| use project_mcp postgresq |
There was a problem hiding this comment.
Typo in the environment name: project_mcp postgresq (missing trailing l). This is inconsistent with use project_steering postgresql on the line above. Even so, this whole file should be removed rather than fixed.
| use project_mcp postgresq | |
| +use project_mcp postgresql |
| #export MESON_EXTRA_SETUP="-Db_coverage=true" | ||
| #export GENINFO_OPTIONS="--ignore-errors inconsistent,gcov" | ||
| #export LCOV_OPTIONS="--ignore-errors inconsistent,gcov" |
There was a problem hiding this comment.
Large blocks of commented-out export statements (libumem tuning, coverage, sanitizer CFLAGS) are dead/commented-out config. PostgreSQL patch hygiene forbids commented-out code and speculative scaffolding for paths that aren't wired up. Remove.
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
Committing .clangd at the repo root contradicts PostgreSQL's own documented policy. The root .gitignore states: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." A clangd/LSP config is exactly such an editor-specific auxiliary file. A patch adding it will be rejected on -hackers as an unrelated, non-minimal change. Keep this file local (via .git/info/exclude or a local excludesFile) instead of tracking it. (high confidence)
| - -I. | ||
| - -I../../../../src/include |
There was a problem hiding this comment.
The include path -I../../../../src/include is a relative path anchored to a specific nesting depth (four levels up). Since .clangd sits at the repo root, src/include is directly under it, so the correct relative path would be -Isrc/include (matching the -I. above it). As written, this points outside the repository and will not resolve, defeating the purpose of the config. (moderate confidence)
| - -I. | |
| - -I../../../../src/include | |
| - -I. | |
| - -Isrc/include |
| @@ -0,0 +1,41 @@ | |||
| # LiteLLM proxy config — bridges Open Code Review (OpenAI protocol) to AWS Bedrock. | |||
There was a problem hiding this comment.
Non-ASCII em-dash (—) in a source file comment. The contribution standard requires ASCII only in source and diffs (no smart quotes, em-dashes, or ellipsis characters). Replace the em-dash with an ASCII equivalent such as - or ; .
The same issue appears on line 24 (Set it DIRECTLY here — NOT via).
| # LiteLLM proxy config — bridges Open Code Review (OpenAI protocol) to AWS Bedrock. | |
| # LiteLLM proxy config - bridges Open Code Review (OpenAI protocol) to AWS Bedrock. |
| ocr-review: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Neither ocr-review nor pg-history sets timeout-minutes. Given the 120s LiteLLM readiness loop, per-request LLM timeouts with concurrency, and the fallback per-comment posting that sleeps 1200ms per comment with retry backoffs up to 60s each, a hung LiteLLM proxy or stalled Bedrock call could keep the job alive until the platform default (6 hours), wasting runner minutes and delaying concurrency cancellation. Add an explicit timeout-minutes to each job.
| ocr-review: | |
| runs-on: ubuntu-latest | |
| ocr-review: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 30 |
| # vars.AWS_ROLE_ARN - IAM role to assume via OIDC (granting bedrock:InvokeModel*) | ||
| # vars.AWS_REGION - e.g. us-east-1 | ||
| # vars.OCR_BEDROCK_MODEL - LiteLLM model string for the Opus inference profile, e.g. | ||
| # bedrock/converse/us.anthropic.claude-opus-4-8 |
There was a problem hiding this comment.
The example model string here (claude-opus-4-8) is inconsistent with the error message in the "Start LiteLLM proxy" step (claude-opus-4-1-20250805-v1:0) and the pip-install comment (Claude Opus 4.8 / PyPI latest 1.87.1). These drift-prone references confuse maintainers about which profile is actually expected. Reconcile the model-string examples so they are consistent across the file.
| on: | ||
| schedule: | ||
| # Run hourly every day | ||
| - cron: '0 * * * *' | ||
| workflow_dispatch: |
There was a problem hiding this comment.
No concurrency control on a workflow that force-pushes the default branch. This runs hourly (cron) AND on workflow_dispatch, and a sibling sync-upstream-manual.yml also force-pushes origin master. Two overlapping runs can rebase and force-push the same branch concurrently, racing each other and permanently discarding commits pushed to origin/master in between (the --force-with-lease in the later push step only guards against the specific ref the runner fetched, not against another sync run overwriting it). Add a concurrency group shared across the sync workflows with cancel-in-progress: false so runs serialize instead of racing.
High confidence.
| on: | |
| schedule: | |
| # Run hourly every day | |
| - cron: '0 * * * *' | |
| workflow_dispatch: | |
| on: | |
| schedule: | |
| # Run hourly every day | |
| - cron: '0 * * * *' | |
| workflow_dispatch: | |
| concurrency: | |
| group: sync-upstream-master | |
| cancel-in-progress: false |
| sync: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: write | ||
| issues: write |
There was a problem hiding this comment.
The job has no timeout-minutes. This unattended scheduled job fetches the full upstream history (fetch-depth: 0) and performs a rebase; a hang would tie up runner minutes indefinitely, once per hour. Other workflows in this repo (e.g. pg-ci.yml) already set timeout-minutes; add an explicit timeout here for consistency and reliability.
Moderate confidence.
| sync: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: write | |
| issues: write | |
| sync: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 15 | |
| permissions: | |
| contents: write | |
| issues: write |
| echo "⚠️ Local master has $DIVERGED commits not in upstream" | ||
|
|
||
| # Check commit messages for "dev setup" or "dev v" pattern | ||
| DEV_SETUP_COMMITS=$(git log --format=%s upstream/master..origin/master | grep -iE "^dev (setup|v[0-9])" | wc -l) |
There was a problem hiding this comment.
Inconsistent commit-range notation makes the divergence classification unreliable. The dev_setup_commits count uses the two-dot range upstream/master..origin/master (commits reachable from origin/master but not upstream/master), while non_github_changes below uses the three-dot symmetric-difference range upstream/master...origin/master for git diff --name-only. For a diverged history these two ranges include different commits, so a commit set can be classified as "only .github/ changes" by one check while the dev-setup subject grep operates over a different commit set. Since these outputs directly gate a destructive rebase + --force-with-lease push (see the Attempt merge/Push to origin steps), a misclassification can either block a legitimate sync or allow overwriting real work. Use one consistent range (two-dot upstream/master..origin/master, i.e. the actual local commits) for all three computations.
High confidence.
Nix-based development environment for PostgreSQL hacking. Not for merge; staged here so per-commit build/test runs can share a single toolchain.
PostgreSQL requires C11, but not all supported compilers provide a fully
functional <stdatomic.h>. This adds an alternative implementation of
port/atomics.h built on the standard C11 <stdatomic.h> header, selectable
at build time alongside the existing platform-specific implementations.
Build-system detection (meson and autoconf):
A new option -- meson -Duse_stdatomic=yes|no|auto and autoconf
--with-stdatomic=yes|no|auto -- controls the choice:
* 'auto' (default): use stdatomic.h if a probe compile succeeds
* 'yes': require stdatomic.h (fail if unavailable)
* 'no': use the traditional platform-specific implementation
Both build systems run the same probe (32-bit fetch_add, a seq_cst
thread fence, and a 64-bit compare_exchange) and define USE_STDATOMIC_H
when it succeeds and the option is not 'no'. For MSVC, stdatomic.h is
only available in Visual Studio 2022 or later with /experimental:c11atomics,
which the build adds automatically; earlier MSVC falls back to the
traditional implementation.
Implementation (src/include/port/atomics/stdatomic_impl.h):
All atomic types (pg_atomic_flag, pg_atomic_uint8/16/32/64) are
implemented with C11 _Atomic. Memory ordering matches the traditional
implementation's observable semantics:
* pg_atomic_read/write_* use memory_order_relaxed, matching the plain
volatile loads/stores of the traditional path (no fence).
* read-modify-write, exchange, and compare-exchange use
memory_order_seq_cst, matching the traditional "full barrier"
contract.
* the _membarrier read/write variants use memory_order_seq_cst.
C++ compatibility is provided via <atomic> for C++11 through C++20,
which do not include <stdatomic.h>; C++23 and later include it directly.
Platform-specific spin-delay hints (PAUSE, ISB, YIELD) are factored into
a new src/include/port/spin_delay.h, with the shared SpinDelayStatus type
and helpers in src/include/port/spin_delay_status.h so both spinlock paths
use them. On ARM64, ISB is used instead of YIELD for better backoff under
contention; on Windows ARM64 this adds a spin delay where the traditional
implementation had none.
Wire-up (src/include/port/atomics.h, storage/spin.h, s_lock.c):
When USE_STDATOMIC_H is defined, atomics.h includes stdatomic_impl.h;
otherwise it includes the arch-*.h and generic-*.h headers exactly as
before. Both paths feed the same shared public-API layer (the static
inline pg_atomic_* wrappers), so the API is identical either way.
For spinlocks, when USE_STDATOMIC_H is defined slock_t is a
pg_atomic_uint32 (0 == unlocked, 1 == locked, so a zeroed slock_t is a
valid free lock) and the SpinLock* operations are built on the u32 atomic
API rather than platform-specific TAS assembly. SpinLockAcquire is a thin
wrapper macro over an inline helper: the macro captures the call site's
__FILE__/__LINE__/__func__ for stuck-spinlock diagnostics while the helper
evaluates its lock argument exactly once. On strong-memory platforms a
relaxed-load test precedes the atomic exchange (PG_SPIN_TRY_RELAXED).
When USE_STDATOMIC_H is not defined, spinlock behavior is byte-for-byte
unchanged from previous PostgreSQL versions.
With stdatomic.h, atomic operations no longer depend on backend-only
infrastructure, so the frontend include restriction in atomics.h is relaxed
for that path.
The traditional implementation remains the default (auto-detect uses it
when stdatomic.h is unavailable), so this is an opt-in alternative rather
than a replacement. Both code paths provide identical public API and
semantics.
Discussion: https://postgr.es/m/CC76554F-41AC-45AA-AF10-370FEC416498%40greg.burd.me#dd4b139cd557d6e87a9f2d706b6415ae
Co-authored-by: Thomas Munro <tmunro@postgresql.org>
Explain the coexistence of the two atomic implementations and their
semantic differences.
src/backend/storage/lmgr/README gains an "Atomic Operations" section
describing the build-time selection and when each path is used.
STDATOMIC_DIFFERENCES.md catalogs every difference between the
implementations:
* Memory ordering: stdatomic.h read/write use relaxed (matching the
traditional plain volatile loads/stores); RMW/CAS use seq_cst; the
_membarrier variants use seq_cst.
* Spinlock flag polarity: an internal detail (stdatomic 1=unlocked vs
traditional 0=unlocked) with no external API impact.
* Platform-specific optimizations: ARM64 ISB vs YIELD, PowerPC
hand-optimized assembly vs compiler-generated, x86 lock;addl vs
compiler-emitted fence.
* Binary compatibility: intentionally broken between implementations.
Differing atomic type sizes (e.g. pg_atomic_flag 4 bytes vs 1 byte)
mean a cluster must be rebuilt when switching implementations; this
is fine because the choice is made at build time.
* New capabilities on the stdatomic path: C++ support, frontend-code
support, and a Windows ARM64/MSVC spin delay.
Discussion: https://postgr.es/m/CC76554F-41AC-45AA-AF10-370FEC416498%40greg.burd.me#dd4b139cd557d6e87a9f2d706b6415ae
Python-based A/B benchmark harness for comparing the stdatomic.h and traditional atomic implementations under load. Local development tooling; not part of the submitted series.
….h exclusively *** NOT FOR MERGE in this cycle. *** This commit is included to show the intended long-term destination of the series, and to let reviewers and the buildfarm exercise the stdatomic-only end state -- but it is deliberately NOT proposed for commit yet. The plan is to ship the earlier patches (which add C11 <stdatomic.h> as a *selectable* implementation alongside the traditional platform-specific code), let the stdatomic path prove itself in the field across the buildfarm and at least one release, and only THEN remove the traditional implementation via a commit like this one. Removing the battle-tested arch-*/generic-* code on day one is not being asked for here; keeping both paths until stdatomic has earned trust is. Everything below describes what this (eventual) removal commit does. This collapses the dual-path atomics into a single stdatomic-only implementation, mirroring the destination state of the series. Rather than selecting between C11 <stdatomic.h> and the platform-specific implementations at build time, stdatomic.h is now the sole backend for PostgreSQL's atomic operations and USE_STDATOMIC_H is effectively always-on. The platform-specific implementation files are deleted outright: - src/include/port/atomics/arch-arm.h - src/include/port/atomics/arch-ppc.h - src/include/port/atomics/arch-x86.h - src/include/port/atomics/generic.h - src/include/port/atomics/generic-gcc.h - src/include/port/atomics/generic-msvc.h - src/include/port/atomics/fallback.h atomics.h drops its #ifdef USE_STDATOMIC_H / #else conditional and the now-dead includes of the arch-*/generic-*/fallback headers, keeping only the stdatomic_impl.h include and the common public-API layer of static inline pg_atomic_* wrappers. storage/spin.h and s_lock.c lose their traditional-spinlock (#else) branches, so only the pg_atomic_flag-based spinlock path remains; the s_lock.h includes that served only that path are removed. Detection is retained but now hard-errors when a working C11 stdatomic.h is unavailable, in both meson.build and configure.ac, instead of silently falling back. The only remaining hand-written platform hint is the CPU spin-delay instruction in src/include/port/spin_delay.h, which stdatomic.h does not cover; it is intentionally left in place along with spin_delay_status.h and stdatomic_impl.h. This depends on the prior commits in the series (build-time detection, stdatomic_impl.h, conditional wire-up, documentation, and the test_atomics module). Co-authored-by: Thomas Munro <tmunro@postgresql.org>
There was a problem hiding this comment.
🔍 OCR found 60 issue(s).
- 25 inline, 35 in summary (inline capped at 25)
📄 src/include/port/atomics/stdatomic_impl.h (L238-L238)
The != 0 polarity here is a footgun for zero-initialized shared memory. Under the 1=unlocked convention, a pg_atomic_flag living in memory that was set up via memset(...,0,...)/zeroed DSM (a very common PostgreSQL pattern) reads as 0 == locked, so the first pg_atomic_test_set_flag() returns false and the flag is permanently "held" until an explicit pg_atomic_init_flag() runs. The traditional generic.h convention (0=unlocked) made a zeroed flag a valid free flag. Extensions such as contrib code relying on zero-init would silently deadlock. This inverted convention should be called out as a hard requirement to always pg_atomic_init_flag() before use, or reconsidered.
📄 src/include/port/atomics/stdatomic_impl.h (L325-L329)
Contract divergence and footgun: pg_atomic_read_u32() is documented in atomics.h as "No barrier semantics", but this returns a full seq_cst load. The comment itself admits the seq_cst is stronger than the documented contract and is compensating for a relaxed store plus dependent-load ordering that the "correct" fix (an acquire read at the hash-join call site / dsa_pointer_atomic_read_membarrier) is deliberately left out of scope. Baking a consumer-specific correctness workaround into the lowest-level primitive is fragile: the compiler-ordering property of the old volatile read is not what seq_cst provides, and future callers get a silent, undocumented stronger guarantee they may come to depend on. Either make the read match the documented relaxed contract and fix the specific consumer, or update the atomics.h documentation to reflect the new guarantee. This out-of-scope 'follow-up work' narrative comment also violates the comments-describe-current-behavior convention.
📄 src/include/port/atomics/stdatomic_impl.h (L106-L106)
This hard StaticAssertDecl (== 2, always-lock-free) contradicts the change's own storage/lmgr/README, which states 64-bit atomics on 32-bit ARM "may emulate them with locks". On such a target ATOMIC_LLONG_LOCK_FREE is 1 (or 0), so this assertion fails the build on a platform the README claims is supported, dropping 32-bit support that the removed fallback.h previously provided. If cross-process lock-free atomics really are a hard requirement, the README wording is wrong; if 32-bit-with-emulation is still supported, this assert is too strict. Reconcile the two.
📄 src/include/port/atomics/stdatomic_impl.h (L102-L102)
Correctness of the whole cross-process safety guarantee is disabled for MSVC-C based on an unverified claim that its <stdatomic.h> spuriously reports LOCK_FREE==1. If that claim is wrong (or changes across MSVC versions), a non-lock-free (libatomic-style, process-local-lock) atomic would silently corrupt cross-process shared memory here with no compile-time or run-time check. Back this carve-out with a run-time Assert(atomic_is_lock_free(...)) on a startup path, or with a concrete reference (MSVC version + doc/thread) justifying the exemption.
📄 src/include/port/atomics/stdatomic_impl.h (L126-L128)
PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY is gated solely on pointer width (SIZEOF_VOID_P >= 8), and the comment asserts "All 64-bit architectures ... guarantee" 8-byte single-copy atomicity for plain aligned loads/stores. bufmgr.c uses this to skip atomic ops on a hot path; pointer width alone does not prove non-atomic aligned 8-byte access is tear-free on every 64-bit ABI/toolchain. Prefer deriving this from an actual 64-bit-atomic-lock-free property (e.g. tie it to the ATOMIC_LLONG_LOCK_FREE guarantee) rather than pointer size, or narrow the claim in the comment.
📄 src/test/benchmarks/atomics_analyzer.py (L43-L45)
Division by zero: diff_pct divides by r["traditional_ms"] with no guard. The results JSON is an external, unvalidated file; a record with traditional_ms == 0 (degenerate/partial/malformed run) raises an uncaught ZeroDivisionError and aborts the whole analysis with a raw traceback. Guard against a zero denominator. (moderate confidence)
💡 Suggested change
Before:
diff_pct = (
(r["traditional_ms"] - r["stdatomic_ms"]) / r["traditional_ms"] * 100
)
After:
trad = r["traditional_ms"]
diff_pct = (
(trad - r["stdatomic_ms"]) / trad * 100 if trad else 0.0
)
📄 src/test/benchmarks/atomics_analyzer.py (L151-L154)
Empty-input crash: analyze_overall() returns {} when self.comparisons is empty, but generate_report() immediately dereferences overall['total_scenarios'] here, raising KeyError: 'total_scenarios' for a results file with an empty results list. The empty case is guarded in analyze_overall but not honored by its only caller. Return early (or emit a "no results" report) when overall is empty. (high confidence)
💡 Suggested change
Before:
overall = self.analyze_overall()
lines.append("OVERALL PERFORMANCE")
lines.append("-" * 80)
lines.append(f"Total scenarios: {overall['total_scenarios']}")
After:
overall = self.analyze_overall()
if not overall:
lines.append("No results to analyze.")
lines.append("=" * 80)
return "\n".join(lines)
lines.append("OVERALL PERFORMANCE")
lines.append("-" * 80)
lines.append(f"Total scenarios: {overall['total_scenarios']}")
📄 src/test/benchmarks/atomics_analyzer.py (L66-L66)
Non-ASCII character: the ± (U+00B1) in this comment violates PostgreSQL's ASCII-only source rule. Use "+/-" instead. (high confidence)
💡 Suggested change
Before:
# Statistical significance: count how many are outside ±2% threshold
After:
# Statistical significance: count how many are outside +/-2% threshold
📄 src/test/benchmarks/atomics_analyzer.py (L156-L156)
Non-ASCII character: the ± (U+00B1) in this report string violates PostgreSQL's ASCII-only source rule and can render inconsistently across locales/terminals. Use "+/-". (high confidence)
💡 Suggested change
Before:
lines.append(f"Mean speedup: {overall['mean_speedup']:.3f}x ± {overall['stdev_speedup']:.3f}")
After:
lines.append(f"Mean speedup: {overall['mean_speedup']:.3f}x +/- {overall['stdev_speedup']:.3f}")
📄 src/test/benchmarks/atomics_analyzer.py (L164-L164)
Non-ASCII character: the ± (U+00B1) here violates PostgreSQL's ASCII-only source rule. Use "+/-". (high confidence)
💡 Suggested change
Before:
lines.append(f"Within measurement noise: {overall['within_noise']} (±{2.0:.1f}%)")
After:
lines.append(f"Within measurement noise: {overall['within_noise']} (+/-{2.0:.1f}%)")
📄 src/include/storage/spin.h (L104-L106)
The comment describes the release store as a "plain store" (twice: here and "the store-side serialization (STLR on ARM64) that a seq_cst store adds"). In this implementation pg_atomic_write_u32() is a memory_order_relaxed atomic store, not a plain (non-atomic) store. Calling it "plain" is misleading: a genuinely plain store to an object that is concurrently read by atomic loads on the acquire side would be a data race / UB under the C11 model, and the correctness of the release-fence idiom relies specifically on it being a relaxed atomic store. Suggest wording that makes the atomic-ness explicit to avoid a future reader "optimizing" it into a non-atomic write.
Additionally, naming the concrete ARM64 instruction ("STLR") hardcodes a platform detail that can drift as compilers/architectures change; the correctness argument only needs "a seq_cst store adds store-side serialization on weakly-ordered targets." Confidence: high on the atomic/plain wording, low on the STLR nit.
💡 Suggested change
Before:
* store; the plain store then publishes the unlocked value. This gives
* the same release semantics as the traditional S_UNLOCK() while avoiding
* the store-side serialization (STLR on ARM64) that a seq_cst store adds.
After:
* store; the following relaxed atomic store then publishes the unlocked
* value. This gives the same release semantics as the traditional
* S_UNLOCK() while avoiding the store-side serialization that a seq_cst
* store adds on weakly-ordered architectures.
📄 src/test/benchmarks/atomics_benchmark.py (L295-L296)
Wrong-database execution: benchmark_db is created here, but the connection pool created in _create_pool is hardcoded to database="postgres" and is never re-pointed to benchmark_db. As a result every subsequent CREATE TABLE, data load, and workload query (all issued through mgr.pool) runs in the postgres database, and benchmark_db is never used. Either connect the pool to benchmark_db after creating it, or drop this CREATE DATABASE step. (high confidence)
📄 src/test/benchmarks/atomics_benchmark.py (L341-L341)
ZeroDivisionError risk and inverted metric semantics. std_result.elapsed_seconds can be exactly 0.0: AtomicsWorkloadRunner._run_single sets elapsed = 0.0 on a failed measurement query and appends it, so the median can be 0.0, causing a ZeroDivisionError that aborts the whole scenario. Additionally, this computes trad/std, but ComparisonResult.speedup is documented as stdatomic_ms / traditional_ms (< 1.0 means faster), which is the inverse. The summary's stdatomic_faster_count (s > 1.0) / traditional_faster_count (s < 1.0) therefore contradict the documented meaning. Guard against zero and reconcile the formula with the docstring. (high confidence)
📄 src/test/benchmarks/atomics_benchmark.py (L402-L403)
Table is not dropped on the error path. If any step between _create_table and this DROP fails (e.g. the data load or workload raises), the DROP is skipped and the table survives. Since table_name is deterministic ({schema.name}_{distribution.value}), a later scenario with the same schema/distribution will hit relation already exists. Move the DROP into a finally block, or use CREATE TABLE ... after an unconditional DROP TABLE IF EXISTS. (moderate confidence)
📄 src/test/benchmarks/atomics_benchmark.py (L500-L500)
median_speedup uses sorted(speedups)[len(speedups) // 2], which returns the upper-middle element rather than the true median (for even-length lists it should average the two middle values). Use statistics.median(speedups) for correctness. (moderate confidence)
📄 src/test/benchmarks/atomics_benchmark.py (L506-L506)
neutral_count relies on exact float equality (s == 1.0). Measured wall-clock timings will essentially never produce an exact ratio of 1.0, so this counter is effectively always 0. If a neutral band is intended, compare within a tolerance (e.g. math.isclose). (low confidence)
📄 src/test/benchmarks/atomics_benchmark.py (L183-L189)
The postgres process is started with stdout=subprocess.PIPE, stderr=subprocess.PIPE but nothing ever drains these pipes (no reader thread, no communicate()). With logging_collector=off, server logs go to stderr; if the OS pipe buffer fills during a long benchmark, the server will block on write and hang. Redirect these to files or DEVNULL, or spawn a drain thread. (moderate confidence)
📄 src/test/benchmarks/atomics_cli.py (L181-L190)
--pattern all does not test all query patterns. QueryPattern defines 9 members (including CONCURRENT_INSERT, CONCURRENT_UPDATE, MIXED_WORKLOAD), and the argparse choices are built from all of them, yet this branch hardcodes only 6. This contradicts the --pattern help text ("Query pattern to test (default: all)") and violates POLA: a user asking for all silently gets a subset. Either rename the option/help to reflect the read-only default set, or derive the list from QueryPattern and let the harness skip patterns it cannot run. The "for now" comment is also aspirational/stale.
📄 src/test/benchmarks/atomics_cli.py (L172-L177)
--distribution all omits DataDistribution.HIGH_NULL. "high_null" is an accepted --distribution choice and a defined enum member, but selecting all expands to only 3 of the 4 distributions, so all is not actually all. Either include HIGH_NULL here or drop it from the argparse choices. Consider distributions = list(DataDistribution) to keep the two in sync.
📄 src/test/benchmarks/atomics_cli.py (L209-L212)
Silent-override footgun: when --full-matrix is given together with --rows, the user's --rows is ignored. build_config sets config.row_counts = args.rows, but full_matrix=True makes AtomicsBenchmarkConfig.get_row_counts() return the hardcoded ROW_COUNTS and never consult row_counts. The banner prints config.get_row_counts(), so it will silently show the full-matrix counts instead of the requested rows. Warn/error on the conflicting combination, or have --rows take precedence.
📄 src/test/benchmarks/atomics_config.py (L8-L9)
Unused import: os is imported here but never referenced anywhere in this module (no os. usage exists in the file). pyflakes/git diff --check-style hygiene will flag it. Remove it.
💡 Suggested change
Before:
import os
from dataclasses import dataclass, field
After:
from dataclasses import dataclass, field
📄 src/test/benchmarks/atomics_workload.py (L227-L229)
rows is only bound on the success path inside the measurement loop. If every one of measure_iterations runs raises an exception, the except branch runs each time, rows is never assigned, and row_count=len(rows) if rows else 0 below raises NameError: name 'rows' is not defined. This converts a caught-and-logged query failure into an unhandled crash. Initialize rows = None before the loop.
💡 Suggested change
Before:
# Measure
timings = []
for _ in range(self.measure_iterations):
After:
# Measure
rows = None
timings = []
for _ in range(self.measure_iterations):
📄 src/test/benchmarks/atomics_workload.py (L236-L239)
A failed measurement appends elapsed = 0.0 into timings, injecting fake zero-latency samples that corrupt the median and silently produce bogus (fast) benchmark numbers instead of surfacing the failure. Additionally, if measure_iterations is 0, timings stays empty and timings[len(timings) // 2] raises IndexError. Exclude failed iterations from timings and guard against an empty list before computing the median.
💡 Suggested change
Before:
except Exception as e:
logger.error("Measurement query failed: %s", e)
elapsed = 0.0
timings.append(elapsed)
After:
except Exception as e:
logger.error("Measurement query failed: %s", e)
📄 src/test/benchmarks/atomics_workload.py (L241-L243)
median_time indexes timings without checking it is non-empty. If all measurement iterations fail (or measure_iterations == 0), timings is empty and this raises IndexError. Guard for the empty case.
💡 Suggested change
Before:
# Use median timing
timings.sort()
median_time = timings[len(timings) // 2]
After:
# Use median timing
timings.sort()
median_time = timings[len(timings) // 2] if timings else 0.0
📄 src/test/benchmarks/atomics_workload.py (L277-L282)
WorkloadResult.implementation is initialized to "" and immediately passed as implementation=result.implementation into every QueryResult, so each QueryResult.implementation is permanently empty. Verified: nothing anywhere assigns .implementation, and the caller _run_on_instance holds the correct impl_name ("traditional"/"stdatomic") but never forwards it. The per-query implementation label is lost, defeating the traditional-vs-stdatomic comparison. Add an implementation parameter to run_workload (or set the field before running patterns) and pass it through.
📄 src/test/benchmarks/atomics_workload.py (L139-L140)
_concurrent_insert_query emits nextval('seq_concurrent_insert') for the id column, but no CREATE SEQUENCE seq_concurrent_insert exists anywhere in the benchmark harness (verified via search). Every CONCURRENT_INSERT execution will fail with an undefined-object error, which is only logged and then produces a bogus 0.0 timing. Either create the sequence as part of table setup or drop the dependency on it.
📄 src/test/benchmarks/atomics_workload.py (L229-L234)
The measurement timer starts before pool.acquire(), so connection-checkout latency is folded into the measured query time. For a micro-benchmark comparing atomics implementations this skews results and adds pool-contention noise into the hot region. Start the timer immediately before conn.fetch(query) (after acquiring the connection).
💡 Suggested change
Before:
for _ in range(self.measure_iterations):
start = time.perf_counter()
async with self.pool.acquire() as conn:
try:
rows = await conn.fetch(query)
elapsed = time.perf_counter() - start
After:
for _ in range(self.measure_iterations):
async with self.pool.acquire() as conn:
try:
start = time.perf_counter()
rows = await conn.fetch(query)
elapsed = time.perf_counter() - start
📄 src/test/regress/pg_regress.c (L1246-L1246)
Switching execl -> execlp is unjustified and introduces a footgun without any functional benefit. shellprog is SHELLPROG, which is always an absolute path ("/bin/sh" in meson.build, "$(SHELL)" in GNUmakefile). Because the name contains a slash, execlp performs no PATH search and behaves identically to execl for every shipped configuration -- so this is effectively a no-op churn that violates the minimal-diff discipline. Worse, if SHELLPROG ever became a bare name, execlp would resolve the shell from an untrusted PATH instead of a fixed location, a security regression (POLA/footgun). Unless there is a concrete reason (with a -hackers reference) to allow PATH resolution here, revert to execl. [high confidence]
💡 Suggested change
Before:
execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
After:
execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
📄 src/test/benchmarks/data_generator.py (L352-L352)
The server-side UUID generator for RANDOM/CLUSTERED uses random(), which is nondeterministic per row and per run. This directly contradicts this module's stated purpose ("Reproducible seeded random data generation") and the reset()/self.seed design used by every other expression here. Since generate_server_side_insert is the wired data-loading path for both the traditional and stdatomic instances, the two builds get different UUID data and no run is reproducible, undermining benchmark comparability. Use a seeded, deterministic hash of g instead, e.g. md5(g::text || '{seed_val}')::uuid, matching the deterministic pattern used for TEXT/INT/etc.
💡 Suggested change
Before:
return f"md5(g::text || '{seed_val}' || random()::text)::uuid"
After:
return f"md5(g::text || '{seed_val}')::uuid"
📄 src/test/benchmarks/data_generator.py (L155-L157)
This HIGH_NULL guard keys off the column type (BIGINT) to protect the id column, but that is incorrect: in atomics_config.py the WIDE_SCHEMA defines genuine non-id BIGINT data columns (col_bigint_1..col_bigint_5). As written, all of those data columns are silently exempted from NULLing under HIGH_NULL, so the distribution is not what's intended. Additionally, in the client path id is set from the row index in generate_insert_sql/generate_copy_data and never reaches _gen_value, so the BIGINT exemption is unnecessary for protecting id here. Key the exemption off the column name (col_name == "id") instead of the type. Note _gen_value does not receive col_name, so the signature must be threaded through. The same type-based guard exists in _server_side_expr (line 309) and should be fixed consistently.
📄 src/test/benchmarks/data_generator.py (L128-L129)
import json is placed inside the function body rather than in the module-level import block (alongside hashlib, uuid, etc.), inconsistent with the rest of the file. Move it to the top of the module.
📄 src/test/benchmarks/data_generator.py (L13-L13)
Optional is imported but never used in this module. Remove it to keep the import list clean.
💡 Suggested change
Before:
from typing import Any, List, Optional
After:
from typing import Any, List
📄 src/test/regress/regress.c (L653-L654)
Comment churn: this added sentence largely restates the preceding "spinlock operations don't perform too wide writes" and hardcodes an implementation detail ("pg_atomic_uint32 (4 bytes)") that will silently drift if slock_t's backing type changes. The original comment already explained the intent. For a minimal diff, consider dropping the added sentence (or keeping it out of this cleanup). Confidence: moderate.
💡 Suggested change
Before:
* spinlock operations don't perform too wide writes. slock_t is a
* pg_atomic_uint32 (4 bytes); this test verifies no over-wide writes.
After:
* spinlock operations don't perform too wide writes.
📄 src/test/regress/regress.c (L672-L674)
This removes the TAS/TAS_SPIN check that verified a single spin cycle does not block when acquiring an already-held lock -- coverage of the fast-path acquire that the new implementation's SpinLockAcquire (which now spins via s_lock()) cannot reproduce with the public API. The removal is unavoidable now that TAS/TAS_SPIN are gone, but the equivalent behavior (pg_atomic_exchange_u32() returning non-zero on a held lock) is left untested. Consider adding a direct pg_atomic_exchange_u32() check to preserve this regression coverage. Confidence: moderate.
📄 src/tools/pgindent/pgindent (L1-L1)
This changes the shebang from #!/usr/bin/perl to #!/usr/bin/env perl, but every other Perl script in the tree (genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, and ~30 more) uses #!/usr/bin/perl. Introducing the lone env perl deviation here breaks tree-wide consistency for no stated reason. If the goal is truly to switch to env perl, it must be done tree-wide and discussed on -hackers; a single-file change like this is unrelated churn that will draw a rejection. Revert to #!/usr/bin/perl. (high confidence)
💡 Suggested change
Before:
#!/usr/bin/env perl
After:
#!/usr/bin/perl
| source_up | ||
| watch_file flake.nix | ||
| use flake | ||
| use project_steering postgresql | ||
| use project_mcp postgresq | ||
| use aws |
There was a problem hiding this comment.
This entire file must not be part of the patch. .envrc is a personal direnv configuration tied to the author's local workflow (Nix flake.nix, project_steering/project_mcp, aws, libumem LD_PRELOAD, coverage flags). The repository's own root .gitignore states the policy explicitly: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." Committing it violates the minimal-diff rule (a top rejection reason on pgsql-hackers), is unrelated to the stated change, and leaks environment-specific details. Remove .envrc from the change and ignore it locally instead. (high confidence)
Additionally, line 5 has a typo: use project_mcp postgresq (missing trailing l), which shows this is untested personal scaffolding rather than intended contribution content.
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This .clangd file is personal editor/LSP tooling configuration and does not belong in a PostgreSQL patch. It is unrelated to any functional change and will draw an immediate rejection on pgsql-hackers as unrelated churn to the repo root. A few concrete problems reinforce this:
CompilationDatabase: build/and the hardcoded-I../../../../src/includeare specific to one contributor's local out-of-tree build layout and are meaningless for anyone else.- The
Add:list duplicates the project's real warning/compile flags (managed by configure/meson). Keeping a second, hand-maintained copy here guarantees drift from the authoritative build config. - Ironically, the companion
.local-gitignoreadded in this same change listsbuild/as a local-only artifact, which confirms this clangd config points at a directory that is itself supposed to be untracked.
This (along with .envrc, .gdbinit, .local-gitignore, and pg-aliases.sh) should be kept local to the developer's machine (e.g. via a personal global gitignore / core.excludesFile) rather than committed. Please drop it from the patch. (high confidence)
| # Local development ignores (not tracked in .gitignore) | ||
| # To enable: git config core.excludesFile .local-gitignore | ||
| .local-gitignore |
There was a problem hiding this comment.
This file must not be part of the patch. .local-gitignore is a personal developer-environment artifact (it is meant to be wired up via git config core.excludesFile, i.e. purely local). Committing it into the tree is exactly the kind of unrelated, non-minimal change that gets rejected on pgsql-hackers: it has nothing to do with the atomics/spinlock work this PR actually contains. A per-developer excludes file belongs in $GIT_DIR/info/exclude or a global excludesfile, not tracked in the repo. Drop this file (and the sibling .clangd, .envrc, .gdbinit, pg-aliases.sh artifacts) from the patch. Confidence: high.
| # Local development ignores (not tracked in .gitignore) | ||
| # To enable: git config core.excludesFile .local-gitignore | ||
| .local-gitignore | ||
| CA+hUKGKFvu3zyvv3aaj5hHs9VtWcjFAmisOwOc7aOZNc5AF3NA@mail.gmail.com |
There was a problem hiding this comment.
This line is a Message-Id (a stray email filename, ...@mail.gmail.com) accidentally committed into the ignore list. It is meaningless to any other developer and leaks a personal artifact. Beyond confirming this file shouldn't exist at all, this specific entry is clearly accidental cruft and must be removed. Confidence: high.
| # HOT Indexed Updates — GDB breakpoints for code review | ||
| # | ||
| # Usage: gdb -x .gdbinit <postgres-binary> |
There was a problem hiding this comment.
This entire .gdbinit file must not be part of a PostgreSQL patch. Two independent, blocking problems:
-
Developer-private artifact / minimal-diff violation. This is a personal debugging aid with no bearing on any feature code. It is bundled with other dev-only files (
.clangd,.envrc,.local-gitignore,pg-aliases.sh) that the accompanying.local-gitignoreitself says are "not tracked in .gitignore". Unrelated scaffolding is the top rejection reason on -hackers. It is also a footgun: GDB auto-sources.gdbinitfrom the working directory, silently installing breakpoints for any developer who runs gdb in the tree. Move it to a personal ignore list instead of tracking it. -
The referenced feature does not exist in this change set, so the file is broken/fabricated. I verified against the tree:
HEAP_INDEXED_UPDATEDandXLOG_HEAP2_INDEXED_UPDATE: no matches anywhere.heap_hot_indexed_tuple_size,heap_hot_indexed_create_tuple(and allheap_hot_indexed_*helpers): no matches.heap_xlog_indexed_update: no matches;heapam_xlog.conly replaysXLOG_HEAP_UPDATE/XLOG_HEAP_HOT_UPDATE.log_heap_updatesetsinfo = XLOG_HEAP_UPDATE(heapam.c:8802) and is called at heapam.c:4097 -- so the hard-coded line breakpoints (heapam.c:4019/4024/4033/4101/4147, etc.) and their "HOT indexed" semantic comments do not match the actual code.
Every
break <symbol>on a non-existent function will fail to resolve, and everybreak <file>:<line>is fragile line-number scaffolding that silently drifts. The descriptive comments ("pure HOT", "redirect-with-data", "embedded modified-column bitmap") describe behavior not present in the tree and cannot be verified.
Recommendation: drop this file (and the sibling dev-only files) from the patch entirely.
| * With the stdatomic.h implementation there is no longer any restriction on | ||
| * including atomics.h from frontend code, since stdatomic.h is a standard | ||
| * header with no backend-only dependencies. |
There was a problem hiding this comment.
Removing the FRONTEND guard is a speculative behavior change: no frontend consumer in the tree needs atomics.h, so this widens the API surface for a path that isn't wired up (YAGNI). More importantly, the guard was a deliberate fail-fast contract; dropping it means a future accidental frontend include (or a later addition to atomics.h / stdatomic_impl.h that pulls in a backend-only symbol) will fail with an obscure error instead of the clear original message. Unless a frontend caller is actually being added in this series, keep the guard.
| * hard build requirement (USE_STDATOMIC_H is always defined); the former | ||
| * platform-specific implementations (arch-*.h, generic-*.h, fallback.h) | ||
| * have been removed. |
There was a problem hiding this comment.
This header comment narrates migration history ("the former platform-specific implementations ... have been removed") which is transient information that belongs in the commit message, not a permanent file header. Per PostgreSQL comment style, after the patch the code should read as if it had always been written this way. Trim the "have been removed" / arch-*.h enumeration and just describe the current design.
| /* | ||
| * Unknown compiler: no-op with compiler barrier | ||
| * | ||
| * At minimum, we need to prevent the compiler from optimizing away the | ||
| * spin loop. | ||
| */ | ||
| (void) 0; |
There was a problem hiding this comment.
The comment promises a compiler barrier ("no-op with compiler barrier" / "prevent the compiler from optimizing away the spin loop"), but (void) 0; emits nothing at all -- no barrier. Contrast the two unknown-architecture fallbacks in this same function, which do emit real barriers: the GCC arm uses __asm__ __volatile__("":::"memory") and the MSVC arm uses _ReadWriteBarrier(). On a genuinely unknown compiler this path provides neither a delay nor the promised barrier, so the comment is factually wrong. Either drop the barrier claim from the comment, or (safer, matching the intent) emit an actual compiler barrier here.
| #if defined(__i386__) || defined(__i386) | ||
| /* x86 32-bit: PAUSE instruction (encoded as rep nop) */ | ||
| __asm__ __volatile__(" rep; nop \n"); | ||
| #elif defined(__x86_64__) | ||
| /* x86-64: PAUSE instruction */ | ||
| __asm__ __volatile__(" rep; nop \n"); |
There was a problem hiding this comment.
These two arms are identical (both emit rep; nop), so splitting __i386__/__i386 and __x86_64__ into separate #if/#elif branches adds no value and just duplicates the body. Fold them into a single condition to keep the mapping obvious and avoid future drift between the two copies.
| #if defined(__i386__) || defined(__i386) | |
| /* x86 32-bit: PAUSE instruction (encoded as rep nop) */ | |
| __asm__ __volatile__(" rep; nop \n"); | |
| #elif defined(__x86_64__) | |
| /* x86-64: PAUSE instruction */ | |
| __asm__ __volatile__(" rep; nop \n"); | |
| #if defined(__i386__) || defined(__i386) || defined(__x86_64__) | |
| /* x86 / x86-64: PAUSE instruction (encoded as rep nop) */ | |
| __asm__ __volatile__(" rep; nop \n"); |
| * is frequently packed next to other fields, that word-wide RMW can clobber | ||
| * the neighbouring bytes, corrupting adjacent shared state under contention. |
There was a problem hiding this comment.
This rationale is inaccurate and will mislead future readers. It claims slock_t is a pg_atomic_flag and that a 32-bit backing width is needed to avoid RISC-V word-wide RMW clobbering neighbours in spinlock use. But spin.h in this same change defines slock_t as pg_atomic_uint32 and its comment explicitly states it does NOT use pg_atomic_flag. So the spinlock justification here does not apply. If the real reason to keep the flag 32-bit is matching generic.h, say only that; drop the incorrect slock_t/spinlock reasoning.
There was a problem hiding this comment.
🔍 OCR found 45 issue(s).
- 25 inline, 20 in summary (inline capped at 25)
📄 src/test/benchmarks/atomics_analyzer.py (L151-L152)
generate_report crashes on an empty results file. When self.comparisons is empty, analyze_overall() returns {} (see the if not self.comparisons: return {} guard), but generate_report immediately indexes overall['total_scenarios'], raising an opaque KeyError. An empty or results-less results.json (a realistic partial/failed-run case) should produce a graceful "no data" message. Add an early check, e.g. if not overall: return "No benchmark results to analyze." right after computing overall.
💡 Suggested change
Before:
overall = self.analyze_overall()
lines.append("OVERALL PERFORMANCE")
After:
overall = self.analyze_overall()
if not overall:
return "No benchmark results to analyze."
lines.append("OVERALL PERFORMANCE")
📄 src/test/benchmarks/atomics_analyzer.py (L43-L45)
Potential ZeroDivisionError: if any result has traditional_ms == 0 this expression crashes at load time with no diagnostic. A malformed/partial results file (or a degenerate near-zero timing) would take down the whole analyzer. Guard the divisor, e.g. skip or treat as 0.0 when traditional_ms is 0. (Moderate confidence: wall-clock timings from the harness are normally non-zero, but nothing here enforces that.)
💡 Suggested change
Before:
diff_pct = (
(r["traditional_ms"] - r["stdatomic_ms"]) / r["traditional_ms"] * 100
)
After:
trad = r["traditional_ms"]
diff_pct = (
(trad - r["stdatomic_ms"]) / trad * 100 if trad else 0.0
)
📄 src/test/benchmarks/atomics_analyzer.py (L13-L13)
Tuple is imported but never used anywhere in the module (dead import; would be flagged by linters/pyflakes). Drop it.
💡 Suggested change
Before:
from typing import Dict, List, Optional, Tuple
After:
from typing import Dict, List, Optional
📄 src/test/benchmarks/atomics_benchmark.py (L341-L341)
ZeroDivisionError risk. std_result.elapsed_seconds can legitimately be 0.0: in atomics_workload._run_single, every failed measurement appends elapsed = 0.0, and the reported elapsed_seconds is the median of the timings, so an all-failing (or timer-rounded) pattern yields 0.0. This division then raises ZeroDivisionError, which is only caught at the run_full_suite level, silently dropping the entire scenario and producing incomplete results with no clear indication. Guard against a zero (or non-positive) denominator before dividing.
💡 Suggested change
Before:
speedup = trad_result.elapsed_seconds / std_result.elapsed_seconds
After:
if std_result.elapsed_seconds <= 0.0:
logger.warning(
"Skipping speedup for pattern %s: stdatomic elapsed time is zero",
pattern,
)
continue
speedup = trad_result.elapsed_seconds / std_result.elapsed_seconds
📄 src/test/benchmarks/atomics_benchmark.py (L183-L189)
Pipe-buffer deadlock. The postgres backend is spawned with stdout=PIPE/stderr=PIPE, but these pipes are never drained (no reader thread, no communicate(); stop() only calls terminate()/wait()). With logging_collector=off, server log goes to stderr; once the OS pipe buffer (~64KB) fills, the postgres process blocks on write and the benchmark hangs indefinitely. Either redirect these streams to a file/DEVNULL or drain them in a background reader.
💡 Suggested change
Before:
self.process = subprocess.Popen(
cmd,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
env=env,
)
After:
self.process = subprocess.Popen(
cmd,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
text=True,
env=env,
)
📄 src/test/benchmarks/atomics_benchmark.py (L15-L16)
Unused imports: field (from dataclasses), datetime, Path, shutil, and signal are imported but never referenced in this file. Dead imports add noise; per PostgreSQL patch-hygiene the diff should be minimal. Remove them.
📄 src/test/benchmarks/atomics_benchmark.py (L500-L500)
median_speedup here is the upper-middle element for even-length lists rather than the average of the two middle values, so the reported "median" is slightly biased. Prefer statistics.median, which handles both parities correctly.
💡 Suggested change
Before:
"median_speedup": sorted(speedups)[len(speedups) // 2],
After:
"median_speedup": statistics.median(speedups),
📄 src/include/storage/spin.h (L108-L109)
Confirmed correct: pg_write_barrier_impl() is atomic_thread_fence(memory_order_release) plus a compiler barrier, and a C11 release fence immediately preceding an atomic store to the same object gives that store release semantics that order all prior loads and stores ahead of it. So the release side is sound (this is not the load-store-reordering hazard one might fear).
Minor: the comment is quite long and leans toward explaining what the code does. PG style prefers concise WHY-comments. Consider trimming to the rationale (why a release fence + relaxed store rather than a seq_cst store), e.g.:
/*
- Release ordering: prior critical-section accesses must complete before
- the unlock store is visible. pg_write_barrier() is a release fence, so
- the following relaxed store publishes the unlocked value with release
- semantics -- matching S_UNLOCK() while avoiding a seq_cst store (STLR on
- ARM64).
*/
📄 src/test/benchmarks/atomics_cli.py (L172-L177)
The --distribution all branch silently omits DataDistribution.HIGH_NULL, even though high_null is an explicit, selectable --distribution choice (and DataDistribution.HIGH_NULL exists in atomics_config.py). A user selecting all reasonably expects every listed distribution, so this is a POLA violation: high_null coverage is dropped without warning and contradicts the (default: all) help text. Confidence: high.
💡 Suggested change
Before:
if args.distribution == "all":
distributions = [
DataDistribution.RANDOM,
DataDistribution.CLUSTERED,
DataDistribution.LOW_CARDINALITY,
]
After:
if args.distribution == "all":
distributions = [
DataDistribution.RANDOM,
DataDistribution.CLUSTERED,
DataDistribution.LOW_CARDINALITY,
DataDistribution.HIGH_NULL,
]
📄 src/test/benchmarks/atomics_config.py (L1-L2)
Patch-hygiene / scope (blocking for -hackers): this file is part of a full Python benchmark harness (atomics_config/benchmark/analyzer/cli/workload, data_generator) that, per the changeset, is being committed alongside a core atomics change plus unrelated developer tooling (.clangd, .envrc, .gdbinit, pg-aliases.sh, .local-gitignore). The .gdbinit in the same commit describes an unrelated "HOT indexed updates patch series," and .local-gitignore lists a mail Message-Id file and NOTES -- strong evidence this is an accidental commit of a personal working tree. A performance-oriented, unwired Python harness does not belong committed to the tree: performance claims should be backed by a reproducible benchmark posted to the thread, not carried as an in-tree module under src/test/benchmarks with no meson/Makefile wiring. Split this out of the atomics patch and drop the personal dotfiles.
📄 src/test/benchmarks/atomics_config.py (L256-L261)
Non-portable hardcoded defaults. The /tmp/pgdata-* data directories are not portable to Windows (an officially supported platform) and collide across concurrent runs or multiple users on a shared host, causing initdb/data-dir conflicts and non-deterministic failures. The fixed ports 5433/5434 (and PostgresInstance's default 5432) are the same footgun. If this harness is meant to run anywhere but one developer's box, derive the data dir from tempfile.mkdtemp()/an explicit output root and pick a free port rather than hardcoding.
📄 src/test/benchmarks/atomics_workload.py (L236-L239)
A failed measurement appends elapsed = 0.0 to timings, so the median is computed over a mix of real timings and zeros — silently skewing the benchmark result toward 0 instead of discarding the run or marking the result invalid. Failed iterations should be excluded from timings (and the result flagged if no valid samples remain).
📄 src/test/benchmarks/atomics_workload.py (L242-L243)
timings[len(timings) // 2] raises IndexError when timings is empty. measure_iterations is user-configurable via the CLI --iterations (see atomics_config.py / atomics_cli.py), so a value of 0 makes this crash. Guard against an empty timings list before indexing.
📄 src/test/benchmarks/atomics_workload.py (L281-L281)
WorkloadResult.implementation is initialized to "" with the comment "Set by caller", but the caller (_run_on_instance/run_benchmark in atomics_benchmark.py) never assigns result.implementation before or after run_workload. As a result every QueryResult.implementation is the empty string, so the field that is meant to distinguish "traditional" vs "stdatomic" carries no information. Either populate it (e.g. pass impl_name into run_workload) or drop the field.
📄 src/test/regress/pg_regress.c (L1246-L1246)
This execl -> execlp change is a no-op for every shipped configuration and is unrelated to this patch's stated purpose (the atomics/spinlock refactor).
shellprog is set from SHELLPROG, which is "/bin/sh" in meson.build and $(SHELL) in GNUmakefile/ecpg's Makefile -- both absolute paths. execlp() only performs a PATH search when the program argument contains no slash; when it contains a slash (like /bin/sh) it behaves identically to execl(). So this line changes nothing today.
Two problems:
- Minimal-diff violation: this is unrelated churn bundled into a spinlock/atomics series. On -hackers this should be dropped from the patch or, if genuinely wanted, split into its own commit with a stated rationale.
- Footgun: switching to
execlpsilently introduces PATH-based resolution of the shell. IfSHELLPROGever becomes a bare name, the test harness would resolveshvia$PATH, a behavior/security change with no corresponding safeguard.
Recommend reverting this hunk unless there is a specific, documented reason (and a -hackers thread) for PATH-searching the shell here.
Confidence: high.
💡 Suggested change
Before:
execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
After:
execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
📄 src/test/benchmarks/data_generator.py (L322-L322)
abs(hashint4(...)) will raise ERROR: integer out of range at runtime. hashint4() returns an int4 in [-2147483648, 2147483647], and abs((-2147483648)::int4) errors because +2147483648 is not representable in int4. Since the generated SQL is executed raw over a full generate_series (up to 10M rows), any row where the hash equals INT_MIN aborts the entire data load. This pattern is repeated across the INT/TEXT/BOOLEAN/TIMESTAMP/FLOAT/NUMERIC low-cardinality branches, the JSONB branches, and the HIGH_NULL CASE below. Use hashint4(...) & 2147483647 (mask off the sign bit) or mod(hashint4(...)::bigint, N) instead of abs(hashint4(...)). [high confidence]
💡 Suggested change
Before:
return f"(ARRAY[{codes}])[1 + abs(hashint4(g + {seed_val})) % {len(LOW_CARD_STATUS_CODES)}]"
After:
return f"(ARRAY[{codes}])[1 + (hashint4(g + {seed_val}) & 2147483647) % {len(LOW_CARD_STATUS_CODES)}]"
📄 src/test/benchmarks/data_generator.py (L352-L352)
The server-side UUID expression injects the non-seeded SQL random() function, so this data is not reproducible across runs. This contradicts the module docstring ("Reproducible seeded random data generation") and diverges from the client-side _gen_uuid, which is deterministic. It also makes benchmark comparisons between the traditional and stdatomic instances load different UUID data. Derive the value deterministically from g and the seed, e.g. md5(g::text || '{seed_val}')::uuid. [high confidence]
💡 Suggested change
Before:
return f"md5(g::text || '{seed_val}' || random()::text)::uuid"
After:
return f"md5(g::text || '{seed_val}')::uuid"
📄 src/test/benchmarks/data_generator.py (L128-L129)
Minor: import json is done locally inside this method while the other stdlib imports (hashlib, random, uuid) are at module top. Move it to the top-level import block for consistency. [low confidence, non-blocking]
📄 src/test/regress/regress.c (L653-L654)
Minor comment churn: the second sentence largely restates the immediately preceding "the spinlock operations don't perform too wide writes." The added "; this test verifies no over-wide writes" is redundant, and hardcoding "slock_t is a pg_atomic_uint32 (4 bytes)" couples the comment to the current implementation (the struct already uses 4-byte data_before/data_after arrays, which is what actually pins the size). Consider keeping the diff minimal by dropping the redundant clause, e.g. just note the 4-byte padding on either side. Low confidence / low severity — style nit, not correctness.
📄 src/tools/pgindent/pgindent (L1-L1)
This shebang change makes pgindent inconsistent with the rest of the tree: every other Perl script in PostgreSQL (30+ files, e.g. src/backend/catalog/genbki.pl, src/tools/copyright.pl, src/tools/mark_pgdllimport.pl) uses #!/usr/bin/perl. This lone switch to #!/usr/bin/env perl breaks that convention.
It is also unrelated to the rest of this change set (which is all atomics/spin-delay infrastructure), so it constitutes unrelated diff churn. If a tree-wide switch to env perl is genuinely desired, it should be a separate, dedicated patch discussed on -hackers that touches all scripts consistently. Otherwise, revert this line. (high confidence)
💡 Suggested change
Before:
#!/usr/bin/env perl
After:
#!/usr/bin/perl
| source_up | ||
| watch_file flake.nix | ||
| use flake | ||
| use project_steering postgresql | ||
| use project_mcp postgresq | ||
| use aws |
There was a problem hiding this comment.
This entire file is a personal direnv configuration and must not be part of the patch. It leaks the author's local dev setup (use flake, project_steering, project_mcp, use aws, libumem/LD_PRELOAD notes, coverage/sanitizer flags) into a change that goes to pgsql-hackers. It is unrelated to the stated purpose, breaks the minimal-diff rule, and would cause needless conflicts and confusion for every reviewer and committer. Remove .envrc from the patch entirely (add it to your personal/global gitignore instead of committing it). High confidence.
| Diagnostics: | ||
| MissingIncludes: None |
There was a problem hiding this comment.
This entire .clangd file should not be committed. It is a personal editor/LSP configuration, exactly the kind of "auxiliary file from local workflows, your preferred editor, etc." that the repository's own .gitignore header (lines 1-3) instructs contributors to exclude locally via $GIT_DIR/info/exclude or ~/.gitexclude. A PostgreSQL patch destined for -hackers/commitfest must be a minimal diff limited to the stated change; committing IDE tooling config pollutes the tree and will be rejected. Beyond policy, the contents are machine-specific and non-portable: CompilationDatabase: build/ and -I../../../../src/include assume one contributor's out-of-tree build layout and relative CWD, and -DDEBUG/-DLOCAL are not macros defined by PostgreSQL's build system. Please drop this file from the patch and gitignore it locally instead.
| # Local development ignores (not tracked in .gitignore) | ||
| # To enable: git config core.excludesFile .local-gitignore | ||
| .local-gitignore |
There was a problem hiding this comment.
This file must not be part of the patch. .local-gitignore is a personal developer-environment artifact: it references a specific mailing-list Message-Id filename, private notes, and local build/install directories (build-valgrind/, install-asan/, .direnv/, etc.). None of this is required by the stated change and it will draw immediate rejection on pgsql-hackers as unrelated churn. PostgreSQL's convention is to add local-only ignore patterns to .git/info/exclude (untracked) rather than committing a personal excludes file. Drop this file from the patch entirely.
| @@ -0,0 +1,156 @@ | |||
| # HOT Indexed Updates — GDB breakpoints for code review | |||
There was a problem hiding this comment.
This entire file is a developer-local debugging artifact and must not be committed to a PostgreSQL patch. A top-level .gdbinit (bundled here with .clangd, .envrc, .local-gitignore, pg-aliases.sh, and benchmark scripts) is unrelated to the feature implementation and is an automatic rejection reason on pgsql-hackers under the minimal-diff / "does more than one thing" rule. It pollutes the tree, causes merge conflicts, and leaks the author's local workflow. Remove it from the patch (keep it local, and add it to a personal, untracked ignore instead).
| break heapam.c:4019 | ||
| break heapam.c:4024 | ||
| break heapam.c:4033 |
There was a problem hiding this comment.
Breakpoints keyed to hard-coded line numbers (heapam.c:3210/4019/4024/4033/4101/4147, heapam_indexscan.c:182/250/297, indexam.c:299, execIndexing.c:370, pruneheap.c:1802/1836/1863/1287/2936) are inherently fragile: inserting or removing any unrelated line above them silently shifts them to the wrong statement, and they cannot be validated in review. Combined with the fact that the underlying feature code is absent from the tree, these line targets are unverifiable.
| StaticAssertDecl(ATOMIC_CHAR_LOCK_FREE == 2, "8-bit atomics are not lock-free"); | ||
| StaticAssertDecl(ATOMIC_SHORT_LOCK_FREE == 2, "16-bit atomics are not lock-free"); |
There was a problem hiding this comment.
These 8-bit/16-bit lock-free assertions are not satisfiable on all supported platforms. PostgreSQL targets RISC-V, whose base 'A' extension has only word/doubleword AMOs; a conforming stdatomic.h there can report ATOMIC_CHAR_LOCK_FREE/ATOMIC_SHORT_LOCK_FREE < 2, making this a hard compile error. Since no code instantiates pg_atomic_uint8/uint16 RMW operations, these two assertions guard nothing and should be removed (keep only the 32/64-bit assertions that back types actually used across processes).
| StaticAssertDecl(ATOMIC_CHAR_LOCK_FREE == 2, "8-bit atomics are not lock-free"); | |
| StaticAssertDecl(ATOMIC_SHORT_LOCK_FREE == 2, "16-bit atomics are not lock-free"); | |
| StaticAssertDecl(ATOMIC_INT_LOCK_FREE == 2, "32-bit atomics are not lock-free"); | |
| StaticAssertDecl(ATOMIC_LLONG_LOCK_FREE == 2, "64-bit atomics are not lock-free"); |
| * So the ordered read is kept for correctness while the write, which was the | ||
| * actual measured cost, is not paid. | ||
| * | ||
| * A cleaner long-term fix is to leave this primitive relaxed (matching the |
There was a problem hiding this comment.
Aspirational / mailing-list-style commentary does not belong in in-tree source. Per project comment conventions, comments describe what the code does now and explain why concisely; "A cleaner long-term fix ... is intentionally out of scope ... noted as follow-up work rather than folded in here" reads like a commit message / -hackers post and will draw review objections. Either fold the proper call-site fix into this patch (see the read-u32 seq_cst issue) or trim this to a short WHY, dropping the future-work narrative.
| #elif defined(__arm__) || defined(__arm) | ||
| /* ARM 32-bit: YIELD hint */ | ||
| __asm__ __volatile__(" yield; \n"); | ||
| #else |
There was a problem hiding this comment.
Portability/performance regression: this ladder handles only x86 (i386/x86_64) and ARM (aarch64/arm32) with a real CPU hint; PPC64, s390x, RISC-V, MIPS, etc. now fall through to a bare compiler barrier. The old (now-deleted) arch-ppc.h emitted a PPC-specific yield hint ("or 31,31,31") in the spin-delay path. Since PPC64 and s390x are supported buildfarm platforms, dropping their hint here is a behavioral change under contention that should be called out in the commit message (with a benchmark) or preserved. Confidence: moderate.
| /* | ||
| * Unknown compiler: no-op with compiler barrier | ||
| * | ||
| * At minimum, we need to prevent the compiler from optimizing away the | ||
| * spin loop. | ||
| */ | ||
| (void) 0; |
There was a problem hiding this comment.
Comment/code contradiction: this comment says "we need to prevent the compiler from optimizing away the spin loop," but (void) 0 emits no compiler barrier (unlike the GCC __asm__ __volatile__("":::"memory") and MSVC _ReadWriteBarrier() fallbacks). Either emit a real barrier here or fix the comment so it does not claim a guarantee the code does not provide. In practice the s_lock.c caller (perform_spin_delay) has side effects so the loop is not actually dead, but the comment is misleading. Confidence: high.
| /* | |
| * Unknown compiler: no-op with compiler barrier | |
| * | |
| * At minimum, we need to prevent the compiler from optimizing away the | |
| * spin loop. | |
| */ | |
| (void) 0; | |
| /* | |
| * Unknown compiler: plain no-op. We cannot emit a portable compiler | |
| * barrier here, but the spinlock slow path (perform_spin_delay) has | |
| * observable side effects, so the loop is not optimized away. | |
| */ | |
| (void) 0; |
| #if defined(__i386__) || defined(__i386) | ||
| /* x86 32-bit: PAUSE instruction (encoded as rep nop) */ | ||
| __asm__ __volatile__(" rep; nop \n"); | ||
| #elif defined(__x86_64__) | ||
| /* x86-64: PAUSE instruction */ | ||
| __asm__ __volatile__(" rep; nop \n"); |
There was a problem hiding this comment.
Minor: the __i386__/__i386 and __x86_64__ branches are identical (rep; nop) and can be merged into a single #if defined(__i386__) || defined(__i386) || defined(__x86_64__) to reduce duplication. Confidence: high (style only).
| #if defined(__i386__) || defined(__i386) | |
| /* x86 32-bit: PAUSE instruction (encoded as rep nop) */ | |
| __asm__ __volatile__(" rep; nop \n"); | |
| #elif defined(__x86_64__) | |
| /* x86-64: PAUSE instruction */ | |
| __asm__ __volatile__(" rep; nop \n"); | |
| #if defined(__i386__) || defined(__i386) || defined(__x86_64__) | |
| /* x86/x86-64: PAUSE instruction (encoded as rep nop) */ | |
| __asm__ __volatile__(" rep; nop \n"); |
github-actions
Bot
force-pushed
the
master
branch
from
3ed29c4 to
e495986
Compare
1 participant
This series reimplements PostgreSQL's atomics (port/atomics.h) on top of
C11 <stdatomic.h>, picking up Thomas Munro's WIP and finishing it into a
reviewable, benchmarked form.
It is deliberately structured as an alternative, selectable
implementation -- not a replacement:
auto|yes|no and DEFAULTS TO "no". With the default, the traditional
platform-specific atomics remain the implementation and are
behavior-preserving; stdatomic is strictly opt-in.
existing arch-/generic- code and wire it up behind the option.
Nothing is removed; both build systems keep building both ways.
endgame -- deleting the traditional code once stdatomic has proven
itself in the field -- purely so reviewers and the buildfarm can
exercise the stdatomic-only end state. It is NOT being proposed for
commit now, and the aggregate diffstat below includes its deletions.
So the answer to "is this making <stdatomic.h> mandatory?" is no:
through patch 5 the traditional path is intact, default, and selectable.
The one substantive semantics decision worth review is the memory
ordering of the generic read/write. Short version: the WRITE is relaxed
(plain STR, matching the documented "no barrier" contract), and the READ
is seq_cst -- which weak-memory correctness actually required (a C11
relaxed load is NOT equivalent to stock's volatile load; a parallel hash
join lost tuples on RISC-V with a relaxed read). Uncontended this costs
nothing measurable, and the earlier ~2% aarch64 regression (which was
the seq_cst STORE, now relaxed) is gone. Relaxing the read too and
giving just the hash-join consumer its ordering is documented follow-up.
Full data, per-op mapping, and methodology are in the accompanying
pgsql-hackers mail.
CI is green on all platforms (Linux 32/64 meson + autoconf, macOS,
Windows MSVC + MinGW, CompilerWarnings), Test world included.