Repository navigation
Trying out <stdatomic.h> for port/atomics.h (opt-in alternative) [cf-7005] #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
4da95d1
1f53069
c985a20
06b9ad9
2c1e097
962764d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,43 @@ | ||||||||||||||||||
| Diagnostics: | ||||||||||||||||||
| MissingIncludes: None | ||||||||||||||||||
|
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Committing
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
This (along with
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This adds a personal, editor-specific clangd config to the tracked tree. It is unrelated to the stated purpose of this change (the atomics / spin_delay work) and bundles developer-local preferences (InlayHints, -DLOCAL, -DDEBUG, a hardcoded
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file should not be part of the patch.
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Two independent reasons, both blocking:
Drop
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file should not be committed. It is a personal editor/IDE configuration for the clangd language server, i.e. exactly the kind of "auxiliary file from local workflows, your preferred editor, etc." that the tree's top-level Several details also confirm this is machine-/developer-specific and non-portable, reinforcing why it does not belong in the shared tree:
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire set of files (
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole change set (
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personal editor/LSP configuration committed to the repository root. This is unrelated to the atomics/spinlock refactor and is author-specific developer tooling that should not be part of the patch posted to pgsql-hackers.
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole file is developer-local editor/LSP config that is unrelated to the atomics/spinlock refactor this PR implements, and it does not belong in the PostgreSQL tree. Beyond that, the config is non-portable: |
||||||||||||||||||
| InlayHints: | ||||||||||||||||||
| Enabled: true | ||||||||||||||||||
| ParameterNames: true | ||||||||||||||||||
| DeducedTypes: true | ||||||||||||||||||
| CompileFlags: | ||||||||||||||||||
| CompilationDatabase: build/ # Search build/ directory for compile_commands.json | ||||||||||||||||||
|
Comment on lines
+7
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This .clangd is personal editor/LSP config unrelated to the atomics/spinlock change; it should not be committed to a PostgreSQL patch. Beyond scope, the hardcoded |
||||||||||||||||||
| Remove: [ -Werror ] | ||||||||||||||||||
| Add: | ||||||||||||||||||
| - -DDEBUG | ||||||||||||||||||
| - -DLOCAL | ||||||||||||||||||
| - -DPGDLLIMPORT= | ||||||||||||||||||
| - -DPIC | ||||||||||||||||||
| - -O2 | ||||||||||||||||||
| - -Wall | ||||||||||||||||||
| - -Wcast-function-type | ||||||||||||||||||
| - -Wconversion | ||||||||||||||||||
| - -Wdeclaration-after-statement | ||||||||||||||||||
| - -Wendif-labels | ||||||||||||||||||
| - -Werror=vla | ||||||||||||||||||
| - -Wextra | ||||||||||||||||||
| - -Wfloat-equal | ||||||||||||||||||
| - -Wformat-security | ||||||||||||||||||
| - -Wimplicit-fallthrough=3 | ||||||||||||||||||
| - -Wmissing-format-attribute | ||||||||||||||||||
| - -Wmissing-prototypes | ||||||||||||||||||
| - -Wno-format-truncation | ||||||||||||||||||
| - -Wno-sign-conversion | ||||||||||||||||||
| - -Wno-stringop-truncation | ||||||||||||||||||
| - -Wno-unused-const-variable | ||||||||||||||||||
| - -Wpointer-arith | ||||||||||||||||||
| - -Wshadow | ||||||||||||||||||
| - -Wshadow=compatible-local | ||||||||||||||||||
| - -fPIC | ||||||||||||||||||
| - -fexcess-precision=standard | ||||||||||||||||||
| - -fno-strict-aliasing | ||||||||||||||||||
| - -fvisibility=hidden | ||||||||||||||||||
| - -fwrapv | ||||||||||||||||||
| - -g | ||||||||||||||||||
| - -std=c11 | ||||||||||||||||||
| - -I. | ||||||||||||||||||
| - -I../../../../src/include | ||||||||||||||||||
|
Comment on lines
+42
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The include path
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The relative include
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hardcoded, location-specific relative include path.
Comment on lines
+42
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,10 @@ | ||||||||||||||||||||||
| # Inherit shared CFLAGS from ~/ws/postgres/.envrc, load this worktree's | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is personal developer-environment scaffolding and leaks an author-specific absolute path ( |
||||||||||||||||||||||
| # flake, then layer the postgres agent context (helpers need flake's PATH + | ||||||||||||||||||||||
| # write into this worktree's dir). | ||||||||||||||||||||||
| source_up | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file cannot work for anyone but the author: it |
||||||||||||||||||||||
| watch_file flake.nix | ||||||||||||||||||||||
| use flake | ||||||||||||||||||||||
|
Comment on lines
+4
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
Comment on lines
+4
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file must not be part of the patch (high confidence).
Comment on lines
+4
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This direnv file sources a hard-coded personal path (
Comment on lines
+4
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This direnv file is developer-local environment scaffolding unrelated to the atomics/spinlock change under review, and it must not be committed to the PostgreSQL tree. It |
||||||||||||||||||||||
| use project_steering c aws | ||||||||||||||||||||||
| use project_steering postgresql | ||||||||||||||||||||||
|
Comment on lines
+4
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file must not be committed.
Comment on lines
+4
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file (and its companions flake.nix, shell.nix, plus pg-aliases.sh, .gdbinit, .clangd, .local-gitignore, glibc-no-fortify-warning.patch) is personal developer-environment scaffolding. It is unrelated to the substantive change in this PR (the C11 stdatomic.h atomics/spinlock refactor in atomics.h, spin.h, s_lock.c, configure). Bundling local Nix/direnv tooling into a patch destined for pgsql-hackers/commitfest violates the minimal-diff discipline and is a near-certain rejection reason. Notably, .local-gitignore does NOT list .envrc/flake.nix/shell.nix, so these are being committed to the tree rather than kept local. Split all of this out of the patch and keep it in your local environment only. [high confidence] |
||||||||||||||||||||||
| use project_mcp postgresq | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in the environment name:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in the directory/config name: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in the direnv directive argument: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo/inconsistency: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in the flake namespace:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in the direnv
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in the direnv directive:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is non-portable author-local config. It depends on an ancestor ~/ws/postgres/.envrc (source_up) and user-global direnv library functions ( |
||||||||||||||||||||||
| use project_mcp context7 | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,156 @@ | ||||||||||||||||||
| # HOT Indexed Updates — GDB breakpoints for code review | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file is a personal debugging artifact and should not be committed. Beyond violating the minimal-diff discipline (a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-ASCII em-dash (U+2014) characters are used throughout the comment block (e.g. "HOT Indexed Updates —", "UPDATE DECISION —"). The project standard requires ASCII only in source and diffs. Replace em-dashes with an ASCII hyphen/dash.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file is a developer-local debugging artifact and must not be committed to a PostgreSQL patch. A top-level There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire
Recommendation: remove this file from the patch entirely. (high confidence) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-ASCII character: the em-dash (
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire
Remove this file (and the sibling scaffolding) from the patch; if a debugging aid is wanted, keep it out of tree (e.g. via a local/global gitignore, which There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file should be dropped from the patch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The header comment uses non-ASCII characters (em-dash in 'Updates — GDB', 'HOT/HOT-indexed/non-HOT choice' line, and 'enable/disable' notes below). PostgreSQL source and diffs must be ASCII only — no smart quotes, em-dashes, or ellipsis characters. These em-dashes (U+2014) appear in multiple section headers throughout the file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file is a developer-local debugging artifact and does not belong in a PostgreSQL patch. It is one of several workspace-local files added in this same change set ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-ASCII em-dash (U+2014) used here and on 9 other lines in this file. PostgreSQL source and diffs must be ASCII-only (no em-dashes, smart quotes, or ellipsis characters); use a plain
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment header (and every section banner below) uses non-ASCII em-dash characters (U+2014). PostgreSQL source and diffs must be ASCII only -- no smart quotes, em-dashes, or ellipsis characters. Even if a debugging aid were wanted in-tree, this alone would fail hygiene checks. Replace all em-dashes with ASCII hyphens. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file is scaffolding for an unrelated "HOT indexed updates" feature and does not belong in this change set, which is an atomics/spinlock refactor (migration of port/atomics to C11 <stdatomic.h>, see the diffs in atomics.h and spin.h). Two concrete problems make this verifiably stale/wrong:
For a patch destined for pgsql-hackers/commitfest, personal debugger scaffolding referencing an unrelated feature should be removed from the patch entirely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole file is developer-local scaffolding that must not be committed to the PostgreSQL tree, and it is also out of scope for this PR. The actual code change under review is the atomics/spinlock refactor (stdatomic.h-based |
||||||||||||||||||
| # | ||||||||||||||||||
| # Usage: gdb -x .gdbinit <postgres-binary> | ||||||||||||||||||
|
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire
Recommendation: drop this file (and the sibling dev-only files) from the patch entirely.
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire file is a personal developer debugging artifact and must not be committed to a PostgreSQL patch. It violates the minimal-diff rule (the top rejection reason on pgsql-hackers) since it is unrelated to any functional change. It is also a footgun: modern GDB auto-loads |
||||||||||||||||||
| # Or from gdb: source .gdbinit | ||||||||||||||||||
| # | ||||||||||||||||||
| # These breakpoints cover the major code paths introduced or modified by | ||||||||||||||||||
| # the HOT indexed updates patch series. They are organized by subsystem | ||||||||||||||||||
| # to make it easy to enable/disable groups during debugging. | ||||||||||||||||||
| # | ||||||||||||||||||
| # Tip: to skip to a specific subsystem, disable all then enable selectively: | ||||||||||||||||||
| # disable breakpoints | ||||||||||||||||||
| # enable 1 2 3 # just the update-decision group | ||||||||||||||||||
|
|
||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
| # 1. UPDATE DECISION — heap_update() HOT/HOT-indexed/non-HOT choice | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-ASCII em-dash characters (U+2014) appear in comment lines throughout the file (e.g. "HOT Indexed Updates — GDB breakpoints", "UPDATE DECISION — heap_update()"). This violates PostgreSQL's ASCII-only rule for source and diffs. Use a plain hyphen/dash instead.
Suggested change
|
||||||||||||||||||
| # src/backend/access/heap/heapam.c | ||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
|
|
||||||||||||||||||
| # Main entry: heap_update | ||||||||||||||||||
| break heapam.c:3210 | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hardcoded absolute line-number breakpoints (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 extremely brittle: any edit to those files shifts the lines and silently moves the breakpoint to unrelated code, producing misleading debug sessions. Function-name breakpoints (as used elsewhere in this same file) are the maintainable pattern. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The line-number breakpoints are already stale and point at the wrong statements. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hard-coded
Comment on lines
+19
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two independent problems make this file unfit for the tree:
This is a personal debugging file; remove it from the patch and keep it in a local
Comment on lines
+19
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This .gdbinit is dead/misleading scaffolding for a separate "HOT indexed updates" patch series that is NOT part of this change. I verified that the referenced symbols do not exist anywhere in this tree: heap_hot_indexed_tuple_size / heap_hot_indexed_create_tuple / heap_xlog_indexed_update, and the flags XLOG_HEAP2_INDEXED_UPDATE / HEAP_INDEXED_UPDATED all return no matches. The line-number breakpoints (heapam.c:3210/4019/4024/4033, indexam.c:299, pruneheap.c:1802/2936, heapam_indexscan.c:182/250/297) therefore point at code that either does not exist or has silently drifted. This file both pollutes the tree with an unrelated patch's debug harness and is evidence the working tree mixes multiple unrelated series; it should not ship in this patch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hardcoded absolute source line numbers (e.g.
Comment on lines
+19
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These breakpoints target a "HOT indexed updates patch series" that is not present in this tree, which contains the atomics/spinlock refactor. Verification against the current source confirms:
Sourcing this file will silently fail to resolve the symbol breakpoints and will bind the numeric ones to the wrong code, misleading anyone who uses it. Beyond that, this is author-local debugging scaffolding that does not belong in the committed tree (see .gitignore note below).
Comment on lines
+19
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file hard-codes line-number breakpoints ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Breakpoints are pinned to absolute source line numbers (heapam.c:3210, heapam.c:4019/4024/4033/4101/4147, heapam_indexscan.c:182/250/297, indexam.c:299, execIndexing.c:370, pruneheap.c:1802/1836/1863/1287/2936). These files are not part of this change, and line-number breakpoints silently point at the wrong code after any edit. This is brittle and confirms the file is stray scaffolding for a different feature branch. |
||||||||||||||||||
|
|
||||||||||||||||||
| # HOT decision block: pure HOT vs HOT indexed vs non-HOT | ||||||||||||||||||
| # Line 4019: pure HOT (no indexed columns changed) | ||||||||||||||||||
| # Line 4024: HOT indexed path (non-catalog, some indexed columns changed) | ||||||||||||||||||
| # Line 4031: predict augmented tuple size | ||||||||||||||||||
| # Line 4033: size+space check before creating augmented tuple | ||||||||||||||||||
| break heapam.c:4019 | ||||||||||||||||||
| break heapam.c:4024 | ||||||||||||||||||
| break heapam.c:4033 | ||||||||||||||||||
|
Comment on lines
+27
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Comment on lines
+27
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Breakpoints anchored to absolute source line numbers are extremely brittle: they silently drift to a different statement the moment any surrounding code changes, and the inline claims about what each line does ("Line 4019: pure HOT", "Line 4024: HOT indexed path", etc.) cannot stay accurate. Combined with finding #1 (none of these files even contain the referenced HOT-indexed logic in this tree), these line/comment pairs are stale/misleading on arrival. This applies to every
Comment on lines
+27
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Breakpoints on hard-coded absolute line numbers (
Comment on lines
+27
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These hard-coded file:line breakpoints are extremely fragile: they break the instant any line above them changes, and the inline comments assert specific behaviors (e.g. 'Line 4019: pure HOT', 'Line 4024: HOT indexed path') that cannot be verified and will drift immediately. Beyond that, the referenced symbols and files do not exist in this branch — a search for
Comment on lines
+27
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hardcoded |
||||||||||||||||||
|
|
||||||||||||||||||
| # Set HEAP_INDEXED_UPDATED flag on new tuple before page insertion | ||||||||||||||||||
| break heapam.c:4101 | ||||||||||||||||||
|
|
||||||||||||||||||
| # Restore HEAP_INDEXED_UPDATED on old tuple (only if it previously had it) | ||||||||||||||||||
| break heapam.c:4147 | ||||||||||||||||||
|
|
||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
| # 2. TUPLE CREATION — building the augmented tuple with embedded bitmap | ||||||||||||||||||
| # src/backend/access/heap/heapam.c | ||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
|
|
||||||||||||||||||
| # Predict augmented tuple size (returns 0 if t_hoff would overflow) | ||||||||||||||||||
| break heap_hot_indexed_tuple_size | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These breakpoints reference symbols and file:line locations for a "HOT indexed updates" feature that does not exist in this tree. I searched the whole codebase: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These breakpoints reference functions and WAL records that do not exist anywhere in this tree -- |
||||||||||||||||||
|
|
||||||||||||||||||
| # Create augmented tuple with embedded modified-column bitmap | ||||||||||||||||||
| break heap_hot_indexed_create_tuple | ||||||||||||||||||
|
|
||||||||||||||||||
| # Serialize Bitmapset into raw bytes in tuple header | ||||||||||||||||||
| break heap_hot_indexed_serialize_bitmap | ||||||||||||||||||
|
|
||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
| # 3. BITMAP UTILITIES — raw bitmap operations for chain following | ||||||||||||||||||
| # src/backend/access/heap/heapam.c | ||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
|
|
||||||||||||||||||
| # Compute raw bitmap byte size from natts | ||||||||||||||||||
| break heap_hot_indexed_bitmap_raw_size | ||||||||||||||||||
|
|
||||||||||||||||||
| # Check if tuple header has room for bitmap between null bitmap and data | ||||||||||||||||||
| break heap_hot_indexed_has_bitmap_space | ||||||||||||||||||
|
|
||||||||||||||||||
| # Read HOT indexed bitmap from tuple header (returns Bitmapset) | ||||||||||||||||||
| break heap_hot_indexed_read_bitmap | ||||||||||||||||||
|
|
||||||||||||||||||
| # Fast overlap check: does tuple's raw bitmap overlap with indexed_attrs? | ||||||||||||||||||
| break heap_hot_indexed_bitmap_overlaps_raw | ||||||||||||||||||
|
|
||||||||||||||||||
| # OR a tuple's raw bitmap into an accumulator buffer | ||||||||||||||||||
| break heap_hot_indexed_bitmap_or_raw | ||||||||||||||||||
|
|
||||||||||||||||||
| # Check if accumulated raw bitmap overlaps with indexed_attrs | ||||||||||||||||||
| break heap_hot_indexed_accum_overlaps | ||||||||||||||||||
|
|
||||||||||||||||||
| # Merge bitmaps from dead tuples into a target tuple on the page | ||||||||||||||||||
| break heap_hot_indexed_merge_bitmaps_raw | ||||||||||||||||||
|
|
||||||||||||||||||
| # Deserialize raw bytes back to Bitmapset | ||||||||||||||||||
| break heap_hot_indexed_deserialize_bitmap | ||||||||||||||||||
|
|
||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
| # 4. INDEX SCAN — HOT chain following with stale-entry detection | ||||||||||||||||||
| # src/backend/access/heap/heapam_indexscan.c | ||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
|
|
||||||||||||||||||
| # Main HOT chain search with indexed update awareness | ||||||||||||||||||
| break heap_hot_search_buffer | ||||||||||||||||||
|
|
||||||||||||||||||
| # Redirect-with-data: initialize bitmap accumulator from collapsed redirect | ||||||||||||||||||
| break heapam_indexscan.c:182 | ||||||||||||||||||
|
|
||||||||||||||||||
| # Accumulate bitmap from INDEXED_UPDATED tuple in chain | ||||||||||||||||||
| break heapam_indexscan.c:250 | ||||||||||||||||||
|
|
||||||||||||||||||
| # Stale entry detection: accumulated bitmap overlaps this index's attrs | ||||||||||||||||||
| break heapam_indexscan.c:297 | ||||||||||||||||||
|
|
||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
| # 5. INDEX SCAN SETUP — indexed_attrs bitmap computation | ||||||||||||||||||
| # src/backend/access/index/indexam.c | ||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
|
|
||||||||||||||||||
| # Compute indexed_attrs for HOT indexed update chain following | ||||||||||||||||||
| break indexam.c:299 | ||||||||||||||||||
|
|
||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
| # 6. INDEX INSERTION — skip unchanged indexes for HOT indexed updates | ||||||||||||||||||
| # src/backend/executor/execIndexing.c | ||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
|
|
||||||||||||||||||
| # Entry: insert/update index tuples | ||||||||||||||||||
| break ExecInsertIndexTuples | ||||||||||||||||||
|
|
||||||||||||||||||
| # Index skip decision: skip indexes whose attrs don't overlap modified set | ||||||||||||||||||
| break execIndexing.c:370 | ||||||||||||||||||
|
|
||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
| # 7. PRUNING — chain collapsing and redirect-with-data | ||||||||||||||||||
| # src/backend/access/heap/pruneheap.c | ||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
|
|
||||||||||||||||||
| # Main prune function | ||||||||||||||||||
| break heap_page_prune_and_freeze | ||||||||||||||||||
|
|
||||||||||||||||||
| # Per-chain pruning entry | ||||||||||||||||||
| break heap_prune_chain | ||||||||||||||||||
|
|
||||||||||||||||||
| # Chain collapsing: collect bitmaps from dead INDEXED_UPDATED intermediates | ||||||||||||||||||
| break pruneheap.c:1802 | ||||||||||||||||||
|
|
||||||||||||||||||
| # OR dead tuple bitmaps into combined bitmap | ||||||||||||||||||
| break pruneheap.c:1836 | ||||||||||||||||||
|
|
||||||||||||||||||
| # Record redirect-with-data for execute phase | ||||||||||||||||||
| break pruneheap.c:1863 | ||||||||||||||||||
|
|
||||||||||||||||||
| # Execute phase: apply redirect-with-data entries on the page | ||||||||||||||||||
| break pruneheap.c:1287 | ||||||||||||||||||
|
|
||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
| # 8. WAL REPLAY — recovery of HOT indexed updates | ||||||||||||||||||
| # src/backend/access/heap/heapam_xlog.c | ||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
|
|
||||||||||||||||||
| # WAL replay for XLOG_HEAP2_INDEXED_UPDATE | ||||||||||||||||||
| break heap_xlog_indexed_update | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||
|
|
||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
| # 9. WAL LOGGING — writing HOT indexed update records | ||||||||||||||||||
| # src/backend/access/heap/heapam.c | ||||||||||||||||||
| # ========================================================================= | ||||||||||||||||||
|
|
||||||||||||||||||
| # WAL logging for heap updates (handles indexed_update flag) | ||||||||||||||||||
| break log_heap_update | ||||||||||||||||||
|
|
||||||||||||||||||
| # Serialize redirect-with-data into WAL record (pruneheap.c) | ||||||||||||||||||
| break pruneheap.c:2936 | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # Weekly check for a newer Claude Opus on Bedrock; opens/updates one issue here | ||
| # when OCR_BEDROCK_MODEL is behind. Notify-only (GITHUB_TOKEN cannot write | ||
| # Actions variables). Logic lives in gburd/ci-workflows. | ||
|
|
||
| name: OCR model self-check | ||
|
|
||
| on: | ||
| schedule: | ||
| - cron: '0 12 * * 1' # Mondays 12:00 UTC | ||
| workflow_dispatch: | ||
|
|
||
| jobs: | ||
| check: | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-model-check.yml@v1 | ||
| with: | ||
| aws_role_arn: ${{ vars.AWS_ROLE_ARN }} | ||
| aws_region: ${{ vars.AWS_REGION }} | ||
| bedrock_model: ${{ vars.OCR_BEDROCK_MODEL }} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| # AI PR review (Claude Opus on Bedrock). Logic and review rules live in | ||
| # gburd/ci-workflows so this fork's master carries a stub instead of ~1000 | ||
| # lines that have to be rebased onto upstream forever. | ||
| # | ||
| # Needs repo variables AWS_ROLE_ARN, AWS_REGION, OCR_BEDROCK_MODEL. Auth is | ||
| # GitHub OIDC; no static AWS keys. See the ci-workflows README. | ||
|
|
||
| name: OCR AI Review | ||
|
|
||
| on: | ||
| pull_request: | ||
| # Note: no draft filter — drafts are reviewed too. | ||
| types: [opened, synchronize, reopened, ready_for_review] | ||
| issue_comment: | ||
| types: [created] | ||
| workflow_dispatch: | ||
| inputs: | ||
| pr_number: | ||
| description: 'PR number to review' | ||
| required: true | ||
| type: number | ||
|
|
||
| # One review per PR; cancel superseded runs to save Bedrock spend. | ||
| concurrency: | ||
| group: ocr-review-${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| review: | ||
| # Comment events only when the comment is on a PR and starts with the | ||
| # trigger keyword; PR events and manual dispatch always. | ||
| if: | | ||
| github.event_name == 'pull_request' || | ||
| github.event_name == 'workflow_dispatch' || | ||
| (github.event_name == 'issue_comment' && github.event.issue.pull_request && | ||
| (startsWith(github.event.comment.body, '/open-code-review') || | ||
| startsWith(github.event.comment.body, '@open-code-review') || | ||
| startsWith(github.event.comment.body, '/pg-history'))) | ||
| uses: gburd/ci-workflows/.github/workflows/ocr-review.yml@v1 | ||
| with: | ||
| aws_role_arn: ${{ vars.AWS_ROLE_ARN }} | ||
| aws_region: ${{ vars.AWS_REGION }} | ||
| bedrock_model: ${{ vars.OCR_BEDROCK_MODEL }} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| # Keep this fork's master rebased on postgres/postgres master, carrying only | ||
| # the fork-owned CI stubs (.github/workflows/fork-*.yml). The workflows they | ||
| # call, and the OCR review rules, live in gburd/ci-workflows. | ||
| # | ||
| # Requires the SYNC_TOKEN secret: a PAT with repo+workflow scope. The default | ||
| # GITHUB_TOKEN is not allowed to push commits that touch .github/workflows/, | ||
| # and every commit we carry does. | ||
|
|
||
| name: Sync from upstream | ||
|
|
||
| on: | ||
| schedule: | ||
| - cron: '17 */3 * * *' | ||
| workflow_dispatch: | ||
|
|
||
| concurrency: | ||
| group: fork-sync-upstream | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| sync: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 0 | ||
| token: ${{ secrets.SYNC_TOKEN }} | ||
|
|
||
| - name: Rebase master onto upstream | ||
| run: | | ||
| set -euo pipefail | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
| git remote add upstream https://github.com/postgres/postgres.git | ||
| git fetch --no-tags upstream master | ||
|
|
||
| # Master carries fork CI only. Anything else means work landed here | ||
| # that belongs on a branch; force-rebasing it on a timer would be wrong. | ||
| if git diff --name-only upstream/master...HEAD | | ||
| grep -v '^\.github/workflows/fork-'; then | ||
| echo "::error::master carries non-CI changes (listed above); move them to a branch" | ||
| exit 1 | ||
| fi | ||
|
|
||
| git rebase upstream/master | ||
| git push --force-with-lease |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # git config core.excludesFile .local-gitignore | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entire group (.clangd, .envrc, .gdbinit, .local-gitignore, .mcp.json, flake.nix, shell.nix, pg-aliases.sh) is author-local development tooling committed into the repository root. The stated change is the atomics/spinlock refactor in the other files; none of this scaffolding is required by it, so it violates the minimal-diff rule and will be rejected on pgsql-hackers. The tree's own top-level .gitignore explicitly documents the correct convention: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." These files should live in a personal, gitignored location, not in version control. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Local-only git excludes file; unrelated to the change set and should not be committed as part of the patch. |
||
| .local-gitignore | ||
|
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is part of the developer-local scaffolding (it is activated by |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| { | ||
| "mcpServers": { | ||
| "postgresq": { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The server key There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The server key "postgresq" appears to be a truncated typo of "postgresql". It is also inconsistent with the sibling "context7" entry: this one has a "type":"http" field while "context7" has none, suggesting an incomplete/asymmetric config. (Separately, this file is author-local tooling that should not be committed.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personal MCP tooling configuration committed to the repo root; unrelated to the change and should be dropped from the patch. Separately, per the JSON key-spelling check: the server key |
||
| "type": "http", | ||
| "url": "https://pg.ddx.io/mcp/" | ||
| }, | ||
|
Comment on lines
+3
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The MCP server key
Comment on lines
+3
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This committed MCP config points at a private/third-party HTTP endpoint ( |
||
| "context7": { | ||
| "args": [ | ||
| "-y", | ||
| "@upstash/context7-mcp@latest" | ||
| ], | ||
| "command": "npx" | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # Project instructions | ||
|
|
||
| See .agent-steering-domains.md for domain-specific steering (local). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
.clangdfile 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 listsbuild/,.direnv/,.cache/,NOTES) and.github/docs/pristine-master-policy.mdconfirm 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).