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.

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

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

use aws
Comment on lines +1 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 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 +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. .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 +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 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.


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

#export MESON_EXTRA_SETUP="-Db_coverage=true"
#export GENINFO_OPTIONS="--ignore-errors inconsistent,gcov"
#export LCOV_OPTIONS="--ignore-errors inconsistent,gcov"
Comment on lines +18 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.

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

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

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


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


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


# 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

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