Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
276 changes: 276 additions & 0 deletions .agent-steering-domains.md

Large diffs are not rendered by default.

43 changes: 43 additions & 0 deletions .clangd
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
Diagnostics:
MissingIncludes: None
Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/include are 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-gitignore added in this same change lists build/ 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)

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 build/ compile-db path). PostgreSQL patches destined for -hackers must be a minimal diff; unrelated tooling files like this are a common rejection reason. This repo already ships .local-gitignore for exactly these local dev artifacts (build/, .cache/, .direnv/). Recommend not tracking .clangd at all and instead listing it in .local-gitignore. (confidence: high)

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This .clangd file is a personal editor/LSP configuration and should not be part of a patch submitted to pgsql-hackers. PostgreSQL requires a minimal, focused diff, and tooling/dev-environment files are a top reason patches are rejected on the list. This repo's own .github/docs/pristine-master-policy.md explicitly classifies .clangd (together with .envrc, .gdbinit, .local-gitignore, pg-aliases.sh) as "dev setup" that belongs on a local branch, not merged into the source tree. Drop this file from the patch (and add it to a local excludes file, which .local-gitignore in this same change already does).

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire file should not be part of the patch. .clangd is a personal editor/LSP configuration, not something the PostgreSQL tree tracks or ships. A patch destined for pgsql-hackers must be a minimal diff limited to the stated purpose (the atomics/spinlock work seen in the other changed files); bundling developer-local tooling (.clangd, .envrc, .gdbinit, .local-gitignore, pg-aliases.sh) will draw an immediate rejection. Recommend removing it and keeping it in a personal/global gitignore instead. (high confidence)

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This .clangd personal LSP/editor config is bundled into a substantive core change (the C11 <stdatomic.h> atomics rewrite touching atomics.h, s_lock.c, spin.h, new stdatomic_impl.h/spin_delay.h, etc.). It must not be part of this patch.

Two independent reasons, both blocking:

  1. This repo's own .github/docs/pristine-master-policy.md classifies .clangd as personal "dev setup" configuration that "doesn't affect the code" and requires it to live in a separate commit whose message starts with "dev setup" -- never mixed with src/ code changes. Mixing them here violates that policy and will trip the upstream-sync guard.
  2. For anything destined for pgsql-hackers/commitfest, a personal editor/LSP config is never part of a patch. Keep the patch minimal and atomic: one logical change (the atomics rework) per commit, with no unrelated tooling files.

Drop .clangd (and the sibling .envrc, .gdbinit, .local-gitignore, pg-aliases.sh) from this change and commit them separately per the pristine-master policy.

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 .gitignore explicitly says "should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." Committing it to the PostgreSQL tree contradicts that documented convention and will draw an immediate rejection on -hackers. Remove .clangd from the patch and keep it out of version control via a local exclude.

Several details also confirm this is machine-/developer-specific and non-portable, reinforcing why it does not belong in the shared tree:

  • Line 8 hardcodes CompilationDatabase: build/, assuming a specific out-of-tree build directory layout that is not the project's default.
  • Line 43 -I../../../../src/include is a fixed relative path that only resolves correctly from one particular directory depth, so it is meaningless for a repo-root config and breaks for anyone with a different working directory.
  • -DPGDLLIMPORT= (line 13) and flags like -O2/-g encode one developer's build preferences rather than a canonical project setting.

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This .clangd file is personal IDE/editor scaffolding and must not be part of a patch destined for pgsql-hackers/commitfest. The repo's root .gitignore explicitly states that "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude" -- i.e. these belong in a personal exclude file, not committed. Committing it pollutes the tree and causes needless merge conflicts. Remove it from the patch. (high confidence)

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire set of files (.clangd, .envrc, .gdbinit, .local-gitignore, .mcp.json, flake.nix, shell.nix, pg-aliases.sh) is personal developer-environment tooling and does not belong in a PostgreSQL patch destined for pgsql-hackers/commitfest. The tree's own .gitignore explicitly states that "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude" -- committing them is exactly the pattern the project tells you to avoid. Combined with the substantive atomics/spinlock rewrite in the other changed files, this patch "does more than one thing" and will be rejected on patch-hygiene grounds. Drop these files from the patch and keep them in a local gitignore/exclude instead.

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole change set (.clangd, .envrc, .gdbinit, .local-gitignore, .mcp.json, flake.nix, shell.nix, pg-aliases.sh) is developer-local editor/direnv/Nix/MCP/GDB/alias scaffolding and must not be part of a patch destined for pgsql-hackers. The repo's own top-level .gitignore explicitly says such files "should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." The <other_changed_files> show this is bundled with a substantive atomics/spinlock refactor (and HOT-indexed-update work), which violates the one-logical-change and minimal-diff rules and will be rejected outright. Keep this tooling untracked.

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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: CompilationDatabase: build/ and -I../../../../src/include are hard-coded relative paths that only resolve from one specific nested working directory and will mislead clangd for anyone else. If a per-developer clangd config is desired it should be git-ignored, not committed. (high confidence)

