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

InlayHints:
Enabled: true
ParameterNames: true
DeducedTypes: true
CompileFlags:
CompilationDatabase: build/ # Search build/ directory for compile_commands.json
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

24 changes: 24 additions & 0 deletions .envrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
source_up
watch_file flake.nix
use flake
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 .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.

use project_steering postgresql
Comment on lines +1 to +4

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.

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.

use project_mcp context7
use aws
Comment on lines +1 to +7

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 is a personal direnv/Nix developer environment file and must not be part of a PostgreSQL patch. It references private tooling (project_steering, project_mcp, use aws) and machine-local configuration that is meaningless to the community and does not belong in the tree. Committing it violates the minimal-diff discipline (it is entirely unrelated to the stated change) and would also add a repo-wide dotfile with no documentation or build-system integration. Note also the typo use project_mcp postgresq (missing trailing l). Remove this file from the patch and keep it in your local (gitignored) environment instead.

Comment on lines +1 to +7

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. .envrc is a personal direnv configuration tied to the author's local workflow (Nix flake.nix, project_steering/project_mcp, aws, libumem LD_PRELOAD, coverage flags). The repository's own root .gitignore states the policy explicitly: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." Committing it violates the minimal-diff rule (a top rejection reason on pgsql-hackers), is unrelated to the stated change, and leaks environment-specific details. Remove .envrc from the change and ignore it locally instead. (high confidence)

Additionally, line 5 has a typo: use project_mcp postgresq (missing trailing l), which shows this is untested personal scaffolding rather than intended contribution content.

Comment on lines +1 to +7

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 direnv configuration and must not be part of the patch. It leaks the author's local dev setup (use flake, project_steering, project_mcp, use aws, libumem/LD_PRELOAD notes, coverage/sanitizer flags) into a change that goes to pgsql-hackers. It is unrelated to the stated purpose, breaks the minimal-diff rule, and would cause needless conflicts and confusion for every reviewer and committer. Remove .envrc from the patch entirely (add it to your personal/global gitignore instead of committing it). High confidence.

Comment on lines +1 to +7

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 to a PostgreSQL patch. .envrc is a personal direnv configuration file that is specific to the author's local development environment (nix flakes, AWS, libumem via LD_PRELOAD, private project_steering/project_mcp helpers). None of this is required by, or related to, the actual code change being submitted, and it will never be accepted on pgsql-hackers. Per the tree's own convention, such auxiliary/editor/workflow files should be ignored locally via $GIT_DIR/info/exclude or core.excludesFile (this repo even adds a .local-gitignore for exactly that purpose), not tracked. Drop this file from the patch entirely.

Comment on lines +1 to +7

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 configuration that hard-codes one developer's local tooling (use flake, use project_steering, use project_mcp, use aws, libumem LD_PRELOAD debug settings, personal CFLAGS/coverage exports). None of this is required by the stated purpose of the patch and it is not portable across the platforms PostgreSQL supports (Linux/Windows/macOS/BSD/Solaris). Including it will draw an immediate rejection on pgsql-hackers as an unrelated, non-minimal change. Remove .envrc from the patch; if you need a local dev environment, keep it untracked (add to your personal/global gitignore, not the tree).

Comment on lines +1 to +7

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 must not be part of a PostgreSQL patch. It is a personal direnv configuration containing developer-specific setup (source_up, use flake, use aws, private use project_steering/use project_mcp directives) that has nothing to do with the stated change. Committing it pollutes the tree, will draw an immediate rejection on -hackers, and belongs in a local (git-ignored) file instead. Remove it from the patch entirely.


# libumem is automatically loaded via LD_PRELOAD in the shell environment
# All programs in this shell will use libumem instead of glibc malloc
# Environment variables:
# UMEM_DEBUG=audit,default
# UMEM_LOGGING=transaction,contents,fail

# Configure libumem with AUDIT mode (more aggressive checking)
#export UMEM_DEBUG="audit,contents,guards" # audit=catch more bugs, guards=detect overruns
#export UMEM_LOGGING="transaction,contents,fail"
Comment on lines +16 to +17

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Large blocks of commented-out configuration (UMEM_DEBUG/UMEM_LOGGING, MESON_EXTRA_SETUP, coverage/sanitizer CFLAGS) are dead, commented-out code and personal knobs. Even if some env bootstrap were desired, commented-out code is not acceptable in the tree.

Comment on lines +16 to +17

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Large block of commented-out configuration (UMEM_DEBUG, MESON_EXTRA_SETUP, CFLAGS, sanitizer flags). PostgreSQL style forbids committing commented-out code, and these are personal debugging knobs that should never enter the tree. This reinforces that the whole file should be dropped from the patch.


#export MESON_EXTRA_SETUP="-Db_coverage=true"
#export GENINFO_OPTIONS="--ignore-errors inconsistent,gcov"
#export LCOV_OPTIONS="--ignore-errors inconsistent,gcov"
Comment on lines +19 to +21

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Large blocks of commented-out export statements (libumem tuning, coverage, sanitizer CFLAGS) are dead/commented-out config. PostgreSQL patch hygiene forbids commented-out code and speculative scaffolding for paths that aren't wired up. Remove.


#export CFLAGS="-Wall -Wextra -Wconversion -Wdouble-promotion -Wno-unused-parameter -Wno-unused-function -Wno-sign-conversion -fsanitize-trap --werror"
# -fsanitize=undefined,address,undefined,thread
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

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


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


# 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/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Node modules
scripts/ai-review/node_modules/
# Note: package-lock.json should be committed for reproducible CI/CD builds

# Logs
scripts/ai-review/cost-log-*.json
scripts/ai-review/*.log

# OS files
.DS_Store
Thumbs.db

# Editor files
*.swp
*.swo
*~
.vscode/
.idea/
Loading