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 a personal editor/LSP configuration and should not be committed to the tree. The repository's own .gitignore (lines 1-3) explicitly states the project convention: "Auxiliary files from local workflows, your preferred editor, etc. should be ignored locally using $GIT_DIR/info/exclude or ~/.gitexclude." A patch destined for pgsql-hackers/commitfest must not add personal tooling config; committing it violates the minimal-diff rule and would be an immediate rejection reason. Keep this file local (via $GIT_DIR/info/exclude) rather than adding it to the repo.

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/tooling configuration and must not be part of a PostgreSQL patch destined for pgsql-hackers. PostgreSQL's tree deliberately keeps editor/IDE-specific config out of the repository (there is no .clangd, .vscode, etc.). Committing it is an unrelated change that violates the minimal-diff rule and will draw an immediate rejection. Such settings belong in your local ignored files (e.g. via a personal global gitignore or a local, uncommitted file), not in the shared tree. Note that several sibling files in this change (.gdbinit, pg-aliases.sh, src/test/benchmarks/siu/scripts/*) appear to be the same class of personal-workflow artifacts and should be dropped from the patch as well.

Beyond not belonging in the tree, the contents are also non-portable and stale: the hardcoded relative include path -I../../../../src/include only resolves from a specific subdirectory depth and is meaningless from the repo root; -DPGDLLIMPORT= defeats the Windows/MSVC PGDLLIMPORT checks; and the flag list will silently diverge from the real build flags maintained by the build system (make/meson).

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
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 .gdbinit is a personal debugging artifact and does not belong in a PostgreSQL patch. It violates the minimal-diff / one-logical-change discipline (an automatic rejection reason on pgsql-hackers) and is also a security footgun: GDB auto-loads a .gdbinit from the current working directory, so anyone running gdb inside the checkout silently executes these commands (auto-load-safe-path concerns). Remove this file from the patch; if such tooling is wanted, keep it in a personal dotfile outside the repo root.

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 destined for pgsql-hackers/commitfest. It is unrelated to the feature implementation and violates the 'minimal diff' discipline -- one of the top rejection reasons on -hackers. It is added alongside other stray developer-only files in this same change (.clangd, pg-aliases.sh, src/test/benchmarks/siu/scripts/build.sh), which reinforces that these all belong in a local/ignored setup, not in the tree. Remove .gdbinit 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 .gdbinit is a personal developer debugging artifact and must not be part of a PostgreSQL patch destined for pgsql-hackers/commitfest. It is unrelated to the HOT indexed updates feature and violates the minimal-diff discipline (a change should not carry per-developer debug scaffolding). The repo's own .github/docs/pristine-master-policy.md already lists .gdbinit as debugger configuration that should stay out of the tracked tree. There is also a footgun: GDB auto-sources a .gdbinit from the current working directory, so anyone running gdb in the repo root would silently execute these commands. Drop this file from the patch. Note the same scope-creep applies to the bundled .clangd, pg-aliases.sh, and src/test/benchmarks/siu/scripts/* dev tooling, which should likewise be excluded. Confidence: high.

#
# Usage: gdb -x .gdbinit <postgres-binary>
# 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 the comment headers throughout this file (lines 1, 15, 38, 52, 81, 98, 106, 117, 140, 148). PostgreSQL requires ASCII-only content in source and diffs; em-dashes, smart quotes and ellipses are disallowed. If any of this were retained, replace the em-dashes with a plain ASCII hyphen/dash. Confidence: high.

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

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 punctuation (em-dash U+2014) in comment text. PostgreSQL source and diffs must be ASCII-only; em/en-dashes and smart quotes are not permitted. This occurs throughout the header comments (lines 1, 15, 38, 52, 81, 98, 106, 117, 140, 148). Even setting aside that the file should be removed, this would fail the ASCII-only rule if reused in real source.

# =========================================================================

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

Breakpoints pinned to hard-coded line numbers in files modified by this very patch series (heapam.c:3210/4019/4024/4033/4101/4147, heapam_indexscan.c:182/250/297, indexam.c:299, execIndexing.c:370, pruneheap.c:1287/1802/1836/1863/2936) are extremely fragile. Any further edit shifts every subsequent line, so the breakpoints silently land on the wrong statements and the annotations (e.g. 'Line 4019: pure HOT', 'Line 4024: HOT indexed path') cannot be trusted. Another reason this file should not be tracked in the repo.

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 set on hard-coded absolute source line numbers are extremely brittle: any edit to these files (or a rebase) immediately makes them point at the wrong statement, silently misleading anyone who sources the file. The accompanying comments (e.g. 'Line 4019: pure HOT') hardcode line placement that will drift. This applies to every file:NNN breakpoint in the file (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). Another reason to drop the file entirely.


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

Hard-coded file:line breakpoints (heapam.c:3210/4019/4024/4033/4101/4147, heapam_indexscan.c:182/250/297, indexam.c:299, execIndexing.c:370, pruneheap.c:1287/1802/1836/1863/2936) are inherently fragile: they silently drift to the wrong statement whenever the source is edited — including by this very patch series. The accompanying comments assert exact per-line semantics (e.g. "Line 4019: pure HOT", "Line 4147: Restore HEAD_INDEXED_UPDATED on old tuple"), which become stale and misleading the moment the code moves. If any GDB helper were kept at all, it should use function-name breakpoints only. Confidence: high.


# 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 function-name breakpoints reference symbols that do not exist in the patched source. The tuple-building code is heap_form_hot_indexed_tuple (heapam.c:4924), not heap_hot_indexed_create_tuple; and there is no heap_hot_indexed_tuple_size/heap_hot_indexed_serialize_bitmap (verified via search — no matches). GDB will fail to bind these breakpoints and the descriptive comments are inaccurate. Only heap_hot_search_buffer and the HEAP_INDEXED_UPDATED flag were confirmed to exist.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Function-name breakpoint on a symbol that does not exist. A codebase search finds no heap_hot_indexed_* symbols anywhere in src/backend/access/heap/*.c or src/include/access/*.h. GDB will silently create unresolved/pending breakpoints for all of these (heap_hot_indexed_tuple_size, heap_hot_indexed_create_tuple, heap_hot_indexed_serialize_bitmap, heap_hot_indexed_bitmap_raw_size, heap_hot_indexed_has_bitmap_space, heap_hot_indexed_read_bitmap, heap_hot_indexed_bitmap_overlaps_raw, heap_hot_indexed_bitmap_or_raw, heap_hot_indexed_accum_overlaps, heap_hot_indexed_merge_bitmaps_raw, heap_hot_indexed_deserialize_bitmap), giving a false impression of coverage. This is a symptom of tracking debug scaffolding in the repo rather than keeping it local.


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

There is no heap_xlog_indexed_update function in the patch. WAL replay for HOT indexed updates is handled inside heap_xlog_update(record, true) (heapam_xlog.c:734, dispatched at line 1300); there is no separate XLOG_HEAP2_INDEXED_UPDATE handler. This breakpoint will never bind and the comment is inaccurate.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Function-name breakpoint on a nonexistent symbol: a codebase search finds no heap_xlog_indexed_update function (there is no XLOG_HEAP2_INDEXED_UPDATE replay handler by that name). This breakpoint will silently remain unresolved.


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