InlayHints:
Enabled: true
ParameterNames: true
DeducedTypes: true
CompileFlags:
CompilationDatabase: build/ # Search build/ directory for compile_commands.json
Comment on lines +7 to +8

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 -I../../../../src/include include path and the long flag list duplicate what meson already emits in compile_commands.json (referenced via CompilationDatabase: build/), so they are redundant and will drift from the real build flags.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Suggested change
- -I.
- -I../../../../src/include
- -I.
- -Isrc/include

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relative include -I../../../../src/include is wrong for a .clangd at the repository root. clangd resolves relative paths against the working directory / the file being indexed, and from the repo root the headers live at src/include, not four levels up (../../../../src/include resolves outside the tree). This would cause include resolution to fail for most files. If a root-level clangd config is intended, use -Isrc/include. (confidence: high)

Suggested change
- -I../../../../src/include
- -Isrc/include

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded, location-specific relative include path. -I../../../../src/include only resolves correctly from a directory nested exactly four levels below the repo root, which is inconsistent with -I. and with CompilationDatabase: build/ (rooted at the repo top). This is a machine-specific artifact that will silently produce wrong include resolution for anyone else, further confirming this file should not be committed.

Comment on lines +42 to +43

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This .clangd lives at the repository root, so -I. points at the root and -I../../../../src/include resolves to four directories above the repo root, i.e. outside the repository entirely. That include path cannot be correct for a root-level config (it looks copied from a deeply-nested per-directory context). Since the file is redundant scaffolding anyway, drop it; if it were kept, the header include path here is simply broken. (high confidence)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -I../../../../src/include path is wrong for a repository-root .clangd, which applies project-wide. From the repo root, ../../../../src/include resolves four directory levels above the repository, pointing outside the tree entirely; the intended header dir is src/include (relative to root). This also conflicts with CompilationDatabase: build/ above, which is the mechanism actually meant to supply include paths. If this file is retained at all (see the blocking comment above), the hardcoded relative -I flags should be removed and left to the compile_commands.json.

Suggested change
- -I../../../../src/include
- -Isrc/include

10 changes: 10 additions & 0 deletions .envrc
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 (~/ws/postgres/.envrc) plus local workflow assumptions (direnv/nix flake, project_steering/project_mcp helpers). It does not belong in a PostgreSQL patch and will not be portable to any other contributor's machine. Remove from the patch.

# flake, then layer the postgres agent context (helpers need flake's PATH +
# write into this worktree's dir).
source_up

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file cannot work for anyone but the author: it source_ups a parent .envrc and the comment hardcodes a personal home path (~/ws/postgres/.envrc), and use project_steering/use project_mcp are custom direnv extensions that only exist in the author's local setup. Committing it to a shared PostgreSQL tree provides no value to other developers and will break direnv allow for them.

watch_file flake.nix
use flake
Comment on lines +4 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This .envrc file is personal developer environment configuration (direnv/nix/AWS/MCP tooling) and must not be part of a PostgreSQL patch. It references local-only setup (source_up, use flake, use project_steering postgresql, use aws, libumem LD_PRELOAD) that is meaningless outside your machine. A patch posted to pgsql-hackers must contain only the minimal diff required for the stated change; committing this file will draw immediate rejection. It also contains a large block of commented-out code (UMEM_DEBUG, CFLAGS, MESON_EXTRA_SETUP, etc.), which is disallowed. Remove this file from the change entirely.

