Repository navigation
Conversation
01237c9 to
3b56d29
Compare
c2a5026 to
d8fed9e
Compare
c999d4e to
d3a10f3
Compare
14db4ac to
bad9884
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "$PG_BUILD_DIR" \ | ||
| "$PG_SOURCE_DIR"' | ||
|
|
||
| alias pg-compdb='compdb -p build/ list > compile_commands.json' |
There was a problem hiding this comment.
This alias hardcodes build/ and writes compile_commands.json into the current working directory, while every other command in this file consistently uses $PG_BUILD_DIR. If the user runs it from anywhere other than the source root, or with a non-build/ build dir, it silently operates on the wrong path. Use $PG_BUILD_DIR for consistency.
| alias pg-compdb='compdb -p build/ list > compile_commands.json' | |
| alias pg-compdb='compdb -p "$PG_BUILD_DIR" list > "$PG_SOURCE_DIR/compile_commands.json"' |
| --suppressions=$PG_SOURCE_DIR/src/tools/valgrind.supp \\ | ||
| --time-stamp=yes \\ | ||
| --log-file=$PG_BENCH_DIR/valgrind-%p.log \\ |
There was a problem hiding this comment.
The generated wrapper interpolates $PG_BENCH_DIR, $PG_SOURCE_DIR and $bindir into the heredoc at generation time (unquoted EOF), but if any of these are empty (the script relies on shell.nix to export them), the emitted script becomes e.g. --log-file=/valgrind-%p.log and --suppressions=/src/tools/valgrind.supp, writing to the filesystem root and pointing at a nonexistent suppressions file. Guard that these variables are non-empty before emitting the wrapper.
| &hscan->xs_prefix_all_dead); | ||
| if (!got_heap_tuple) | ||
| { | ||
| hscan->xs_hot_indexed_recheck = false; | ||
| hscan->xs_prefix_all_dead = false; | ||
| } |
There was a problem hiding this comment.
The entire prefix_all_dead / prefix_dead mechanism added here is dead scaffolding: hscan->xs_prefix_all_dead is written (here and on the no-tuple reset path) but never read anywhere in the tree (confirmed: the only references to xs_prefix_all_dead/prefix_all_dead/prefix_dead are writes in this file plus the field/param declarations in heapam.h). The comments claim it exists so "the index-access layer can kill the arriving leaf", but no consumer implements that leaf-kill decision. This violates YAGNI/minimalism: it adds a new parameter to the exported heap_hot_search_buffer signature, a new IndexScanHeapData field, a local, and the extra branch logic at lines 478-480 and 498-508, all for a code path that is never exercised. Either wire up the consumer that reads xs_prefix_all_dead (and add a test covering the stale-leaf-kill path), or drop the prefix_all_dead parameter, the xs_prefix_all_dead field, and the associated branches entirely until the feature that needs it lands.
| * Catalogs never take HOT-indexed updates (see HeapUpdateHotAllowable()), | ||
| * so they keep the strict checks. | ||
| */ | ||
| bool inexact_ok = !IsCatalogRelation(rel); |
There was a problem hiding this comment.
The gate inexact_ok = !IsCatalogRelation(rel) relaxes these ERRCODE_INDEX_CORRUPTED checks for every non-catalog relation, not only relations that actually took a HOT-indexed (SIU) update. A single-index table, or one on which SIU never fired, loses the "points past end / points to unused LP / points to heap-only tuple" safety net during ordinary DML. The adjacent comments assert the opposite ("Only relations that can take HOT-indexed updates can produce such an entry" / "only a relation that can take HOT-indexed updates can get here legitimately"), which overstates the gate's precision — inexact_ok is true for all user tables regardless of whether they are SIU-capable. Please either tighten the predicate to something that reflects SIU capability (and verify the downstream chain walk truly reaches a "vacuumable" conclusion in every relaxed case) or reword the comments to state the actual, broader scope so a future reader is not misled about how much of this corruption detection remains active. [high confidence]
| idxattrs = RelationGetIndexedAttrs(indexRelation); | ||
| while ((x = bms_next_member(idxattrs, x)) >= 0) |
There was a problem hiding this comment.
Performance (low/moderate confidence): heapam_index_entry_needs_recheck is called once per returned tuple from heapam_index_getnext_slot (both the plain and index-only paths). Whenever the chain walk crossed a HOT/SIU hop (hot_indexed_recheck true), it calls RelationGetIndexedAttrs(indexRelation), which does a bms_copy on every call (see relcache.c: it deliberately never hands out the cached pointer) and is freed again at line 920. On a table with heavy HOT-indexed update churn and many not-yet-cleaned stale ancestor entries, this is a per-tuple palloc/bms_copy/bms_free in the scan hot path. Consider caching the index's indexed-attrs bitmap once per scan (e.g. in IndexScanHeapData at scan_begin) rather than recomputing it per tuple, so the stale path pays the copy once instead of per row.
Squashes the four CI commits this fork carried on top of upstream into
one, and cuts the carry from 3776 lines to ~1000.
Removed:
- .github/{README,QUICKSTART}.md and .github/docs/* (2273 lines). Beyond
being bulk we rebase hourly, they had drifted into being wrong: they
described a daily (not hourly) sync, Claude 3.5 Sonnet via the direct
Anthropic API (we run Opus on Bedrock), a static IAM user (we use
OIDC), and a Windows dependency-builder workflow that the same commit
which documented it had already deleted. 16 referenced paths did not
exist. An AI reviewer reading this repo ingests that as truth.
- sync-upstream-manual.yml (252 lines). sync-upstream.yml already has
workflow_dispatch; the sole behavioral difference was a force_push
toggle whose "false" setting cannot work after a rebase anyway.
- .github/.gitignore, which only ignored scripts/ai-review/, a directory
that does not exist here.
Rewritten:
- sync-upstream.yml -> fork-sync-upstream.yml, 260 lines to 47. git
rebase already handles the fast-forward, no-op and diverged cases that
the ahead/behind arithmetic hand-rolled, so all that goes away, as do
the issue open/comment/close machinery (a scheduled-run failure is
already emailed to the one consumer, and the run log carries what the
issue body would) and the "dev setup|dev v[0-9]" commit allowlist,
which matched zero commits on master and had since it was written.
Sync drops to every three hours: hourly meant 24 full-history clones
of an 840MB repo per day, each force-push also re-triggering pg-ci.yml
on master, to collect a handful of upstream commits.
Renamed:
- ocr-review.yml, ocr-model-check.yml -> fork-*.yml. Upstream owns
.github/ too (it has touched pg-ci.yml three times this year), so
fork-owned workflows now live under a name prefix upstream will not
collide with, making the rebase structurally conflict-free and letting
the sync guard match fork-owned paths exactly rather than allowing all
of .github/.
Unchanged: the OCR review workflows' logic and .github/ocr/* (rule.json,
context.md, litellm.yaml, pg-history.py) - the part that carries value.
The OCR review workflows and their config were the last bulk this fork carried on top of upstream: ~970 lines rebased onto postgres/postgres every few hours, editable only by amending a commit that gets force- pushed. They are now reusable (workflow_call) workflows in the sidecar repo gburd/ci-workflows, and master carries two caller stubs instead. Carry after this: 3 files, ~110 lines, one commit. The sidecar is also where the value is: ocr/rule.json and ocr/context.md (the PostgreSQL committer-grade review corpus) become normal reviewable files with normal PRs and history, rather than a blob inside a commit that is rewritten on every sync. Mechanics worth knowing: - The review jobs check ci-workflows out at github.job_workflow_sha, so the rules always come from the same commit as the logic using them, and pinning @v1 (or a sha) in the stub pins both. This replaces the old "git show origin/<default_branch>:.github/ocr/..." dance, which existed only because the config had to be read off a branch other than the PR's. - AWS settings are passed as explicit inputs rather than read from vars inside the reusable workflow. vars does resolve across the boundary, but the contract stays visible at the call site. - A reusable workflow does not change the OIDC subject: it stays repo:gburd/postgres:*, so the ocr-bedrock-ci trust policy is unchanged. - ci-workflows must stay public; callers read it with GITHUB_TOKEN. The sync guard narrows to .github/workflows/fork-* accordingly, and sync-upstream keeps needing SYNC_TOKEN: the stubs are still workflow files, which GITHUB_TOKEN may not push.
This isn't a commit that will be submitting for review, it is purely for local developer tooling while developing this patch set. Ignore it.
An index stores a locator for every entry: the value it hands back to the table AM to identify the row. Every in-core table AM uses an ItemPointerData, a block number plus an in-block offset, and a good deal of core code leans on that shape. Bitmap scans split the locator into a block and an offset, GIN packs the offset into 11 bits, TID range scans expose its ordering to SQL, and nbtree breaks ties between equal keys with it. A table AM that identifies rows some other way has no way, today, to tell core any of this, and core has no way to ask. Give both sides a place to say what they mean. A table AM returns a per-relation locator descriptor from a new relation_locator callback, reached through RelationGetLocatorDesc() and cached on the relcache entry. The descriptor gives the locator's width (negative for a variable-width locator), a name for error messages, whether a row keeps its locator for its whole life (stable), whether the version an UPDATE replaced can still be fetched by its locator (old_version_retained, with RelationUpdatesInPlace() for the executor), and max_offset, the largest offset any of the relation's locators carries. max_offset has two readers today: the planner does not consider bitmap scans of a relation whose max_offset exceeds what a TID bitmap can hold, and ginbuild() refuses such a table, since GIN's posting lists and partial-match scans assume the same bound. This is the gap Jeff Davis ran into in "MaxOffsetNumber for Table AMs" (2021). The other fields describe properties that later commits in this series act on. amlocator.h and a new "Row Locators" section of tableam.sgml also write down what core relies on of every locator that no field states: that it travels as an ItemPointerData, that it is the row's ctid, which values are reserved, and the (block, offset) order that nbtree, ANALYZE, TID range scans and BRIN depend on. A table AM without the callback is treated as heap is, so out-of-tree AMs keep working untouched. Heap returns a single static descriptor that writes down what core already assumes of a TID, so for heap this is a no-op. Discussion: https://postgr.es/m/0bbeb784050503036344e1f08513f13b2083244b.camel@j-davis.com
The field is documented as "an update chain was followed", but that is not what its readers act on. heapam_tuple_lock sets it only after replacing *tid, and every reader (ExecLockRows, GetTupleForTrigger, ri_LockPKTuple, and the EvalPlanQual paths in ExecUpdate and ExecDelete) responds by re-evaluating the row it read earlier. What they need to know is that the version they locked is later than the one they named, because a concurrent transaction updated the row and committed. For heap that is the same thing as following the update chain, so the rename changes no behavior. It gives a table AM whose rows are not identified by a version-specific TID a definition it can implement, which "Support table AMs that update rows in place" completes.
Keep physical tuple links and transaction/command IDs inside the heap AM. Callers need to distinguish same-command and triggered modifications, current-transaction ownership, and partition moves, not interpret heap headers or special TID values. Translate only fields defined for the returned result, using the supplied command ID and preserving the creation-command check when a lock follows an update chain. Initialize failure facts on every call without losing retargeted across internal retries. Keep existing heap partition-move errors while allowing MERGE to recognize an AM-reported move. Exercise the booleans directly through the real lock callback, including poisoned output, child transactions, command visibility and retargeting. Check MERGE cardinality handling in a child transaction as well.
table_tuple_lock, table_tuple_update and table_tuple_delete take a locator, and for heap a TID names one version of a row. A table AM whose locator names only the row, as one that updates rows in place does, cannot tell from the call whether the caller read the row through its snapshot, in which case a newer committed version means the caller must re-evaluate, or has already locked the latest version itself, in which case it must not. Guessing either way is wrong: re-evaluating a row the command already locked runs EvalPlanQual where ON CONFLICT DO UPDATE and MERGE cannot take it, and assuming "already locked" whenever the transaction holds a lock loses concurrent updates in an UPDATE fed by a SELECT ... FOR UPDATE subquery, or in a FOR UPDATE OF join that reaches a locked row twice. Only the executor knows. Add TUPLE_LOCK_FLAG_LOCKED_VERSION, TABLE_UPDATE_LOCKED_VERSION and TABLE_DELETE_LOCKED_VERSION, meaning "this is the version this command has already locked", and pass them where that is so: after a BEFORE ROW trigger or EvalPlanQual has locked the row in ExecUpdate, ExecDelete and MERGE, for the row ON CONFLICT DO UPDATE and DO SELECT lock, and from logical replication apply, which locks the row first and has no snapshot. ExecLockRows and the RI triggers never pass them, since their rows come from snapshot scans. GetTupleForTrigger learns of it through a new row_locked argument to ExecBRUpdateTriggers and ExecBRDeleteTriggers. Heap ignores the new bits, since its TID already names a version, so nothing changes for heap.
Index AMs, the executor and every index-facing table AM callback pass a row's locator as an ItemPointerData, and validate_index() packs it into an int8 for CREATE INDEX CONCURRENTLY. A table AM whose locator descriptor declares a wider or variable-width locator can therefore not be indexed by any in-core index AM. Let the index side say whether it can do better. An index AM sets amcanvarlocator when it can store a locator that does not fit in an ItemPointerData, reported by pg_indexam_has_property() as "can_var_locator". Unless it does, building an index on a table whose locator is variable-width or wider than an ItemPointerData fails with an error naming both access methods and the locator. The check is in index_create(), before any catalog entry is made, and again in index_build(), because a table's locator can change under existing indexes: ALTER TABLE ... SET ACCESS METHOD rebuilds them through reindex_relation(), and REINDEX, TRUNCATE and VACUUM FULL rebuild them too. Every index build, including partition index cloning and constraint indexes, passes through one of the two. No in-core index AM sets the flag and no in-core table AM has a wide locator, so nothing changes for existing AMs. Carrying a wider locator through aminsert, index scans and vacuum is left for a later series; the flag lets an index AM and a table AM that agree on one find each other until then.
An AFTER ROW trigger event records the locators of the rows it fires for, and the rows are fetched by locator when the trigger runs, which for a deferred trigger is at COMMIT. That works for heap because a locator names a version and heap keeps old versions until VACUUM. For a table AM whose locator names only the row, the fetch returns whatever the row is when the trigger fires: OLD is already overwritten, and NEW may have changed again since. Let an event carry copies of its rows instead. A new AFTER_TRIGGER_IMAGES event kind records, alongside the locators, an index into a per-transaction store of copied rows: the OLD row of an UPDATE, and the NEW row of an INSERT or UPDATE for every row trigger other than a foreign key check or a unique recheck, which need the row's current version to decide whether it is still live. A copy is made once per row and shared by all the triggers that fire for it. Copies stay in memory up to work_mem and spill to a temporary file beyond that, owned by a resource owner of its own so an abort closes it without retrying a failed write. They are released at the end of a top-level statement once no deferred event remains, and a subtransaction abort rolls the store back to where it began. ExecARUpdateTriggers takes the old row the caller holds rather than fetching it again. Events carry copies only for a table whose locator descriptor says old_version_retained = false. The next commit makes the executor copy the old row before the write, so that the copy is the pre-update row. Heap says true, so heap events are unchanged.
Core assumes throughout that an UPDATE leaves the pre-update version
of a row readable through its locator: it re-fetches the old row by
TID when an AFTER trigger fires, for RETURNING OLD, and when
EvalPlanQual rechecks a joined row. Heap keeps the old version on
its page until VACUUM, so this is always true for heap. A table AM
that overwrites a row in place has replaced the old version by then,
so on such an AM AFTER ROW triggers and OLD transition tables see
the wrong values, and foreign-key enforcement misfires.
When the locator descriptor says old_version_retained = false:
- The executor copies the old row out of its slot before the
write, in ExecUpdate, MERGE and logical replication apply, so
RETURNING OLD and the AFTER ROW triggers read the copy.
- After-trigger events carry row copies (previous commit).
- EvalPlanQual carries an unlocked joined row whole (ROW_MARK_COPY)
instead of re-fetching it.
The AM must still keep a deleted row fetchable by its locator until
the deleting transaction ends, and a fetch by locator must return
the row's current version; amlocator.h states both. With the
version-naming bits from earlier in the series this is enough for
an in-place AM to give heap's answers.
Heap's descriptor says true, so no heap path changes.
A bitmap scan splits each locator into a group and a slot within it, the block and line pointer offset for heap, and BitmapAnd and BitmapOr combine their inputs group by group without touching the table. That is exact as long as every index entry for one row version names the same slot. A table AM that cannot promise that for some groups has no way to say so: an intersection would drop a row whose two entries name different slots, and a union over groups that do not match the AM's storage may need a recheck. Add two optional per-group callbacks to the locator descriptor. bitmap_and_inexact makes BitmapAnd union a flagged group's slots and mark it for recheck instead of intersecting it, and bitmap_or_inexact makes BitmapOr mark a flagged group for recheck. Each takes the group and a buffer the callback may keep pinned across calls on one scan. Every bitmap index scan records its table on the bitmap it fills (tbm_set_relation), so a combine at any depth of the bitmap tree consults the table's callbacks. Heap leaves both NULL here, so it takes the exact path it always did.
Add regression coverage for existing classic Heap-Only Tuple (HOT) update behavior, committed first so the behavioral changes in the later HOT-indexed commits are diffable against a known-good baseline. This commit adds tests so as to codify explicitly the HOT contract for the heap AM. The new hot_updates regression test exercises: - Basic HOT vs non-HOT update decisions - The all-or-none property across multiple indexes - Partial indexes and predicate handling - BRIN (summarizing) indexes allowing HOT updates - TOAST column handling with HOT - Unique constraint behavior - Multi-column indexes - Partitioned table HOT updates - HOT chain formation and the index-scan walk over a chain Authored-by: Greg Burd <greg@burd.me>
The executor needs to know which indexed attributes changed without assuming how a table AM represents their values. Add an optional modified_attrs callback and table_modified_attrs() wrapper that return the changed subset of a candidate bitmap. An AM without the callback conservatively reports every candidate as changed. This separates the comparison contract from the update interface. Heap implements the callback before the executor begins using it, so the update conversion preserves existing HOT and locking choices.
Provide heapam_modified_attrs(), the heap AM's implementation of the table AM modified_attrs callback introduced in the preceding commit. It answers "which of these attributes changed value between the old and new versions of a row?" using the heap's own notion of attribute equality (datum_image_eq on the logical slot values) and system-column semantics (the FirstLowInvalidHeapAttributeNumber offset convention and the tableoid special case). This is the heap-specific mechanism that previously lived in the executor as ExecCompareSlotAttrs(); moving it behind the table AM boundary keeps the executor agnostic of how any given AM stores its data or what its system columns mean, while the executor retains the policy decision of which indexes to maintain from the returned overlap. An AM that does not provide the callback is treated as though every candidate attribute changed (maintain all indexes).
Refactor executor update logic to determine which indexed columns have actually changed during an UPDATE operation rather than leaving this up to HeapDetermineColumnsInfo() in heap_update(). Finding this set of attributes is not heap-specific, but more general to all table AMs and having this information in the executor could inform other decisions about when index inserts are required and when they are not regardless of the table AM's MVCC implementation strategy. The heap-only tuple decision (HOT) in heap functions as it always has; what moves to the executor is only the determination of the "modified indexed attributes" (modified_idx_attrs). ExecUpdateModifiedIdxAttrs() replaces HeapDetermineColumnsInfo() and is called before table_tuple_update() crucially without the need for an exclusive buffer lock on the page that holds the tuple being updated. This reduces the time the buffer lock is held later within heapam_tuple_update() and heap_update(). Besides identifying the set of modified indexed attributes HeapDetermineColumnsInfo() was also partially responsible for the decision about what to WAL log for the replica identity key. That logic moves into heap_update() and into the replacement helper HeapUpdateModifiedIdxAttrs(), so simple_heap_update() and heapam_tuple_update() share the same logic since both call into heap_update(). Updates stemming from logical replication also use the new ExecUpdateModifiedIdxAttrs() in ExecSimpleRelationUpdate(). ExecUpdateModifiedIdxAttrs() uses ExecCompareSlotAttrs() to identify which attributes have changed and then intersects that with the set of indexed attributes to identify the modified indexed set, the modified_idx_attrs. This patch introduces a few helper functions to reduce code duplication and increase readability: HeapUpdateHotAllowable() and HeapUpdateDetermineLockmode(), used in both heap_update() and simple_heap_update(). heap_update() is now called with lockmode pre-determined and a boolean indicating whether the update may be HOT, both const. If during heap_update() the new tuple fits on the same page and that boolean is true, the update is HOT. So although the functions and timing of the HOT decision code have changed, none of the logic governing when HOT is allowed has changed. Development of this feature exposed nondeterministic behavior in three existing tests, which have been adjusted to avoid inconsistent results due to tuple ordering during heap page scans. Authored-by: Greg Burd <greg@burd.me> Discussion: https://www.postgresql.org/message-id/flat/78574B24-BE0A-42C5-8075-3FA9FA63B8FC%40amazon.com
An update that preserves existing row locators need not replace entries in indexes whose referenced attributes did not change. Separate this executor and index support from the heap storage implementation so other table access methods can use the contract. Track each index's referenced attributes independently of the narrow indexUnchanged hint. Carry a table-AM stale-entry signal through scan consumers, and provide an optional checked fetch for constraint enforcement. Access methods without that callback use the existing fetch operations. Distinguish index-only scans from requests to return index tuples when deciding whether to retain btree leaf pins. Prepare the selective-update counters and their flush paths here. The existing heap statistics call supplies false for the new argument until the heap implementation can produce such updates; this is the only heap change in this preparatory commit. Bump the statistics file format identifier for the added counters.
Define the on-disk representation a HOT-indexed update and its later prune/collapse produce, ahead of the code that reads or writes it: - HEAP_INDEXED_UPDATED (htup_details.h), the t_infomask2 bit marking a heap-only tuple whose producing UPDATE also changed an indexed column; and - access/hot_indexed.h, the inline fixed-size modified-attrs bitmap stored in the tail of such a tuple, plus the xid-free "collapse-survivor stub" format (HEAP_INDEXED_UPDATED with natts == 0, a forward link, and the segment's bitmap) and the accessors both share. The design itself is documented in README.HOT-INDEXED, which the next commit adds along with the code that writes this format. Co-authored-by: Greg Burd <greg@burd.me> Co-authored-by: Nathan Bossart <nathandbossart@gmail.com>
A selective-indexed update, added by the next commit, plants a fresh index entry that points at the new heap-only tuple mid-chain while every unchanged index still points at the chain root. The two entries name different line pointers on the same heap block, and an exact BitmapAnd intersection of the two indexes would drop the row. The disagreement is always within one block, because a HOT chain never leaves its page, so what BitmapAnd needs is a per-block answer that it can read without touching the heap. Add VISIBILITYMAP_LOCATOR_SPLIT, read lock-free like ALL_VISIBLE, with functions to set, clear and test it. The visibility map widens from 2 to 4 bits per heap block to carry it, and pg_upgrade rewrites each _vm fork from the old layout, a block at a time, as it did for 9.6's frozen bit. The rewrite is gated on VISIBILITYMAP_WIDTH_CHANGE_CAT_VER, which equals the catalog version this commit installs. The bit is not part of visibility and is excluded from VISIBILITYMAP_VALID_BITS. Nothing sets the bit yet; the next commit sets it and points heap's bitmap_and_inexact callback at it. Bump catalog version.
An update that changes only some indexed attributes can preserve unchanged index entries by keeping the new tuple in the HOT chain. Record each hop's modified attributes in an inline bitmap. New entries address the new heap-only tuple; unchanged entries continue to address earlier members of the chain. Have scans reject entries whose crossed attributes affect the index being scanned. Give constraint fetches the arriving index too, so unrelated changes do not request an unsafe expression-key recheck under a B-tree leaf lock. Keep the key comparison for overlapping attributes: away-and-back changes can leave the key equal. Include pruning and vacuum support with the first writer. Preserve needed dead members as forwarding stubs, including their saved bitmap widths, and include existing stubs when pruning again. Resolve sibling redirects against the completed plan so no surviving anchor bypasses a changed-key hop or points at reclaimed storage. Keep pages with stubs or selective redirects out of the all-visible state. Set the visibility map locator-split bit when indexes can name different offsets for the same row. Log and replay the update and prune changes, initializing a new visibility map page before setting its split bit when redo has no applicable full-page image. Resolve existing anchors under the reference snapshot during concurrent index validation. Accept unchanged suffixes without inserting duplicate entries, but reject crossed changes even when the key cycles back. Adapt index build, rewrite and bitmap scans to the new representation. Retain conservative fallbacks for catalog, replication and expression-index input changes. Cover repeated pruning, index scan order, direct anchors, concurrent index builds and unique checks here. Add recovery checks alongside the amcheck support they require in a later commit. Co-authored-by: Nathan Bossart <nathandbossart@gmail.com>
Expose the HOT-indexed counters maintained by the write path in pg_stat_all_tables and pg_stat_all_indexes. Provide pg_relation_hot_indexed_stats() to inspect chain composition, and document the counters in monitoring.sgml. Teach pg_surgery to handle HOT-indexed tuples and collapse stubs. Cover that handling and concurrent update, vacuum and scan behavior here. Add the pageinspect and recovery suites with the following amcheck support, since those tests also verify heap integrity.
Recognize heap-only tuples with direct index anchors, xid-free collapse stubs and multiple redirects to one live tuple as valid heap representations, while continuing to check their chains. For heapallindexed, fingerprint the stored leaf keys with the TID of the snapshot-visible version reached through a valid anchor. Reject anchors that cross changes to the index, including away-and-back changes. Probe with the scanned heap version rather than a rebuilt anchor, so different valid anchors do not report missing entries and a broken chain cannot borrow coverage from another tuple. Use this path for heap handler aliases too. Validate raw TID bounds, physical links, tuple headers and bitmap bounds under the buffer share lock before walking visibility. In particular, never let an invalid block number reach a read that could extend the heap. Respect saved stub widths and ordinary off-page update links. Cover rebuilt anchors, concurrent updates and vacuum, aliases, old stub widths and corrupt keys, TIDs and chains. Check backend survival and unchanged heap storage after corruption errors. Carry selective chains and collapsed stubs through pg_upgrade as well. Add the pageinspect and recovery suites here, after both statistics and selective-aware amcheck are available. These include repeated prune replay and visibility map initialization without an applicable full-page image, with full-page writes and consistency checking both on and off, and visibility map tail truncation.
Core disqualifies HOT-indexed updates unconditionally on the logical-replication apply path: HeapUpdateHotAllowable returns HEAP_UPDATE_ALL_INDEXES whenever IsLogicalWorker() is true, because a HOT-indexed update of a replica-identity attribute leaves a stale index leaf that the apply worker's replica-identity lookups would otherwise be exposed to. This commit relaxes that unconditional bail-out with a per-subscription hot_indexed_on_apply option (subhotindexedonapply: off / subset_only (default) / always). HeapUpdateHotAllowable now consults it when running in an apply worker, comparing the relation's indexed-attribute set against its primary-key attributes: "off" disqualifies HOT-indexed whenever any indexed attribute lies outside the primary key, "subset_only" requires the indexed attributes to be a subset of the primary key, and "always" applies no apply-path gating. The apply worker's replica-identity lookups (see RelationFindReplTupleByIndex) cope with the stale leaf, but only when the indexed attributes are a subset of the replica identity, which is what the default subset_only enforces. Wire the option through CREATE/ALTER SUBSCRIPTION, pg_subscription, pg_dump, and psql's \dRs+, and document it (create_subscription, alter_subscription, catalogs). Cover apply under each mode (039), apply under REPLICA IDENTITY FULL and a non-PK USING INDEX whose key is cycled (040), and decoding of HOT-indexed update chains (test_decoding). Authored-by: Greg Burd <greg@burd.me>
src/test/modules/test_locator_tableam provides table AMs that store
rows exactly as heap does and differ only in the locator properties
they declare, so the parts of the locator contract that heap does
not exercise run in the regression tests:
- heap_noretain declares old_version_retained = false. A
SnapshotAny fetch by locator follows the update chain to the
row's current version, which is what an AM that updates in place
hands back, so a caller that wrongly re-fetches an old row after
an UPDATE gets the new one and the test fails. It covers AFTER
ROW UPDATE and DELETE triggers, OLD and NEW transition tables,
deferred constraint triggers, foreign key CASCADE and RESTRICT,
RETURNING OLD, MERGE, the trigger-copy spill under a small
work_mem, temp_file_limit failures, savepoint rollback, and a
cross-partition update.
- heap_wide declares a 16-byte locator, so every path that builds
an index on it must fail, and pg_indexam_has_property() reports
can_var_locator false for every in-core index AM.
- heap_bigoffset declares offsets up to MaxOffsetNumber, so the
planner must not consider bitmap scans of it and GIN must refuse
to index it.
- heap_orrecheck sets bitmap_or_inexact, so BitmapOr consults it.
The executor's version-naming bits are not exercised, since all
these AMs use heap's lock and update code. This is test scaffolding
for reviewers and is not meant to be committed.
A/B and single-variant benchmark scripts for HOT-indexed updates: build two postgres variants, run pgbench workloads exercising classic-HOT, non-HOT, and HOT-indexed paths, and a self-contained bloat probe that reports the skip count (index writes avoided on unchanged indexes) and changed-index bounding. Not for merge; kept for evaluating the feature.
There was a problem hiding this comment.
🔍 OCR found 42 issue(s).
- 25 inline, 17 in summary (inline capped at 25)
⚠️ 1 warning(s) during review
📄 src/test/benchmarks/siu/scripts/run.sh (L38-L38)
Hardcoded default port 57480 (and 57481/57503 in the sibling scripts) risks colliding on shared/CI hosts and is non-portable test tooling. PostgreSQL test infrastructure assigns ports dynamically (PostgreSQL::Test::Cluster) precisely to avoid this. Fixed default ports in committed benchmark drivers invite flaky collisions.
📄 src/test/benchmarks/siu/scripts/run.sh (L102-L103)
sleep 2 is used as the readiness barrier after pg_ctl ... start. This is exactly the sleep-as-synchronization anti-pattern the project discipline forbids: on a slow/loaded host 2s may be insufficient and the subsequent psql calls will fail, and on a fast host it needlessly delays. pg_ctl start -w already waits for readiness (it is used here without -w); add -w or poll with pg_isready/SELECT 1 instead. Same pattern appears in soak.sh and split_ab.sh.
📄 src/backend/utils/cache/relcache.c (L5433-L5437)
On the first-use (uncached) path, the parse trees produced by stringToNode() and the strings from TextDatumGetCString() are palloc'd in the caller's current context and never freed, so they leak into whatever context is current at the call site. This happens once per index (subsequent calls hit the rd_indattr fast path), so it is a bounded one-time leak rather than a per-tuple leak. Still, since several callers invoke this from executor/per-tuple contexts (e.g. execIndexing.c) where the current context may be long-lived, consider pfree'ing the intermediate indexprs/indpred trees (and the CStrings) after pull_varattnos, matching how transient catalog parse trees are normally cleaned up. Low severity; confidence moderate.
📄 src/bin/pg_upgrade/pg_upgrade.h (L127-L127)
footgun / silent-corruption hazard [high confidence]: this threshold (202610201) must equal the CATALOG_VERSION_NO installed by the commit that widens the VM, but the tree's current CATALOG_VERSION_NO is 202611021. As written, any cluster initdb'd with a catversion in [202610201, 202611021) that still has a 2-bit VM would satisfy cat_ver >= VISIBILITYMAP_WIDTH_CHANGE_CAT_VER and NOT be rewritten -> a 2-bit _vm fork silently left in place on the new cluster, i.e. VM corruption. The XXX comment already acknowledges the committer must re-pick this value at commit time, and it is duplicated in t/010_vm_conversion.pl ($width_change_cat_ver = 202610201). At minimum keep the two copies in sync and make sure the final value matches the real widening catversion; consider a build-time check tying this to CATALOG_VERSION_NO so the two cannot drift silently.
📄 src/bin/pg_upgrade/file.c (L360-L360)
portability/convention [low confidence]: rewriteVisibilityMap() is compiled and used on all platforms (unlike copyFile(), which is wrapped in #ifndef WIN32), but it uses raw sequential read()/write() on the fds. The sibling I/O code added to this same module, slru_io.c, uses pg_pread()/pg_pwrite()/pg_pwritev_with_retry(). For consistency with that newer code and the tree's portability guidance, prefer pg_pread/pg_pwrite here too. Raw read()/write() with PG_BINARY is functionally fine for sequential access, so this is a convention note, not a correctness bug.
📄 src/test/benchmarks/siu/scripts/run.sh (L391-L393)
A/A control is broken: it mixes a tepid client against a master server, so it does not run "the master build as both arms" as the comment claims. start_pg/stop_pg/setup_schemas all take $b (= master in A/A), but run_one is invoked with $v (master, then tepid). Inside run_one, psql_as/pgbench_as resolve binaries via bin_of/LD_of on that first argument, so the second A/A arm drives pgbench/psql from $BENCH/tepid/... against a postmaster started from $BENCH/master/..., and it fails outright if no tepid build exists. Pass the resolved build ($b) into run_one instead of the logical label $v (keeping a separate label argument only for the CSV variant column), so the A/A noise-floor control actually uses the master binaries on both arms. (high confidence)
📄 src/test/benchmarks/siu/scripts/run.sh (L365-L366)
Comment is inaccurate: id=id is not "id % 1"; it is a self-assignment no-op update on the PK. Reword to match the code (e.g. "SET id=id so no indexed column changes"). (high confidence)
📄 src/backend/utils/cache/relcache.c (L5479-L5480)
The attrKind list in this header comment was updated for the HOT_BLOCKING -> INDEXED rename, but the newly added INDEX_ATTR_BITMAP_EXPRESSION kind (handled in the switch at the bottom of both this function and RelationGetIndexAttrBitmapNoCopy) is not documented here. Add a line describing it so the comment stays complete.
Confidence: high.
💡 Suggested change
Before:
* INDEX_ATTR_BITMAP_INDEXED Columns referenced by indexes
* INDEX_ATTR_BITMAP_SUMMARIZED Columns only included in summarizing indexes
After:
* INDEX_ATTR_BITMAP_INDEXED Columns referenced by indexes
* INDEX_ATTR_BITMAP_SUMMARIZED Columns only included in summarizing indexes
* INDEX_ATTR_BITMAP_EXPRESSION Columns referenced by expression indexes
📄 src/test/modules/test_locator_tableam/test_locator_tableam.c (L6-L7)
Stale comment: this file now defines four AMs (heap_noretain, heap_wide, heap_bigoffset, heap_orrecheck), all of which set index_build_range_scan = as_heap_index_build_range_scan. "Both AMs" here (line 6) and "both AMs present heap's routine" (line 25) date from an earlier two-AM version and have drifted from the code. Replace with wording that covers all four AMs.
💡 Suggested change
Before:
* Both AMs store rows exactly as heap does and differ from it only in the
* locator properties they declare; see amlocator.h.
After:
* All four AMs store rows exactly as heap does and differ from it only in the
* locator properties they declare; see amlocator.h.
📄 src/test/modules/test_locator_tableam/test_locator_tableam.c (L13-L13)
Over-length comment line (~96 chars), inconsistent with the ~72-75 char wrapping of the surrounding block. pgindent does not reflow comment prose, so this will not be auto-fixed; rewrap it.
💡 Suggested change
Before:
* what such an AM would hand back. The old version is still on the page, so heap's own
After:
* what such an AM would hand back. The old version is still on the
* page, so heap's own
📄 src/test/modules/test_locator_tableam/test_locator_tableam.c (L25-L26)
Same "both AMs" staleness as the file header: four AMs (not two) swap in heap's routine for the index build scan via as_heap_index_build_range_scan(). Reword to "all four AMs".
💡 Suggested change
Before:
* heap's index build scan uses it, so both AMs present heap's routine for the
* duration of that scan. CREATE INDEX CONCURRENTLY is not supported.
After:
* heap's index build scan uses it, so all four AMs present heap's routine for
* the duration of that scan. CREATE INDEX CONCURRENTLY is not supported.
📄 src/test/modules/test_locator_tableam/sql/width_check.sql (L17-L17)
wide_pk (line 17) is never dropped. It is fine here only because CREATE TABLE wide_pk (a int PRIMARY KEY) is expected to error (heap_wide refuses the PK index) and the table is never created. That is non-obvious; a one-line comment stating the CREATE is expected to fail (like the other error-path comments in this file) would prevent a future reader from "fixing" the cleanup list. Low confidence / style.
💡 Suggested change
Before:
CREATE TABLE wide_pk (a int PRIMARY KEY) USING heap_wide;
After:
-- this CREATE fails on the PK index, so the table is never created
CREATE TABLE wide_pk (a int PRIMARY KEY) USING heap_wide;
📄 src/test/subscription/t/039_hot_indexed_apply.pl (L340-L342)
This sub-test claims to exercise the _bt_check_unique stale-leaf recheck for all three modes, but only always actually does. On tab_uk the UNIQUE(payload, tag) constraint is NOT a subset of the PK (id), so under off and subset_only HeapUpdateHotAllowable() returns HEAP_UPDATE_ALL_INDEXES (non-HOT): the UPDATE writes a fresh tuple on a new TID with a fresh index entry and the payload=0 version becomes a dead tuple. The old leaf then points to a dead tuple, so the INSERT of the old key succeeds via the ordinary dead-tuple path, not via the stale-but-live recheck in _bt_check_unique. The comment on lines 340-342 ("The leaf entry ... may now be stale ... _bt_check_unique rechecks the live key") is therefore only accurate for always; the off/subset_only iterations would pass even if the recheck logic were broken, so they add no regression coverage for the path the comment describes. Either correct the comment to make clear only always exercises the recheck, or make the non-always iterations assert the distinct (dead-tuple) code path they actually traverse.
📄 src/test/subscription/t/039_hot_indexed_apply.pl (L2-L2)
File starts with a spurious blank line before the copyright notice. Sibling TAP tests (including the companion 040_hot_indexed_replica_identity.pl) place the # Copyright (c) ... notice on line 1. Remove the leading blank line for consistency.
📄 src/test/subscription/t/039_hot_indexed_apply.pl (L205-L208)
This case asserts only that the HOT-indexed delta is zero, discarding both the n_tup_upd and n_tup_hot_upd deltas (both bound to undef). An is(..., 0) assertion on a counter is satisfied vacuously if HOT-indexed never fires at all (e.g. the feature is reverted or regresses to never-eligible), so this sub-test cannot distinguish "off correctly suppresses HOT-indexed" from "HOT-indexed is broken everywhere". The comment even reasons that plain HOT must stay zero because payload is indexed on the subscriber, but that invariant is never checked. Capture and assert the deltas as a positive control (e.g. that n_tup_upd advanced by 20, and that HOT stays zero) so the case would actually catch a regression. Note the always case later does provide a positive control, which partially mitigates this.
📄 src/test/subscription/t/040_hot_indexed_replica_identity.pl (L73-L75)
Comment accuracy: tab_full has REPLICA IDENTITY FULL and no primary key or unique index (only the subscriber-side secondary indexes on b/c), so "the identity/PK row is matched again" is misleading - there is no PK here, and for tab_full the match is via full-row seqscan. Reword to reference only the replica identity.
📄 src/test/subscription/t/040_hot_indexed_replica_identity.pl (L114-L115)
Stale/misleading comment: "stubs recognised" references a concept ("stubs") that is not used or defined anywhere in this test. The check is simply that verify_heapam reports zero corruption on the HOT-indexed chains left by apply. Drop or correct the parenthetical.
| @@ -1243,7 +1243,7 @@ spawn_process(const char *cmdline) | |||
| char *cmdline2; | |||
|
|
|||
| cmdline2 = psprintf("exec %s", cmdline); | |||
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |||
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |||
There was a problem hiding this comment.
This execl -> execlp change is a behavioral and security-sensitive change that is unrelated to the stated developer-tooling scope of this patch, and it should be dropped (or split into its own justified patch).
shellprog is initialized from SHELLPROG, which is an absolute path on both build systems (/bin/sh in src/test/regress/meson.build, $(SHELL) in src/test/regress/GNUmakefile). With an absolute path, execl and execlp behave identically. The only effect of execlp is that, if shellprog ever becomes a bare name without a slash, it triggers a PATH search -- meaning the shell binary that gets run would depend on the invoker's PATH rather than the configured absolute path. For a test harness that spawns a shell this is a POLA violation and a potential footgun (a PATH-resolved shell could differ from the intended one). Revert to execl.
| execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL); | |
| execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL); |
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl | |||
| #!/usr/bin/env perl | |||
There was a problem hiding this comment.
Out-of-scope change that diverges from the tree convention. Every other Perl script under src/tools/ uses #!/usr/bin/perl (e.g. src/tools/copyright.pl, src/tools/git_changelog, src/tools/mark_pgdllimport.pl). Switching only pgindent to #!/usr/bin/env perl is an unrelated, inconsistent edit that does not belong in this feature series. Revert to keep the diff minimal and consistent.
| #!/usr/bin/env perl | |
| #!/usr/bin/perl |
| SET enable_indexscan = off; | ||
| SET enable_bitmapscan = on; | ||
| -- Must return the row. | ||
| SELECT count(*) AS bloom_bitmapand FROM bloom_hot_indexed WHERE bcol = 11 AND changed = 22; |
There was a problem hiding this comment.
The test asserts only count() = 1 and relies on soft enable_ GUCs, so it does not actually prove (a) that the UPDATE took the HOT-indexed path, nor (b) that a BitmapAnd plan was chosen. If the HOT-indexed path ever silently regresses (e.g. the new tuple no longer fits the chain-root block, so it is not HOT-indexed), or the planner falls back to a seqscan, this test still passes while no longer exercising the VISIBILITYMAP_LOCATOR_SPLIT / bitmap_and_inexact path the comment claims to guard. Sibling tests in this series (test_decoding/sql/hot_indexed.sql, pg_surgery, pageinspect) additionally assert n_hot_indexed > 0 via pg_relation_hot_indexed_stats() to pin the path. Consider adding that assertion here (and/or an EXPLAIN plan check) to keep the test from becoming vacuous. Low confidence / test-robustness note: the parallel cases in pageinspect/sql/hot_indexed_updates.sql use the same count-only pattern, so this is consistent with the series rather than a bloom-specific defect.
| # ========================================================================= | ||
|
|
||
| # Main entry: heap_update | ||
| break heapam.c:3210 |
There was a problem hiding this comment.
Hardcoded source line numbers in breakpoints are already stale and will silently rot. Verified against the current tree: heap_update() begins at heapam.c:3297 (the break heapam.c:3210 labeled "Main entry: heap_update" actually lands inside heap_delete's toast handling), and break heapam.c:4019 labeled "pure HOT" lands inside heap_lock_tuple's visibility-map clearing code. These line-number breakpoints are misleading now and will drift further with every edit. More broadly, this personal .gdbinit (along with .clangd, flake.nix, shell.nix, pg-aliases.sh) is developer scaffolding that does not belong in a PostgreSQL feature patch destined for pgsql-hackers -- it is non-minimal, out-of-scope churn in the repo root. Drop these from the patch (keep them local / gitignored).
| alias pg-full-clean='trash "$PG_BUILD_DIR" "$PG_INSTALL_DIR" 2>/dev/null || rm -rf "$PG_BUILD_DIR" "$PG_INSTALL_DIR"; echo "Build and install directories cleaned"' | ||
|
|
||
| # Database management | ||
| alias pg-init='trash "$PG_DATA_DIR" 2>/dev/null || rm -rf "$PG_DATA_DIR"; "$PG_INSTALL_DIR/bin/initdb" --debug --no-clean "$PG_DATA_DIR"' |
There was a problem hiding this comment.
Data-loss footgun: rm -rf "$PG_DATA_DIR" runs unconditionally after the trash fallback. These variables are only set inside the Nix dev shell (shell.nix sets PG_DATA_DIR=/tmp/test-db-$(basename $PWD)); if this alias is sourced outside that environment PG_DATA_DIR is unset and this becomes rm -rf "". The same pattern recurs in pg-full-clean ($PG_BUILD_DIR/$PG_INSTALL_DIR) and pg_clean_for_compiler. Guard the variable before any destructive rm -rf, e.g. [ -n "$PG_DATA_DIR" ] && .... (Note this whole file is also out-of-scope developer scaffolding that should not be committed to the repo in a feature patch.)
| alias pg-init='trash "$PG_DATA_DIR" 2>/dev/null || rm -rf "$PG_DATA_DIR"; "$PG_INSTALL_DIR/bin/initdb" --debug --no-clean "$PG_DATA_DIR"' | |
| alias pg-init='[ -n "$PG_DATA_DIR" ] && { trash "$PG_DATA_DIR" 2>/dev/null || rm -rf "$PG_DATA_DIR"; }; "$PG_INSTALL_DIR/bin/initdb" --debug --no-clean "$PG_DATA_DIR"' |
| indexedattrs = RelationGetIndexedAttrs(indexDesc); | ||
| indexInfo->ii_IndexNeedsUpdate = | ||
| row_moved || bms_overlap(indexedattrs, modified_idx_attrs); | ||
| bms_free(indexedattrs); |
There was a problem hiding this comment.
RelationGetIndexedAttrs() returns a freshly bms_copy'd Bitmapset (allocated in the current context), so this does one palloc+free per index per updated row. When row_moved is true, ii_IndexNeedsUpdate is unconditionally true, yet we still fetch and free the bitmap and the bms_overlap() result is discarded by the || short-circuit. For a non-HOT (row-moving) bulk UPDATE on a table with several indexes this is N redundant bms_copy/bms_free pairs per row on the UPDATE hot path. Short-circuit the fetch when row_moved is already known.
| indexedattrs = RelationGetIndexedAttrs(indexDesc); | |
| indexInfo->ii_IndexNeedsUpdate = | |
| row_moved || bms_overlap(indexedattrs, modified_idx_attrs); | |
| bms_free(indexedattrs); | |
| if (row_moved) | |
| indexInfo->ii_IndexNeedsUpdate = true; | |
| else | |
| { | |
| indexedattrs = RelationGetIndexedAttrs(indexDesc); | |
| indexInfo->ii_IndexNeedsUpdate = | |
| bms_overlap(indexedattrs, modified_idx_attrs); | |
| bms_free(indexedattrs); | |
| } |
| /* pg_stat_get_tuples_hot_indexed_upd_skipped */ | ||
| Datum | ||
| pg_stat_get_tuples_hot_indexed_upd_skipped(PG_FUNCTION_ARGS) | ||
| { |
There was a problem hiding this comment.
These two functions are byte-for-byte identical to the body produced by the existing PG_STAT_GET_IDXENTRY_INT64(stat) macro (defined a few lines below). They are hand-written only because the macro hard-codes a pg_stat_get_idx_ prefix, whereas the SQL names chosen here are pg_stat_get_tuples_hot_indexed_upd_{skipped,matched}. Consider either (a) renaming the SQL functions to pg_stat_get_idx_... so the macro can be reused, or (b) generalizing the macro to take an explicit C-function name. Reusing existing infrastructure keeps these index-entry accessors DRY and avoids the two copies drifting. (low confidence, maintainability)
| PgStat_Counter tuples_hot_indexed_upd_skipped; | ||
| PgStat_Counter tuples_hot_indexed_upd_matched; |
There was a problem hiding this comment.
Minor naming clarity: the field is named ..._matched but its own comment defines it as "inserted a fresh entry (a key attribute changed)" - i.e. the index key did not match / changed. "matched" reads as the opposite (key matched, no insert needed), which is actually what ..._skipped counts. Consider a name that reflects the documented semantics, e.g. tuples_hot_indexed_upd_inserted, to avoid confusing future readers. Low confidence / taste.
| sed -i.bak 's/\.bitmap_and_inexact = heap_bitmap_and_inexact,/.bitmap_and_inexact = NULL,\/* split_ab: split-off *\//' \ | ||
| "$hh" |
There was a problem hiding this comment.
This build step edits backend source by text substitution, hard-coding the exact initializer text .bitmap_and_inexact = heap_bitmap_and_inexact, from heapam_handler.c. It happens to match the current source character-for-character, but it couples the benchmark to internal struct-initializer formatting that the very same changeset introduces. A rename of the callback, a reformat by pgindent, or a change in member spacing will make the grep -q 'split_ab: split-off' guard abort the whole build. A benchmark that mutates backend source via sed to disable a feature is fragile and will silently rot. Consider gating the read-side behavior behind a runtime switch (e.g. a developer GUC) the benchmark can toggle, rather than patching source text.
| # JOBS parallel compile jobs (default: nproc or 8) | ||
| set -euo pipefail | ||
|
|
||
| BENCH=${BENCH:-/scratch/siu-bench} |
There was a problem hiding this comment.
Default paths embed a specific developer's machine layout (/scratch/siu-bench, $HOME/ws/postgres/tepid, /scratch/pg). Committing personal-filesystem defaults into src/test/benchmarks is at odds with the tree's conventions for test tooling (no hardcoded paths). If this harness is to live in-tree, the defaults should be derived relative to the checkout (e.g. a temp dir under the build/ or a TMPDIR-based location) rather than absolute scratch paths.
No description provided.