Comment on lines +4 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire file must not be part of the patch (high confidence). .envrc is a personal direnv config that depends on machine-specific setup (flake.nix, custom use project_steering / use project_mcp functions, AWS creds, libumem via LD_PRELOAD). None of it is required by the stated change (atomics/spinlock work), it is not portable, and it leaks the author's local development environment into the tree. Per PostgreSQL patch hygiene (minimal diff), drop this file from the commit and add it to a personal/global gitignore instead.

Comment on lines +4 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This direnv file sources a hard-coded personal path (source_up relying on ~/ws/postgres/.envrc) and loads personal MCP/steering plugins (use project_mcp postgresq -- also with the postgresq typo -- and context7). These are host-specific, non-portable, and tie the repo to one developer's machine layout. It should not be tracked in the repository.

Comment on lines +4 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 source_ups a hard-coded personal path (~/ws/postgres/.envrc) and wires in private helpers (project_steering, project_mcp postgresq, context7) that no other contributor has, so direnv allow here will fail or pull in unexpected context for anyone else. Remove it from the patch (git-ignore it locally instead). (high confidence)

use project_steering c aws
use project_steering postgresql
Comment on lines +4 to +8

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire file must not be committed. .envrc is a personal direnv artifact tied to your local machine (Nix flakes, project-specific MCP servers, AWS, libumem via LD_PRELOAD). It is unrelated to the stated purpose of this change (atomics/spinlock work in src/include/port and src/backend/storage/lmgr), so it violates the minimal-diff rule and would immediately draw a rejection on -hackers. If you want to keep it locally, add it to a personal/global gitignore rather than the tree. Confidence: high.

Comment on lines +4 to +8

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
use project_mcp postgresq
+use project_mcp postgresql

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the directory/config name: postgresq is missing a trailing q (should be postgresql). Beyond that, this file should not be part of the patch at all (see above).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: use project_mcp postgresq is missing the trailing l (should be postgresql). This is inconsistent with flake.nix referencing postgresql. Since the whole file should not be committed this is moot, but the value is clearly wrong as written. Confidence: moderate.

Suggested change
use project_mcp postgresq
+use project_mcp postgresql

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the direnv directive argument: project_mcp postgresq is missing the trailing l (should be postgresql). This is dead-on-arrival for the intended MCP registration. However, this whole file should not be in the patch at all -- fixing the typo is secondary to removing the file.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo/inconsistency: use project_mcp postgresq is missing the trailing l (compare to the postgresql used on the line above). This is dead local config that shouldn't be in the patch anyway, but if kept it points at the wrong target.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the flake namespace: postgresq is missing the trailing l (should be postgresql). If this file were kept it would fail to load the intended MCP project. This further shows the file is a local artifact that should not be committed.

Suggested change
use project_mcp postgresq
+use project_mcp postgresql

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the direnv use project_mcp argument: postgresq is missing the trailing l (should be postgresql, matching use project_steering postgresql on the line above). If any use_project_mcp helper validates its argument this will silently point at the wrong/nonexistent target. [moderate confidence]

Suggested change
use project_mcp postgresq
+use project_mcp postgresql

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the direnv directive: postgresq is missing the trailing l and is inconsistent with use project_steering postgresql on the line above. If this use project_mcp block is intended to load a postgresql MCP profile, the mismatched name will silently fail to resolve the intended target.

Suggested change
use project_mcp postgresq
+use project_mcp postgresql

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postgresq is a misspelling/truncation of "postgresql" and must match the server key defined in .mcp.json (which has the same typo). Keep the identifiers consistent and spelled correctly.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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_steering, use project_mcp) that do not exist in the repository, so direnv allow will error for anyone else. Note "postgresq" here mirrors the same truncated key in .mcp.json. This file should not be committed to the tree.

use project_mcp context7
156 changes: 156 additions & 0 deletions .gdbinit
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
# HOT Indexed Updates — GDB breakpoints for code review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 .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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 - instead.

Suggested change
# HOT Indexed Updates — GDB breakpoints for code review
+# HOT Indexed Updates - GDB breakpoints for code review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire .gdbinit file should not be part of this patch. It is a developer-personal debugging artifact, which violates the minimal-diff discipline for a pgsql-hackers/commitfest submission. It is also a footgun: a repo-root .gdbinit can be auto-sourced by GDB (subject to auto-load safe-path), silently altering the debugging behavior of anyone running gdb from the repo root. I verified it is not covered by .gitignore or .local-gitignore, so it is being tracked. Remove it from the patch, or move it to a personal/ignored location (e.g. add it to .local-gitignore).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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
# HOT Indexed Updates — GDB breakpoints for code review
# HOT Indexed Updates - GDB breakpoints for code review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 .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).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire .gdbinit file should not be part of the patch. Three problems, in order of severity:

  1. Orphaned scaffolding for a feature not in this changeset. A codebase search confirms none of the referenced symbols exist in src/ (no heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_xlog_indexed_update, HEAP_INDEXED_UPDATED, etc.). The only symbol that resolves is the pre-existing heap_hot_search_buffer. Meanwhile the actual substantive files in this change (s_lock.c, atomics.h, spin.h) are about spinlocks/atomics, not "HOT indexed updates." This file is debugging scaffolding for a different, non-present patch series -- it is dead against the current tree and signals a mis-scoped commit.

  2. Security footgun. A .gdbinit checked in at the repo root is auto-loaded by GDB when run from that directory and can execute arbitrary commands. Committing one is actively discouraged upstream.

  3. Minimal-diff / YAGNI violation. This is a developer-local convenience artifact, not buildable/testable functionality. It belongs in a local ignore (note .local-gitignore is added in this same changeset but does not list .gdbinit), not in a patch destined for pgsql-hackers/commitfest.

Recommendation: remove this file from the patch entirely. (high confidence)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-ASCII character: the em-dash (—) violates the ASCII-only rule for source and diffs. The same em-dash appears throughout this file's comments (lines 6-7, and the #=== section headers). If any version of this file were to be kept, these must be replaced with a plain hyphen (-). (high confidence)

Suggested change
# HOT Indexed Updates — GDB breakpoints for code review
# HOT Indexed Updates - GDB breakpoints for code review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire .gdbinit is a developer-local debugging artifact and should not be committed to the PostgreSQL tree. It fails the minimal-diff requirement: it is unrelated scaffolding (alongside .clangd, .envrc, .local-gitignore, pg-aliases.sh) with no functional relationship to the feature, and it will be rejected on pgsql-hackers on sight. Two concrete, verifiable defects make it worse than dead weight:

  1. Broken atomicity / references to non-existent code. Every symbol these breakpoints name (heap_hot_indexed_tuple_size, heap_hot_indexed_create_tuple, heap_hot_indexed_read_bitmap, heap_xlog_indexed_update, etc.) is found only inside this file -- a codebase search returns zero matches in src/backend/access/heap/*.c. gdb will fail to set these breakpoints against the current tree, so this commit does not stand on its own.

  2. Security footgun. gdb auto-loads .gdbinit from the working directory (subject to auto-load safe-path), so shipping one in-tree at the repo root can silently execute debugger commands in any contributor's session.

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 .local-gitignore here already attempts).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire file should be dropped from the patch. .gdbinit is a developer-local debugging artifact, not part of the HOT-indexed-updates change; it adds no tests or docs and violates the minimal-diff discipline. It is also bundled with other local-tooling files (.clangd, .envrc, .local-gitignore, pg-aliases.sh) in the same change -- a patch that does more than one thing. Note the companion .local-gitignore you added lists NOTES/AGENTS.md/build dirs but omits .gdbinit; keeping it in the ignore set (or .gitignore) is the right home for it. Committing a .gdbinit at repo root is also a POLA/security footgun: GDB auto-loads .gdbinit from the working directory, silently changing debugger behavior for anyone building in the tree.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 (.clangd, .envrc, .local-gitignore, pg-aliases.sh — the latter even references .gdbinit directly), which confirms it was staged by accident. Committing a .gdbinit at the repo root is also a footgun (POLA violation): GDB auto-loads .gdbinit from the working directory, so anyone launching gdb in the tree root would silently get ~40 breakpoints set. This violates the minimal-diff discipline and will draw an immediate rejection on -hackers. Remove it from the patch. (high confidence)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 -- or - instead. (high confidence)

Suggested change
# HOT Indexed Updates — GDB breakpoints for code review
+# HOT Indexed Updates -- GDB breakpoints for code review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This .gdbinit file is a personal developer debugging artifact and should not be committed to the repository. It violates the minimal-diff discipline (a top rejection reason on pgsql-hackers) and is unrelated to the actual HOT-indexed-updates feature implementation. This change set already adds .local-gitignore, which documents the correct mechanism (git config core.excludesFile .local-gitignore) for keeping such files untracked locally. Remove this file (along with the other tooling artifacts: .clangd, .envrc, pg-aliases.sh) from the patch and add it to your personal excludes instead.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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:

  1. It breaks on functions that do not exist anywhere in this tree. heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_xlog_indexed_update, etc. have no definition in the source (the only matches are inside this .gdbinit). Sourcing this file would fail to set those breakpoints.

  2. The hardcoded line-number breakpoints point at unrelated code. For example break heapam.c:3210 is annotated "Main entry: heap_update" but line 3210 is the closing brace } of a different function. break heapam_indexscan.c:182 is annotated "initialize bitmap accumulator" but line 182 is lp = PageGetItemId(page, offnum);. Line-number breakpoints are inherently fragile and have already drifted.

For a patch destined for pgsql-hackers/commitfest, personal debugger scaffolding referencing an unrelated feature should be removed from the patch entirely.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 atomics.h/spin.h), but every breakpoint here targets an unrelated, unmerged "HOT indexed updates" feature in heapam.c/pruneheap.c/execIndexing.c/indexam.c -- files that are not modified by this change set at all. Bundling it makes the PR do more than one thing and will draw an immediate rejection on pgsql-hackers. Remove .gdbinit from the patch.

#
# Usage: gdb -x .gdbinit <postgres-binary>
Comment on lines +1 to +3

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire .gdbinit file must not be part of a PostgreSQL patch. Two independent, blocking problems:

  1. 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-gitignore itself says are "not tracked in .gitignore". Unrelated scaffolding is the top rejection reason on -hackers. It is also a footgun: GDB auto-sources .gdbinit from 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.

  2. The referenced feature does not exist in this change set, so the file is broken/fabricated. I verified against the tree:

    • HEAP_INDEXED_UPDATED and XLOG_HEAP2_INDEXED_UPDATE: no matches anywhere.
    • heap_hot_indexed_tuple_size, heap_hot_indexed_create_tuple (and all heap_hot_indexed_* helpers): no matches.
    • heap_xlog_indexed_update: no matches; heapam_xlog.c only replays XLOG_HEAP_UPDATE/XLOG_HEAP_HOT_UPDATE.
    • log_heap_update sets info = 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 every break <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.

Comment on lines +1 to +3

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 .gdbinit from the working directory, so committing this into the tracked tree causes it to auto-execute for anyone running gdb at the repo root. Developer debugging scripts belong in a local ignore file, not the source tree. Remove this file (and the accompanying .clangd, .envrc, .local-gitignore, pg-aliases.sh environment dumps) from the patch.

# 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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
# 1. UPDATE DECISION — heap_update() HOT/HOT-indexed/non-HOT choice
# 1. UPDATE DECISION - heap_update() HOT/HOT-indexed/non-HOT choice

# src/backend/access/heap/heapam.c
# =========================================================================

# Main entry: heap_update
break heapam.c:3210

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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. heapam.c:3210 is the comment block for simple_heap_delete, not the heap_update entry. heapam.c:4019 is a comment inside the "doesn't fit, avoid deadlock" branch; heapam.c:4024 is the else { /* We're all done */ } block -- neither is the HOT decision, which is actually the bms_overlap(modified_attrs, hot_attrs) test around line 4070. Line-number-anchored breakpoints are inherently brittle: any edit shifts them silently, so they are misleading on arrival with no mechanism to keep them in sync.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard-coded break <file>:<line> directives are inherently fragile: any edit to the surrounding source (including the feature commits in this same series) shifts these lines, so the breakpoints silently land on the wrong code. The comments assert precise semantics (e.g. "Line 4019: pure HOT", "Line 4024: HOT indexed path") that cannot be kept accurate and will mislead anyone who sources the file. If a debugging helper must exist, prefer symbol-based breakpoints (as done elsewhere in this file) over file:line, but ideally this file should not be committed at all.

Comment on lines +19 to +20

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two independent problems make this file unfit for the tree:

  1. It is orphaned scaffolding. Every symbol breakpoint here targets a "HOT indexed updates" feature that does not exist in this branch: heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_xlog_indexed_update, and the HEAP_INDEXED_UPDATED flag all resolve to zero matches across the source tree. GDB will silently create pending breakpoints for names that never load.

  2. The line-number breakpoints are already stale. heapam.c:3210 is not the heap_update entry -- it is the comment header of simple_heap_delete. heapam_indexscan.c:182 is a plain if (valid) visibility check, not the claimed "Redirect-with-data: initialize bitmap accumulator". Line-number breakpoints rot on any edit.

This is a personal debugging file; remove it from the patch and keep it in a local $GIT_DIR/info/exclude. (high confidence)

Comment on lines +19 to +20

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded absolute source line numbers (e.g. heapam.c:3210, heapam.c:4019, pruneheap.c:1802, indexam.c:299, heapam_indexscan.c:182) drift the instant any of those files change and will silently set breakpoints at the wrong location. Even for personal use this is a footgun; it should not be committed to the repository.

Comment on lines +19 to +20

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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:

  • The function-name breakpoints are dead references: heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_hot_indexed_read_bitmap, heap_xlog_indexed_update, etc. do not exist anywhere in src/backend/access/heap/*.c.
  • The hardcoded file:line breakpoints are stale and bind to unrelated code: e.g. heapam.c:3210 (commented "Main entry: heap_update") is actually the closing brace of heap_delete. heapam_indexscan.c is only 588 lines and is plain index-scan code, so heapam_indexscan.c:250/:297 land in unrelated getnext-slot logic.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file hard-codes line-number breakpoints (break heapam.c:3210, heapam.c:4019, pruneheap.c:2936, indexam.c:299, execIndexing.c:370, etc.) and symbol breakpoints for a HOT-indexed-update feature (heap_hot_indexed_create_tuple, heap_hot_search_buffer, heap_xlog_indexed_update, ...) that does not exist in this tree. I searched the source: heap_hot_indexed_create_tuple and heap_xlog_indexed_update have no matches anywhere. GDB will error out on these breakpoints at load time, and the line-number breakpoints silently point at the wrong code the moment any line above them shifts. Beyond being broken, this is a personal debug script that does not belong in a committed patch -- it should live in $GIT_DIR/info/exclude / ~/.gitexclude as the repo's own top-level .gitignore instructs.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 break <file>:<line> in the file (heapam.c, heapam_indexscan.c, indexam.c, execIndexing.c, pruneheap.c). (high confidence)

Comment on lines +27 to +29

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breakpoints on hard-coded absolute 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 stale-by-construction. Any edit above these lines shifts every subsequent breakpoint to the wrong location, and these numbers describe one in-progress working copy that will not match the committed code, let alone survive a rebase. The accompanying comments (e.g. "Line 4019: pure HOT") hard-code assumptions about code that this very series is modifying, so they are wrong the moment the surrounding patch changes. There is no CI to catch this drift.

Comment on lines +27 to +29

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 heap_hot_indexed_create_tuple, heap_xlog_indexed_update, and HEAP_INDEXED_UPDATED returns no matches anywhere in the tree. This debugging aid documents a feature that is not present in the reviewed change, making it both unverifiable and misleading.

Comment on lines +27 to +29

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded file:lineno 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 inherently fragile — they drift the instant surrounding code changes, and the very patch series they accompany is modifying these exact files. The accompanying comments (e.g. "Line 4019: pure HOT", "returns 0 if t_hoff would overflow") assert precise semantics tied to those line numbers and will silently rot, giving false debugging context. (high confidence)


# 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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: heap_hot_indexed_*, heap_xlog_indexed_update, heap_hot_search_buffer, HEAP_INDEXED_UPDATED, XLOG_HEAP2_INDEXED_UPDATE, and log_heap_update match only inside this .gdbinit file. heapam_indexscan.c exists but is only 299 lines of standard index-scan callbacks with no HOT-chain/bitmap logic, so breakpoints at lines 182/250/297 and heap_hot_search_buffer target code that isn't present. None of the referenced feature source files are in this change set either. Committing debug config for absent code is a non-atomic/non-bisectable, dead/misleading state.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The heap_hot_indexed_* helper functions targeted by these symbol breakpoints do not exist anywhere in this tree (searched src/backend/access/heap/*.c: no matches). GDB will emit "Function "..." not defined" for each on load. The described "HOT indexed updates" mechanism (embedded modified-column bitmap, HEAP_INDEXED_UPDATED, redirect-with-data) is not present in the source being submitted -- this file was authored against a private tree state not represented in the patch.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 -- heap_hot_indexed_tuple_size, heap_hot_indexed_create_tuple, heap_hot_search_buffer, heap_xlog_indexed_update, etc. appear only here in .gdbinit and in no .c/.h file. This file is tied to an unrelated, not-present "HOT indexed updates" patch series and is dead/aspirational relative to the actual change (the C11 atomics rewrite).


# 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heap_xlog_indexed_update does not exist in this tree (no match under src/backend). This symbol breakpoint will fail to bind ("Function not defined") on load. The XLOG_HEAP2_INDEXED_UPDATE record type it refers to is not part of the submitted code.


# =========================================================================
# 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
18 changes: 18 additions & 0 deletions .github/workflows/fork-ocr-model-check.yml
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 }}
43 changes: 43 additions & 0 deletions .github/workflows/fork-ocr-review.yml
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 }}
48 changes: 48 additions & 0 deletions .github/workflows/fork-sync-upstream.yml
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
3 changes: 3 additions & 0 deletions .local-gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# git config core.excludesFile .local-gitignore

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.local-gitignore is a personal local-exclude mechanism (meant to be wired up with git config core.excludesFile). It should not be a tracked file in the repository -- the whole point of a local excludes file is that it stays local. Combined with the other dev-tooling dotfiles, drop this from the patch.

Comment on lines +1 to +2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 shell.nix running git config core.excludesFile .local-gitignore on every shell entry, which globally reconfigures the user's git excludesFile for this repo as a side effect). It is unrelated to the atomics/spinlock change and should not be committed to the PostgreSQL tree. (high confidence)


15 changes: 15 additions & 0 deletions .mcp.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"mcpServers": {
"postgresq": {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server key postgresq appears to be a typo/truncation of "postgresql". It is also referenced as project_mcp postgresq in .envrc, so the typo is duplicated. Rename both to the correct, consistent key.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 "postgresq" is missing a trailing 'l' (likely intended "postgresql"). It is referenced consistently as project_mcp postgresq in .envrc, so it is cosmetic rather than functional, but the key name is a typo.

"type": "http",
"url": "https://pg.ddx.io/mcp/"
},
Comment on lines +3 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MCP server key "postgresq" is misspelled/truncated (should presumably be "postgresql"), and it points at a personal third-party endpoint https://pg.ddx.io/mcp/. Shipping a developer's MCP config that routes context through an external service in a shared repository is a supply-chain/privacy concern. This file should not be committed at all.

Comment on lines +3 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This committed MCP config points at a private/third-party HTTP endpoint (https://pg.ddx.io/mcp/) and auto-launches npx @upstash/context7-mcp@latest. It is both unrelated to the atomics/spinlock change and a footgun: committing an auto-connecting external endpoint plus an auto-npx command into the tree means every checkout silently fetches and runs remote tooling against an untrusted host. This file should not be in the PostgreSQL tree. (high confidence)

"context7": {
"args": [
"-y",
"@upstash/context7-mcp@latest"
],
"command": "npx"
}
}
}
3 changes: 3 additions & 0 deletions AGENTS.md
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).
Loading
Loading