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}
| block_num int; | ||
| page_item record; | ||
| BEGIN | ||
| block_num := (target_ctid::text::point)[0]::int; |
There was a problem hiding this comment.
Parsing a tid by casting through text->point ((ctid::text::point)[0]::int) is fragile and non-idiomatic; it relies on tid's textual (block,offset) format being reparsable as a point and is not used anywhere else in the regression suite. This appears four times across these helpers. Prefer extracting block/offset without the intermediate point cast to make the intent clear and avoid coupling to text formatting.
| CREATE OR REPLACE FUNCTION get_hot_count(rel_name text) | ||
| RETURNS TABLE (updates BIGINT, hot BIGINT) AS $$ |
There was a problem hiding this comment.
get_hot_count is defined here and dropped at the end of the file, but it is never called anywhere in this test (it appears to be copy-pasted from the sibling hot_updates.sql, where it is used). Dead helper code violates the minimalism discipline and wastes reviewer time. Remove the definition (and its matching DROP FUNCTION get_hot_count(text); in the cleanup section), or use it.
| SELECT hot_idx AS hot_idx_before FROM get_hi_count('hi_brin') \gset | ||
| UPDATE hi_brin SET bcol = 200 WHERE id = 1; | ||
| SELECT pg_stat_force_next_flush(); | ||
| SELECT (hot - 0) > 0 AS classic_hot_fired, |
There was a problem hiding this comment.
(hot - 0) is a no-op subtraction that reads as leftover scaffolding. Simplify to hot > 0 for clarity; it asserts the same thing.
| SELECT (hot - 0) > 0 AS classic_hot_fired, | |
| SELECT hot > 0 AS classic_hot_fired, |
| Assert(bmnatts >= 0 && bmnatts <= relnatts); | ||
| if (bmnatts < 0 || bmnatts > relnatts) | ||
| bmnatts = relnatts; | ||
|
|
||
| HotIndexedBitmapUnion(crossed_bitmap, | ||
| HotIndexedGetModifiedBitmap(heapTuple->t_data, | ||
| heapTuple->t_len, | ||
| bmnatts), | ||
| bmnatts); |
There was a problem hiding this comment.
The defensive clamp bounds bmnatts to relnatts, but the subsequent read is still unbounded against the physical tuple length. HotIndexedGetModifiedBitmap() computes the bitmap pointer as (char *) htup + item_len - HotIndexedBitmapBytes(bmnatts), and HotIndexedBitmapUnion() then reads HotIndexedBitmapBytes(bmnatts) bytes from it. On a corrupt/short tuple where heapTuple->t_len (== item_len) is smaller than HotIndexedBitmapBytes(bmnatts), the pointer underflows below the tuple start and the union reads out of bounds, under a buffer share-lock. Since the stated intent of this block is exactly to be robust against a corrupt page ("On a corrupt page a stub's unbounded stashed natts could otherwise overflow..."), the clamp should also ensure the bitmap region fits within heapTuple->t_len (e.g. require t_len >= header_size + HotIndexedBitmapBytes(bmnatts) before reading, else skip). Confidence: moderate.
| /* See the comment on the stub case's crossed_bitmap use. */ | ||
| Assert(bmnatts >= 0 && bmnatts <= relnatts); | ||
| if (bmnatts < 0 || bmnatts > relnatts) | ||
| bmnatts = relnatts; |
There was a problem hiding this comment.
Same unbounded read as in the stub case: the clamp restricts bmnatts to relnatts but does not verify that heapTuple->t_len actually contains HotIndexedBitmapBytes(bmnatts) trailing bytes past the header. A corrupt tuple with a short t_len causes HotIndexedGetModifiedBitmap() to point before the tuple and HotIndexedBitmapUnion() to over-read. Consider bounding the read against t_len here as well. Confidence: moderate.
| if (crossed_bitmap) | ||
| { | ||
| int bmnatts = | ||
| HotIndexedTupleBitmapNatts(heapTuple->t_data); | ||
|
|
||
| /* See the comment on the stub case's crossed_bitmap use. */ |
There was a problem hiding this comment.
Style/DRY: the crossed-bitmap accumulation here is byte-for-byte identical to the block in the stub-handling branch above (compute bmnatts, Assert + clamp against relnatts, then HotIndexedBitmapUnion(HotIndexedGetModifiedBitmap(...))). Factor the two into a small static helper (e.g. accumulate_crossed_bitmap(crossed_bitmap, heapTuple, relnatts)) so the clamp/read logic lives in one place; this also ensures any future bounds fix is applied consistently. Confidence: high.
| if (chain_hops++ > maxoff_guard) | ||
| break; /* defend against a corrupt forward-link cycle */ |
There was a problem hiding this comment.
The guard silently breaks out of the loop on a suspected forward-link cycle, so a corrupt chain surfaces to the caller as "tuple not found" with no WARNING/elog. This can mask on-disk corruption from operators and diverges from what amcheck would expect to flag. Existing broken-chain handling in this loop also breaks silently, so this is consistent, but given the comment explicitly targets corrupt pages, consider at least an elog(DEBUG*/WARNING) (or a comment noting the deliberate silence) so the condition is diagnosable. Confidence: low.
| /* | ||
| * Under HOT-indexed updates, a stale btree entry can outlive heap | ||
| * pruning/vacuum of the page it targets; if the target offset is past | ||
| * the current max, treat as vacuumable instead of raising an | ||
| * index-corruption error. | ||
| */ | ||
| return; |
There was a problem hiding this comment.
These three previously-fatal ERRCODE_INDEX_CORRUPTED checks (offset past maxoff, LP_UNUSED target, and the heap-only-tuple check below) are now unconditional early returns for every relation. There is no gate on whether the relation actually participates in HOT-indexed updates, so genuine index corruption on ordinary (non-SIU) tables that used to be caught here will now be silently reclassified as 'vacuumable'. That masks real corruption and can drive wrong index-tuple deletion decisions. If the tolerance is required only under HOT-indexed, these returns should be gated on the relation/chain actually being HOT-indexed (rel is available in the caller); otherwise this is a real regression in corruption detection.
| elog(ERROR, "tuple concurrently deleted"); | ||
|
|
||
| return; |
There was a problem hiding this comment.
This return; is unreachable: elog(ERROR) does not return. It is dead code and will not survive pgindent/committer review. Drop it (elog(ERROR) already terminates control flow).
| elog(ERROR, "tuple concurrently deleted"); | |
| return; | |
| elog(ERROR, "tuple concurrently deleted"); |
| OffsetNumber cur = ItemIdGetRedirect(lp); | ||
| int64 len = 0; |
There was a problem hiding this comment.
Critical: the chain walk does not handle HOT-indexed collapse-survivor stubs, so it under-reports chain lengths and mis-counts chains once any collapse/prune has stubbed a member.
A stub is an LP_NORMAL item with HEAP_INDEXED_UPDATED set, natts==0, HEAP_HOT_UPDATED cleared, and its t_ctid.offnum forwarding to the next key tuple (see hot_indexed.h and pruneheap.c). Two consequences here:
- A redirect can point directly at a stub (heap_prune_chain: root_target = next_survivor, which becomes a stub offset when a dead-prefix member is stubbed). This code then does len++ for the stub and, because HEAP_HOT_UPDATED is cleared on a stub, breaks immediately -- counting a bogus length-1 chain and never reaching the real live tail via the forward link.
- A mid-chain stub (dead member replaced by a stub) likewise terminates the walk early instead of being stepped through.
Additionally, following t_ctid across a stub is doubly broken: a stub's t_ctid block half holds the write-time natts (HotIndexedStubSetBitmapNatts), not a block number, so the ItemPointerGetBlockNumber(&thdr->t_ctid) != blk guard would spuriously fire.
The canonical walk in pruneheap.c (heap_prune_chain, ~line 1700, and heap_prune_expected_xmin_for_chain, ~line 1572) steps transparently through stubs via HotIndexedHeaderIsStub()/HotIndexedStubGetForward() and does not count them as chain members. This diagnostic must do the same, or its reported avg_chain_len/max_chain_len/n_chains are systematically wrong on any relation that has been pruned. (high confidence)
| thdr = (HeapTupleHeader) PageGetItem(page, chain_lp); | ||
| len++; | ||
| if (!(thdr->t_infomask2 & HEAP_HOT_UPDATED)) | ||
| break; |
There was a problem hiding this comment.
Stub handling missing here too: ItemIdIsNormal(chain_lp) is true for a collapse-survivor stub, so len++ counts it as a chain member (stubs are explicitly "not a real tuple" and are signature-skipped everywhere else), and the subsequent HEAP_HOT_UPDATED check breaks the walk at the stub instead of following its forward link (HotIndexedStubGetForward). Detect stubs with HotIndexedHeaderIsStub() and step through them without incrementing len, mirroring heap_prune_chain().
| * avg_chain_len -- mean length across chains rooted at an LP_REDIRECT, | ||
| * derived by walking each redirect target to the end | ||
| * of its HEAP_HOT_UPDATED chain. |
There was a problem hiding this comment.
This header comment overstates accuracy: it claims avg/max chain length are "derived by walking each redirect target to the end of its HEAP_HOT_UPDATED chain" and that n_chains "Matches the number of distinct HOT chains that have survived the most recent prune." Because the walk stops at the first collapse-survivor stub (see the stub-handling issue below), both claims are false on any pruned/collapsed relation: chains are truncated at stubs and lengths are undercounted. Fix the walk to step through stubs, then the comment will hold.
| vmflags, | ||
| conflict_xid, | ||
| false, /* no cleanup lock required */ | ||
| false, /* no cleanup lock required: see below */ |
There was a problem hiding this comment.
The reworded comment promises "see below", but there is no cleanup-lock explanation anywhere below this line in lazy_vacuum_heap_page (the function only proceeds to WAL args, END_CRIT_SECTION, and VM/error bookkeeping). The actual rationale lives in the function header comment above ("a full cleanup lock is also acceptable") and in the caller. A dangling "see below" reference is worse than the original bare comment. Either drop the addition or point at the real location. Confidence: high.
| false, /* no cleanup lock required: see below */ | |
| false, /* no cleanup lock required */ |
| /* A reclaimable item is a classic LP_DEAD line pointer. */ | ||
| Assert(ItemIdIsDead(itemid) && !ItemIdHasStorage(itemid)); | ||
| ItemIdSetUnused(itemid); | ||
| unused[nunused++] = toff; |
There was a problem hiding this comment.
Removing Assert(nunused > 0) makes nunused == 0 reachable, but the unchanged comment 15 lines below still claims "The page is guaranteed to have had dead line pointers, so we always set PD_ALL_VISIBLE." If a queued page can now reach this function with zero reclaimable LP_DEAD items (the stated SIU motivation), that comment is stale and the if (RelationNeedsWAL) block would still emit a prune WAL record and MarkBufferDirty for a no-op page. Please confirm num_offsets == 0 is actually reachable here and, if so, update the stale comment and consider skipping the WAL/dirty work when nothing was reaped. Confidence: moderate.
|
📊 OCR posted 24/25 inline comment(s). 1 could not be posted
|
679390f to
816ce0e
Compare
Keeps gburd/postgres rebased hourly on postgres/postgres master with only .github/ changes on top (sync-upstream automatic + manual). Drops the bespoke Windows dependency-builder workflow: Windows is already built and tested in CI by upstream's pg-ci.yml (Visual Studio + MinGW meson jobs), so a separate dependency prebuild that nothing consumed was redundant.
The Open Code Review system: ocr-review and ocr-model-check workflows plus .github/ocr config (LiteLLM->Bedrock Claude Opus 4.8, rule.json, context.md, pg-history.py).
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>
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://commitfest.postgresql.org/patch/5556/ Discussion: https://www.postgresql.org/message-id/flat/78574B24-BE0A-42C5-8075-3FA9FA63B8FC%40amazon.com
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. README.HOT-INDEXED introduces the design and the relaxed classic-HOT invariant; later commits document the eligibility, write, read, and prune/collapse machinery in their own sections. Co-authored-by: Greg Burd <greg@burd.me> Co-authored-by: Nathan Bossart <nathandbossart@gmail.com>
Implement the HOT-indexed (Selective Index Update) feature on the foundation laid by the executor's modified-attribute identification. Eligibility: HeapUpdateHotAllowable returns a HeapUpdateIndexMode -- HEAP_UPDATE_ALL_INDEXES (not HOT; every index needs an entry), HEAP_UPDATE_HOT (classic HOT; no index needs an entry), or HEAP_SELECTIVE_INDEX_UPDATE (HOT chain, only the changed indexes maintained) -- computed from modified_idx_attrs and the per-relation indexed-attribute set (RelationGetIndexedAttrs). An UPDATE that changes a non-summarizing indexed attribute is HEAP_SELECTIVE_INDEX_UPDATE unless it is forced to HEAP_UPDATE_ALL_INDEXES by one of: every indexed attribute changed (nothing to skip), an attribute referenced by an expression index changed (expression-aware maintenance is not implemented yet), a system catalog, or the logical-replication apply gate (see the apply-gating commit). Partial indexes, exclusion constraints, partitioned tables, and non-btree access methods are all eligible -- the read path is access-method agnostic and the predicate column is part of the index's attribute set, so no carve-out is needed for them. Write path: the table-AM update contract carries modified attributes IN/OUT as a Bitmapset (on output the AM adds the whole-row sentinel, TableTupleUpdateAllIndexes, to signal "every index needs an entry"), and heap_update, for HEAP_SELECTIVE_INDEX_UPDATE, keeps the new version on the HOT chain while ExecInsertIndexTuples maintains only the indexes whose attributes changed. The new heap-only tuple records, in an inline bitmap in its tail, the attributes that changed at its hop. Only the stored tuple carries the bitmap and the HEAP_INDEXED_UPDATED flag; the caller's in-memory copy is left unmarked so the flag never promises a trailing bitmap that is not present. Read path: a chain walk to the live tuple unions the modified-attribute bitmaps of every hop it crosses. The index-access layer treats that crossed-attribute bitmap as the staleness authority: if it overlaps the arriving index's key columns the entry is stale and is dropped, and the row is re-supplied by the fresh entry the same update planted. The read path is access-method agnostic and needs no value recheck or leaf key: it is correct even when a key is cycled away and back, because the value-restoring update planted a fresh entry whose walk crosses no later key-changing hop. Unique checks are the one place that does compare values: _bt_check_unique fetches the conflicting tuple under SnapshotDirty and, on a crossed-hop arrival, compares the live tuple's current key against the arriving leaf with the index's own ordering procedure (_bt_heap_keys_equal_leaf, BTORDER_PROC under each column's collation). Using the opclass comparator -- not a bitwise image comparison -- distinguishes a stale ancestor leaf from a genuinely live duplicate (equal under the opclass even if not bitwise-identical) and, in the in-flight window of a restoring update, routes the stale-ancestor hit into _bt_doinsert's xwait so the duplicate is still caught. The comparison reads plain key columns straight from the heap slot; it never evaluates an indexed expression, because an UPDATE touching an expression-index attribute is ineligible for HOT-indexed, so an expression index is never the one receiving the fresh entry whose insert runs this check. Co-authored-by: Greg Burd <greg@burd.me> Co-authored-by: Nathan Bossart <nathandbossart@gmail.com>
A HOT-indexed (SIU) update's fresh entry in a changed index points at the new heap-only tuple, not at the chain root the way every other index entry for the same logical row does -- that positional distinction is what lets the read side judge staleness from the crossed-attribute bitmap without a value recheck. BitmapAnd/BitmapOr combine two indexes' TID sets at raw block+offset granularity in tidbitmap.c, before either side ever touches the heap. An unrelated, unchanged index's root-pointing entry for the same row will not agree with the changed index's fresh entry's offset, so an exact-mode intersection can silently drop a row that matches both predicates. Reported by Alexander Korotkov. Fixed by reserving one otherwise-unused bit (bit 14) in a stored TID's offset field, ItemPointerSIUMaybeStaleFlag. MaxOffsetNumber never needs more than 14 bits even at the largest configurable BLCKSZ, so the bit is free for any real offset; it is set only on the local TID copy handed to a HOT-indexed fresh entry's index_insert() call in ExecInsertIndexTuples, never on the slot's own tts_tid. ItemPointerGetOffsetNumber and ItemPointerCompare strip the bit by default (via the sentinel-safe ItemPointerOffsetNumberStrip, which leaves SpecTokenOffsetNumber/MovedPartitionsOffsetNumber -- both of which already have this bit set as part of their own encoding -- untouched) so every ordinary consumer keeps seeing the real offset; only ItemPointerGetOffsetNumberNoCheck exposes the raw value. The consumption is centralized in tbm_add_tuples(), the single choke point every amgetbitmap funnels exact heap TIDs through: it tests the raw flag (before the offset is stripped) and, when set, adds the whole page as lossy (tbm_add_page) instead of the single exact offset. Per tbm_intersect_page's own case analysis a lossy page survives any AND/OR against an exact-mode page and forces a recheck, so BitmapHeapScan resolves the chain and the existing heap-side crossed-attribute staleness test makes the final, correct call. Because this lives in tbm_add_tuples and not in each access method, no index AM needs to know about HOT-indexed chains: btree, hash, GIN, GiST, SP-GiST, contrib/bloom, and out-of-tree AMs are all correct with no AM-specific code, and a TID that never carries the flag takes the identical path it always did. GIN's own page-level lossy sentinel (ItemPointerIsLossyPage, an unrelated 0xffff marker used before a real heap TID is produced) is untouched; the new check only applies to genuine heap-item TIDs. The cost is precision, not correctness: any heap page carrying a live fresh entry contributes lossy (a whole-page recheck for all its tuples on that bitmap scan, not just the SIU row) until the chain collapses. It is bounded and self-healing -- prune/VACUUM collapse restores exact-mode entries. amcheck's heapallindexed verification fingerprints leaf tuples' stored TIDs and compares them against the plain heap TIDs it re-derives from the heap scan; verify_nbtree.c now strips the marker while fingerprinting so a fresh entry does not raise a spurious "lacks matching index tuple". pageinspect 1.14's bt_page_items reports the real offset in its ctid and htid columns (earlier versions surfaced the marker as an inflated offset) and adds a hot_indexed boolean column exposing the marker explicitly. Caught its own regression during development: the first cut masked bit 14 unconditionally, which corrupted SpecTokenOffsetNumber and MovedPartitionsOffsetNumber (both already have bit 14 set), silently breaking cross-partition-UPDATE conflict detection -- caught by the isolation suite (eval-plan-qual, merge-update, partition-key-update). Fixed by gating the strip on the value being below the sentinel range. Regression coverage (BitmapAnd/BitmapOr across a changed+unchanged index for every access method SIU exercises, plus a bloom case in contrib/bloom and heapallindexed on a changed index) is added alongside the rest of the HOT-indexed test suite.
A HOT-indexed update plants index entries that point at mid-chain heap-only tuples, so a dead chain member cannot simply be removed: a not-yet-swept index entry may still arrive at it, and the per-hop modified-attrs bitmap on it is what a reader unions to judge staleness. Teach prune to collapse a dead chain prefix into xid-free forwarding stubs: each preserved dead key tuple is rewritten in place to a stub (frozen, natts == 0, HEAP_INDEXED_UPDATED, forwarding via t_ctid.offnum) that keeps its segment's modified-attrs bitmap, and a member whose attributes are wholly subsumed by later hops is reclaimed instead. Readers step through stubs transparently and still cross every surviving hop's bitmap. The collapse back to classic HOT is driven by prune: once a chain is fully dead, a later prune (heap_prune_chain / heap_prune_chain_find_live) reclaims its members and re-points the root redirect straight at the first live tuple. VACUUM's index cleanup sweeps the stale leaves; its second pass (lazy_vacuum_heap_page) does the usual LP_DEAD -> LP_UNUSED conversion and leaves the HOT-indexed collapse to prune. The collapse reuses the existing prune/freeze WAL via an xlhp_prune_items sub-record carrying the (offset, forward) stub pairs; no new record type is introduced. A page that still carries a preserved stub (or a redirect that forwards into a live HOT-indexed member) is kept non-all-visible so index-only scans heap-fetch through the chain; heap_page_would_be_all_visible recognizes both the redirect-to-SIU and the stub case explicitly. Co-authored-by: Greg Burd <greg@burd.me> Co-authored-by: Nathan Bossart <nathandbossart@gmail.com>
verify_heapam must not flag the HOT-indexed artifacts as corruption: a live HEAP_INDEXED_UPDATED heap-only tuple whose mid-chain line pointer is preserved because an index entry still points at it, an xid-free collapse-survivor stub, and more than one LP_REDIRECT forwarding to the same live tuple are all legitimate. Recognize them and continue checking the rest of the chain. Cover this with an amcheck regression test, and add a pg_upgrade test that carries a relation with HOT-indexed chains, an ABA-cycled indexed column, an out-of-line indexed column, and VACUUM-collapsed stubs across an upgrade, verifying the data, verify_heapam, bt_index_check, and the chain scans on the new cluster. Authored-by: Greg Burd <greg@burd.me>
Expose the HOT-indexed activity counters maintained by the write path: pg_stat_all_tables.n_tup_hot_indexed_upd, the per-index n_tup_hot_indexed_upd_matched / n_tup_hot_indexed_upd_skipped counters in pg_stat_all_indexes, and pg_relation_hot_indexed_stats() reporting per-relation HOT-indexed chain composition. Document them in monitoring.sgml and the README. With statistics, prune/collapse, and amcheck recognition all in place, add the full feature test suite, which uses those facilities to verify behavior: - hot_indexed_updates (regression): eligibility and classification; selective maintenance across multiple/composite indexes; the crossed-attribute read path for equality, range, and inequality scans; a key cycled away and back (ABA), including across two distinct live rows; TOASTed indexed columns; partial-index predicate flips (key and non-key predicate columns); trigger-modified indexed columns; exclusion-constraint tables; partitioned tables; non-btree access methods (hash, GIN, GiST); a UNIQUE index on a type where image equality differs from operator equality; CREATE INDEX / REINDEX and DROP INDEX over live chains; prune reclamation, stub mixes, and re-collapse across partial VACUUMs; the never-all-visible guard; and DDL after a chain exists (ADD COLUMN crossing a bitmap-size boundary, DROP COLUMN). - hot_indexed_adversarial (isolation): concurrent UPDATE / VACUUM / prune and index scans, key cycling, aborts, and reader consistency across a concurrent collapse. - 054_hot_indexed_recovery (recovery): WAL replay of the chain and its collapse under wal_consistency_checking. - pg_surgery handling of HOT-indexed tuples and collapse-survivor stubs. Authored-by: Greg Burd <greg@burd.me>
A HOT-indexed update of a replica-identity attribute on a subscriber leaves a stale index leaf that the apply worker's replica-identity lookups must tolerate -- which they do, but only when the subscriber's indexed attributes do not extend past the columns those lookups key on. Add the per-subscription hot_indexed_on_apply option (subhotindexedonapply: off / subset_only (default) / always) and have HeapUpdateHotAllowable consult 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. 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>
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 91 issue(s).
- 25 inline, 66 in summary (inline capped at 25)
📄 src/backend/access/heap/vacuumlazy.c (L2723-L2725)
The comment claims this pass "prefer[s] a cleanup lock ... as the first pass does", but got_cleanup_lock is only consulted for the exclusive-lock fallback on the next line and is never threaded into lazy_vacuum_heap_page() to enable any cleanup-only work (stub/redirect reclamation is explicitly deferred to a later prune). As written, both the cleanup-lock and exclusive-lock paths do identical work, so acquiring a cleanup lock here buys nothing functionally while risking extra pin-wait contention versus the previous unconditional exclusive lock. Either use got_cleanup_lock to gate opportunistic reclamation, or drop the ConditionalLockBufferForCleanup attempt and keep the plain exclusive lock the comment already says suffices.
📄 src/backend/access/index/indexam.c (L713-L714)
Performance: on the HOT-indexed recheck path this does a bms_copy (inside RelationGetIndexedAttrs) plus bms_free for every fetched tuple whose chain walk crossed a hop -- palloc/free churn on the index-scan hot path under a HOT-indexed-heavy workload. The copy exists only to survive an intervening AcceptInvalidationMessages(), but there is none between here and the loop below, so the relcache-owned rd_indattr could be tested read-only instead. Consider adding a helper that tests the crossed uint8[] bitmap directly against the cached bitmap (rd_indattr) without copying/freeing.
📄 src/backend/access/heap/pruneheap.c (L2591-L2597)
This comment directly contradicts the new comment added in log_heap_prune_and_freeze at the no-cleanup-lock branch, which states the opposite: "we can ... re-point redirects. The latter happens when vacuum's second pass reclaims a collapsed HOT-indexed chain and re-points the root redirect at first_live." Both were added by this patch and describe the same scenario (no cleanup lock; here lp_truncate_only is set from (xlrec.flags & XLHP_CLEANUP_LOCK) == 0 in the redo caller). If the log_heap_prune_and_freeze claim is the accurate one, then a no-cleanup-lock WAL record can carry nredirected > 0, and on replay heap_page_prune_execute is invoked with lp_truncate_only=true and nredirected > 0, tripping this retained assertion -- a latent assertion-failure/crash-on-replay footgun. At most one of the two comments can be true; reconcile them and make the two assertions consistent with the actual invariant. Also, "in this series" is PR-framing wording that should not survive into committed code.
📄 src/backend/access/heap/pruneheap.c (L2286-L2287)
Two consecutive blank lines were introduced here; PostgreSQL style / pgindent uses a single blank line between functions. Remove one blank line to keep a minimal, clean diff.
💡 Suggested change
Before:
return true;
}
After:
return true;
}
📄 src/backend/access/heap/pruneheap.c (L2772-L2772)
This is a trailing-period-only edit on an otherwise untouched comment. It is unrelated churn that inflates the diff for no functional reason; revert to the original text to keep the diff minimal.
💡 Suggested change
Before:
/* Setting LP_DEAD to LP_UNUSED in vacuum's second pass. */
After:
/* Setting LP_DEAD to LP_UNUSED in vacuum's second pass */
📄 src/backend/access/nbtree/nbtinsert.c (L27-L27)
The executor/tuptable.h include is redundant: access/tableam.h (included on line 22) already includes executor/tuptable.h, which provides TupleTableSlot, ExecClearTuple, and slot_getattr. Keeping the diff minimal argues for dropping this line; an explicit include is defensible only if the intent is to not rely on the transitive path. Low-confidence style nit, not correctness.
📄 src/backend/catalog/indexing.c (L21-L21)
These three new includes are unused and are diff noise. access/tableam.h (no table_*/TAM symbol is referenced in this file), nodes/bitmapset.h (no Bitmapset/bms_* used), and utils/relcache.h (its only relevant symbol here, RelationPtr, is already provided transitively via the pre-existing utils/rel.h, which #includes utils/relcache.h; and that RelationPtr use isn't even new to this patch). The only new dependency this hunk introduces is the changed simple_heap_update signature, which is already declared in access/heapam.h (already included). Drop these three includes to keep the diff minimal. (high confidence)
📄 src/backend/executor/execReplication.c (L971-L971)
This assertion is user-reachable and will fire on an assert-enabled build. ExecUpdateModifiedIdxAttrs -> ExecCompareSlotAttrs keeps attribute 0 (whole-tuple reference) in the returned bitmap: when a subscriber table has an index whose expression references a whole-row Var (e.g. CREATE INDEX ON t ((t.*::text))), RelationGetIndexAttrBitmap(INDEX_ATTR_BITMAP_INDEXED) records attribute 0 via pull_varattnos, and bitmap index 0 - FirstLowInvalidHeapAttributeNumber equals the TableTupleUpdateAllIndexes sentinel (7). ExecCompareSlotAttrs continues without deleting it for attrnum == 0, so modified_idx_attrs can already contain the sentinel here.
This directly contradicts the documented contract of table_tuple_update/simple_table_tuple_update (see tableam.h: "*modified_attrs may already contain TableTupleUpdateAllIndexes on input when an index expression references a whole-row Var") and the mirror path ExecUpdateAct in nodeModifyTable.c, which explicitly treats the sentinel's presence on input as harmless rather than asserting it away. Assert() must be reserved for can't-happen invariants, not a user-reachable configuration. Remove this assertion; the code below already handles the sentinel correctly by computing all_indexes from it.
📄 src/backend/executor/execTuples.c (L2104-L2104)
Stale/incorrect function reference in comment: HeapDetermineColumnsInfo() does not exist anywhere in the tree. The actual heapam counterpart is HeapUpdateModifiedIdxAttrs() (which uses heap_attr_equals() internally) -- see the cross-reference comment at heapam.c that names ExecCompareSlotAttrs and HeapUpdateModifiedIdxAttrs as the mirrored pair. This drifted comment should name the real function so the documented equivalence is verifiable.
💡 Suggested change
Before:
* This function and heap_attr_equals()/HeapDetermineColumnsInfo() are
After:
* This function and heap_attr_equals()/HeapUpdateModifiedIdxAttrs() are
📄 src/backend/executor/execIndexing.c (L481-L488)
DRY / maintenance footgun: these two index_insert() calls are identical 7-argument invocations that differ only in the TID argument (&siu_tid vs tupleid). A future signature change must be kept in sync across both copies. Select the TID with a single pointer and keep one call, e.g.:
ItemPointerData siu_tid;
ItemPointer insert_tid = tupleid;
if ((flags & EIIT_IS_HOT_INDEXED) && !indexInfo->ii_Summarizing)
{
siu_tid = *tupleid;
ItemPointerSetSIUMaybeStale(&siu_tid);
insert_tid = &siu_tid;
}
satisfiesConstraint =
index_insert(indexRelation, values, isnull, insert_tid,
heapRelation, checkUnique, indexUnchanged, indexInfo);This is behavior-preserving and collapses the duplicated block.
📄 src/backend/executor/execIndexing.c (L917-L922)
This weakens a corruption-detection invariant. Once a stale self-arrival for this TID has been seen (found_self_siu_hit latched true, or the current arrival is stale), any number of subsequent self-arrivals for the same TID are silently tolerated -- including genuine duplicate-TID corruption (two non-stale entries for the same heap TID). Sequence S, N, N: the first stale arrival latches found_self_siu_hit, the second (legit direct) arrival is tolerated, and a third non-stale arrival -- which would be real corruption -- also continues instead of raising. Consider tightening so the tolerance covers exactly the expected multiplicity (at most one non-stale direct entry plus the stale chain-walk entries), rather than an unbounded set, so real duplicate-TID corruption is still caught.
📄 src/backend/executor/execIndexing.c (L1172-L1175)
Minor: when all_indexes is true, ii_IndexNeedsUpdate is unconditionally true, yet RelationGetIndexedAttrs() (which does a bms_copy of the cached bitmap) and the paired bms_free() still run for every index on every updated row. RelationGetIndexExpressions/Predicate are only walked once and cached, so the cost is the per-row copy/free churn. Skip the copy when all_indexes is set, e.g. compute indexedattrs only in the else branch.
📄 src/backend/executor/nodeIndexscan.c (L162-L166)
InstrCountFiltered2 increments nfiltered2, which for an IndexScan node EXPLAIN reports as "Rows Removed by Index Recheck" (see explain.c: show_instrumentation_count("Rows Removed by Index Recheck", 2, ...), gated on indexqualorig). HOT-indexed-stale drops are not index rechecks, so they either silently inflate the recheck count (when a recheckable qual exists) or vanish entirely (when it does not). This misattributes the counter and is misleading in EXPLAIN ANALYZE. Consider a dedicated counter/label for stale-entry drops, or at minimum document the conflation.
Confidence: high.
📄 src/backend/executor/nodeIndexscan.c (L300-L304)
Same counter misattribution as in IndexNext: InstrCountFiltered2 feeds nfiltered2, shown as "Rows Removed by Index Recheck" for IndexScan nodes. Distance-ordered (kNN) drops reported here will be mislabeled as index rechecks in EXPLAIN ANALYZE.
Confidence: high.
📄 src/backend/nodes/makefuncs.c (L848-L849)
Removing the init of ii_CheckedUnchanged is correct (the field no longer exists in IndexInfo). However, ii_IndexUnchanged is still a live field in struct IndexInfo (execnodes.h) and is read on the UPDATE path (execIndexing.c: indexInfo->ii_IndexUnchanged at the aminsert hint). Dropping its explicit = false here is only safe because makeNode() uses palloc0. This creates a style inconsistency: makeIndexInfo still explicitly initializes sibling bool flags (e.g. ii_BrokenHotChain). Since ii_IndexUnchanged is populated by ExecSetIndexUnchanged before use and defaults to false via palloc0, there is no bug — but for a minimal, consistent diff, keep the explicit n->ii_IndexUnchanged = false; (only ii_CheckedUnchanged should be removed). Confidence: high that this is harmless; low that it warrants a change.
💡 Suggested change
Before:
- n->ii_CheckedUnchanged = false;
- n->ii_IndexUnchanged = false;
After:
n->ii_IndexUnchanged = false;
📄 src/backend/replication/logical/worker.c (L6087-L6087)
Contradictory comments in the same block. The leading comment says this returns the "cached" apply mode, but the inner comment states it derives "directly from MySubscription rather than caching, so there is no second copy to keep in sync." There is no cache here — the value is read live from MySubscription. Drop "cached" from the header comment so it describes what the code actually does. (The two comment blocks are also largely redundant — both restate the "readers outside worker.c avoid Subscription-struct visibility" rationale; consider collapsing to one.)
💡 Suggested change
Before:
* Return the cached HOT-indexed apply mode of the current logical replication
After:
* Return the HOT-indexed apply mode of the current logical replication
📄 src/backend/nodes/tidbitmap.c (L379-L383)
This 21-line header addition is disproportionately verbose for this subsystem and editorializes ("makes the final call", "takes the identical path it always did", the whole final paragraph justifying the choke-point choice). For -hackers, comments should be tight and explain why, not narrate the design rationale at length. Consider trimming to a few lines: the invariant (a SIU-flagged entry points mid-chain, so an exact offset match under AND/OR could drop a row), the fix (add the page lossy so it survives AND against exact pages and forces a recheck), and a pointer to ItemPointerSIUMaybeStaleFlag. Low confidence / style.
📄 src/backend/executor/nodeModifyTable.c (L2521-L2522)
Memory leak on the concurrent-update retry and on all non-TM_Ok returns. modified_attrs is allocated by ExecUpdateModifiedIdxAttrs() before table_tuple_update() and is only freed in ExecUpdateEpilogue(). But the epilogue is skipped whenever table_tuple_update() returns a non-Ok result: in ExecUpdate() the TM_Updated case does goto redo_act, re-entering ExecUpdateAct() which overwrites the pointer here (modified_attrs = NULL) without freeing the previous allocation; the TM_Deleted/TM_SelfModified cases return NULL; and the MERGE lmerge_matched path only calls the epilogue when result == TM_Ok, discarding the stack updateCxt on goto lmerge_matched. Each such event leaks one Bitmapset in the per-query context, defeating the stated intent of the bms_free in the epilogue. Free any prior bitmap before resetting, e.g. bms_free(updateCxt->modified_attrs); updateCxt->modified_attrs = NULL;
💡 Suggested change
Before:
/* Reset any state left over from a previous call */
updateCxt->modified_attrs = NULL;
After:
/* Free any bitmap left over from a previous (retried) call and reset */
bms_free(updateCxt->modified_attrs);
updateCxt->modified_attrs = NULL;
📄 src/backend/executor/nodeModifyTable.c (L219-L219)
Typo in this declaration-site doc comment: "UPDATE statment" should be "UPDATE statement".
💡 Suggested change
Before:
* are in the UPDATE statment and are known to be referenced by at least one
After:
* are in the UPDATE statement and are known to be referenced by at least one
📄 src/backend/storage/page/itemptr.c (L67-L68)
This changes user-visible semantics of the SQL tid type. ItemPointerCompare is the implementation behind tideq/tidne/tidlt/.../bttidcmp (see src/backend/utils/adt/tid.c) and thus the tid_ops btree opclass. After this change any two tid values whose offsets differ only in bit 14 compare equal / mis-order: e.g. '(0,16384)'::tid = '(0,0)'::tid now returns true, and a btree index on a tid column, ORDER BY tid, DISTINCT/GROUP BY tid, etc. silently treat them as equal. A real heap ctid can never carry offset 16384 (MaxHeapTuplesPerPage << 1<<14), so the practical exposure is essentially nil, but this is a backward-compat behavior change to a first-class user type that accepts arbitrary offsets. This is intentional per README.HOT-INDEXED, but the choice to overload the general-purpose comparator (rather than stripping only at the nbtree/heap TID call sites) deserves explicit justification and, ideally, a note in the docs/commit message; a reviewer on -hackers will raise exactly this. Confidence: moderate.
📄 src/include/access/amapi.h (L30-L32)
This is an unrelated whitespace-only change: it removes a blank line in a section untouched by the HOT-indexed-update feature. PostgreSQL patch hygiene requires a minimal diff — reformatting untouched lines that the change does not need is a common rejection reason on pgsql-hackers and adds needless merge-conflict risk. Drop this hunk. (high confidence)
💡 Suggested change
Before:
typedef struct IndexInfo IndexInfo;
/*
After:
typedef struct IndexInfo IndexInfo;
/*
📄 src/bin/pg_upgrade/t/009_hot_indexed.pl (L28-L28)
This test requires the amcheck contrib extension (CREATE EXTENSION amcheck; verify_heapam/bt_index_check below), but under the autotools/make check path amcheck will not be present in the temporary install. Every other test directory that uses amcheck adds contrib/amcheck to its Makefile's EXTRA_INSTALL (e.g. src/test/recovery/Makefile, src/test/modules/nbtree/Makefile), but src/bin/pg_upgrade/Makefile currently only lists contrib/test_decoding src/test/modules/dummy_seclabel src/test/modules/test_extensions and is not modified in this change. As a result CREATE EXTENSION amcheck will fail and this test will error out under make check (the meson build is unaffected). Add contrib/amcheck to EXTRA_INSTALL in src/bin/pg_upgrade/Makefile. (high confidence)
📄 src/bin/pg_upgrade/t/009_hot_indexed.pl (L44-L44)
The exact-count assertions below (the '1'/'0' index-scan results and the '0' verify_heapam results) rely on all four single-column UPDATEs staying HOT-indexed and producing the described chain shape. Whether an update stays on-page depends on fillfactor, tuple size and free space, which vary with BLCKSZ. With 2000-byte STORAGE EXTERNAL rows and fillfactor=50 on a non-default block size (e.g. 4kB or 32kB buildfarm animals), page-fill behaviour can differ and yield non-HOT updates / different chain shapes, making these assertions potentially non-deterministic. Consider making the setup robust to non-default block sizes, or document/verify the assumption holds across supported BLCKSZ. (moderate confidence)
📄 src/backend/utils/cache/relcache.c (L5396-L5401)
The cache never engages for an index whose referenced-attribute set is empty. bms_copy(NULL) returns NULL, so for a constant-expression index (e.g. CREATE INDEX ON t ((1))) or any index/predicate that references no heap Var, attrs is NULL and rd_indattr stays NULL. The fast-path guard if (indexRel->rd_indattr != NULL) above then fails on every subsequent call, so this function re-parses the raw catalog trees via stringToNode(TextDatumGetCString(...)) each time -- leaking the parsed List and CString into the caller's context -- on the per-tuple hot paths (execIndexing.c, indexam.c). Use a separate boolean/valid flag, or an initialized-once sentinel, to distinguish "empty result" from "not computed" so the cache is populated even when the bitmap is empty.
📄 src/include/access/heapam.h (L511-L512)
Minor alignment inconsistency between these two new declarations: the continuation line for HeapUpdateDetermineLockmode has one extra leading space compared to HeapUpdateHotAllowable, even though both preceding lines start with extern + a 22-char function name, so the second-parameter column is identical. Run pgindent to normalize (the tree gates on a clean git diff --check / pgindent).
💡 Suggested change
Before:
extern LockTupleMode HeapUpdateDetermineLockmode(Relation relation,
const Bitmapset *modified_idx_attrs);
After:
extern LockTupleMode HeapUpdateDetermineLockmode(Relation relation,
const Bitmapset *modified_idx_attrs);
📄 src/include/access/htup_details.h (L292-L297)
This flag is overloaded: besides the live HOT/SIU tuple case described here, prune also sets it on xid-free forwarding "stub" line pointers that have natts == 0 (see HotIndexedHeaderIsStub() and the header comment in access/hot_indexed.h). Since this is the canonical definition site for the flag, consider noting the stub meaning (or pointing to hot_indexed.h) so readers don't assume HEAP_INDEXED_UPDATED always implies a live, data-bearing tuple. Documentation nit, not a correctness issue. Confidence: high.
📄 src/include/catalog/catversion.h (L60-L62)
Do not modify catversion.h in a patch. Bumping CATALOG_VERSION_NO (and adding a placeholder comment about it) is the committer's job at push time. Including it here guarantees merge/rebase conflicts against every other in-flight catalog patch and is the single most common author mistake in catalog patches. Even though this hunk only adds a comment rather than changing the number, touching this file at all is wrong -- drop the entire change. The fact that a new catalog column was added belongs in the commit message, not as a comment in this file.
📄 src/include/access/relscan.h (L148-L148)
These five new field comments carry multi-paragraph cross-module invariant proofs (bitmap-union completeness, ABA cycling, MVCC-safety of the kill) that are duplicated near-verbatim in index_fetch_heap() in indexam.c and in README.HOT-INDEXED. Header field comments should say concisely what the field is; the reasoning belongs in one authoritative place (the .c consumer / README). Duplicated prose in a widely-included header will drift from the implementation. Suggest trimming each to a one- or two-line description and pointing at indexam.c / README.HOT-INDEXED for the invariant. (low confidence, maintainability nit.)
📄 src/include/catalog/pg_subscription.h (L236-L236)
American English is the PostgreSQL convention throughout the tree; "behaviour" should be "behavior". This is the only such spelling in this header.
Confidence: high.
💡 Suggested change
Before:
* behaviour before this option was introduced.
After:
* behavior before this option was introduced.
📄 src/include/access/tableam.h (L489-L494)
The callback contract here documents only scan->xs_hot_indexed_recheck, but the heap AM's index_fetch_tuple implementation also populates two other new IndexFetchTableData fields that the index-access layer consumes: xs_hot_indexed_crossed (the union of crossed per-hop modified-attrs bitmaps, used by indexam.c to judge staleness) and xs_prefix_all_dead (used to decide whether the arriving leaf can be killed). An out-of-tree AM author reading only this contract would not know these must also be set/left in a consistent state on every return path. Please either document all three fields the AM is responsible for, or explicitly state that the others are optional/for-heap-only, so the callback contract is complete.
📄 src/include/replication/logicalworker.h (L28-L29)
The comment says "cached hot_indexed_on_apply mode", but the implementation in worker.c does the opposite: its comment says "Derive directly from MySubscription rather than caching, so there is no second copy to keep in sync". The header comment is inaccurate and will mislead readers into thinking a cache exists. Drop "cached". (moderate confidence)
💡 Suggested change
Before:
* Accessor for the cached hot_indexed_on_apply mode of the current apply
* worker's subscription. Returns a LOGICALREP_HOT_INDEXED_* code (see
After:
* Accessor for the hot_indexed_on_apply mode of the current apply
* worker's subscription. Returns a LOGICALREP_HOT_INDEXED_* code (see
📄 src/include/pgstat.h (L772-L781)
The line-continuation backslashes in these two new macros are misaligned versus the surrounding macros (e.g. pgstat_count_index_tuples just above), so this block will not pass pgindent cleanly. Two issues: (1) the longer macro names push the trailing backslash on the #define line to a different column than the neighbours; (2) the body lines with the long counter names have the backslash jammed directly against the ++; with no tab, breaking the aligned backslash column used throughout this block. Run pgindent and git diff --check, and align all continuation backslashes to the same column as the adjacent macros.
📄 src/include/utils/rel.h (L225-L229)
This warning is misleading. rd_indattr is a private relcache cache field with no external consumers (grep finds it only in relcache.c), and its sole accessor RelationGetIndexedAttrs() already returns bms_copy(indexRel->rd_indattr) unconditionally -- it never hands out the cached pointer. Documenting a "consumers must bms_copy" contract for a field nobody outside relcache reads describes behavior that doesn't exist and invites the reader to look for a borrowed-pointer path that isn't there. Trim the comment to describe what the field is (a lazily-built cache of one index's referenced heap attrs, freed with rd_indexcxt) and drop the AcceptInvalidationMessages sentence. Confidence: high.
💡 Suggested change
Before:
* RelationGetIndexedAttrs() and cached in rd_indexcxt. Consumers must
* bms_copy before relying on the pointer beyond any potential
* AcceptInvalidationMessages() call.
*/
Bitmapset *rd_indattr;
After:
* RelationGetIndexedAttrs() and cached in rd_indexcxt; freed on relcache
* rebuild along with rd_indexcxt.
*/
Bitmapset *rd_indattr;
📄 src/include/utils/rel.h (L165-L165)
Naming footgun: this change introduces rd_indexedattr here (heap-relation side: the union of columns across all of a table's indexes) while also adding rd_indattr further down (index-relation side: one index's referenced heap attrs). rd_indexedattr vs rd_indattr are near-identical names for two different concepts on two different relation kinds, which is a POLA violation that will confuse future readers and is easy to mix up. Consider a clearer, distinct name for the per-index cache field (e.g. rd_idxattr is no better; something like rd_indreferencedattr) or at least ensure the two comments make the heap-vs-index distinction unmistakable. Confidence: moderate.
📄 src/include/utils/relcache.h (L65-L76)
This detailed doc block duplicates the function-header comment already present at the definition in relcache.c (near line 5282), diverging from the established convention in this very header where prototypes like RelationGetIndexAttrBitmap (below) carry no comment and are documented only at the definition. Duplicated comments drift: this one already differs from the .c version. Worse, both are imprecise about the cache field -- the header says "caches its own copy in rd_indexcxt" (that's the memory context, not a field) and the .c comment says "rd_indexedattr", but the actual cached field is rd_indattr (see rel.h). Recommend dropping this block from the header (keep just the prototype) and fixing the field name at the definition. Confidence: high.
💡 Suggested change
Before:
+/*
+ * RelationGetIndexedAttrs -- return a freshly-palloc'd Bitmapset of every
+ * heap attribute this index references, via keys, INCLUDE columns,
+ * expressions, or partial-index predicates.
+ *
+ * The argument must be an index Relation (not its owning heap). Attribute
+ * numbers are offset by FirstLowInvalidHeapAttributeNumber. The result is
+ * palloc'd in the caller's context; bms_free when done. The relcache
+ * caches its own copy in rd_indexcxt so subsequent calls only pay for the
+ * final bms_copy.
+ */
+extern Bitmapset *RelationGetIndexedAttrs(Relation indexRel);
After:
+extern Bitmapset *RelationGetIndexedAttrs(Relation indexRel);
📄 src/include/storage/itemptr.h (L109-L109)
"worst case 0x6000" describes the flagged value (max real offset 0x2000 OR'd with the 0x4000 marker), not a bare offset as the phrasing implies. Since the strip threshold argument only needs "flagged value stays below the 0xfffd sentinel range", state that explicitly to avoid the reader concluding a real offset can reach 0x6000.
💡 Suggested change
Before:
* (worst case 0x6000 at BLCKSZ=32KB, vs. sentinels at 0xfffd/0xfffe), so
After:
* A flagged real offset is always
* well below the sentinel range even at the largest configurable BLCKSZ
* (worst case 0x2000 | 0x4000 = 0x6000 at BLCKSZ=32KB, vs. sentinels at
* 0xfffd/0xfffe), so
📄 src/include/storage/itemptr.h (L190-L191)
This claim is now stale/incomplete: it names only "each amgetbitmap implementation" as needing the raw/no-strip path, but ItemPointerCompare() in itemptr.c was also changed to strip via ItemPointerOffsetNumberStrip, and the amcheck/pageinspect readers also depend on the raw value + strip. ItemPointerCompare is a central, exported TID comparator (used by btree tidops, GIN, tidbitmap, etc.), so the assertion that ordinary consumers are unaffected and only bitmap AMs are special understates the blast radius. Update the comment to reflect that the equality/ordering path (ItemPointerCompare) also strips.
📄 src/test/benchmarks/siu/scripts/bit14_ab.sh (L2-L7)
The stated purpose here is contradicted by build.sh, which produces these two variants. In build.sh the "master" variant is built from MASTER_REV = git merge-base "$TEPID_REV" origin/master, i.e. the tepid branch's merge-base with master -- the ENTIRE SIU/HOT-indexed feature is absent, not merely the bit14 (ItemPointerSIUMaybeStaleFlag) fix reverted. The "tepid" variant is the full feature branch. So the two are NOT "otherwise identical", and this run does NOT isolate the bit14 fix's cost/benefit; it re-measures the whole feature's effect -- exactly the thing this comment says it excludes ("not the SIU feature's own win"). This produces a silently misleading A/B comparison. Either build a dedicated master-with-only-bit14-reverted variant (and reference it here), or correct this comment to describe what is actually compared. (high confidence)
📄 src/test/benchmarks/siu/scripts/bit14_ab.sh (L98-L99)
grep -oP uses GNU grep's Perl-compatible regex mode (-P), which is not available in the grep on macOS/BSD/Solaris that PostgreSQL supports. On those platforms extraction returns empty for both tps and lat, so every iteration is treated as FAILED and no CSV row is ever written. The sibling harnesses (run.sh, soak.sh) parse the same pgbench output portably with awk instead, e.g. awk '/tps = /{print $3; exit}' and awk '/latency average = /{print $4; exit}'. Make this file consistent with the suite and portable. (moderate confidence)
💡 Suggested change
Before:
tps=$(grep -oP 'tps = \K[0-9.]+' "$log" | tail -1)
lat=$(grep -oP 'latency average = \K[0-9.]+' "$log" | tail -1)
After:
tps=$(awk '/tps = /{print $3; exit}' "$log")
lat=$(awk '/latency average = /{print $4; exit}' "$log")
📄 src/test/benchmarks/siu/scripts/bit14_ab.sh (L47-L49)
Unguarded rm -rf "$datadir" diverges from the safer pattern the sibling scripts (run.sh, soak.sh) use for the same purpose: [ -d "$datadir" ] && find "$datadir" -mindepth 1 -delete && rmdir "$datadir". If BENCH is ever exported empty (bypassing the :- default, which only fires when unset), $datadir collapses toward a dangerous prefix. Match the suite's existing convention. (low confidence)
💡 Suggested change
Before:
local datadir=$BENCH/_data_bit14_$v
rm -rf "$datadir"
mkdir -p "$datadir"
After:
local datadir=$BENCH/_data_bit14_$v
[ -d "$datadir" ] && find "$datadir" -mindepth 1 -delete && rmdir "$datadir"
mkdir -p "$datadir"
📄 src/test/benchmarks/siu/scripts/bitmap_and_mixed.sql (L24-L24)
The two columns used in this predicate, b and d, are seeded identically (SELECT i, i, i, i, ... in seed()/seed_siu_table), so b == d for every row. The d BETWEEN ... clause is therefore fully redundant: the planner estimates a single index scan on siu_b already returns the same small row set and will almost certainly choose a plain index scan on one column rather than a BitmapAnd. That defeats the script's entire stated purpose (comment lines 5-7: "force a BitmapAnd across siu_b and siu_d"). Under -M prepared (as bit14_ab.sh invokes it) a generic plan makes this even more likely.
To actually force a BitmapAnd, the two predicate columns must be independently selective so that neither index alone is cheap enough on its own. Either seed b and d with independent values (e.g. (random()*rows)::int for each), or pick two columns whose combined selectivity requires intersecting both bitmaps. As written, the benchmark measures index-scan cost, not BitmapAnd cost. (moderate-high confidence)
📄 src/test/benchmarks/siu/scripts/bloat.sh (L30-L30)
Unguarded destructive rm -rf on a path derived from a user-overridable env var. DATADIR=$BENCH/_data_bloat and BENCH comes straight from the environment; if BENCH is exported empty (e.g. BENCH= ./bloat.sh), set -u does not catch it because the default ${BENCH:-...} only fires when unset, and rm -rf "$DATADIR" becomes rm -rf /_data_bloat. The sibling run.sh deliberately avoids a blind rm -rf by using find "$datadir" -mindepth 1 -delete && rmdir "$datadir". Guard the path before deleting, e.g. require it to be a non-empty subdir of a non-empty BENCH. (medium confidence)
💡 Suggested change
Before:
DATADIR=$BENCH/_data_bloat
After:
DATADIR=$BENCH/_data_bloat
[ -n "${BENCH:-}" ] && [ -n "${DATADIR:-}" ] || { echo "BENCH/DATADIR must be non-empty" >&2; exit 1; }
📄 src/test/benchmarks/siu/scripts/build.sh (L1-L2)
This whole benchmark directory (build.sh plus run.sh/soak.sh/bloat.sh/bit14_ab.sh and the .sql files) is orphaned developer-local scaffolding: nothing in the tree references src/test/benchmarks/siu (no Makefile, meson.build, or CI target). It hardcodes machine-specific absolute paths ($HOME/ws/postgres/tepid, /scratch/siu-bench, /scratch/pg) and the private branch name 'tepid'. This does not belong in a committable PostgreSQL patch: it is not wired into any build/test target, is not portable or self-contained, and violates the minimal-diff/YAGNI discipline that pgsql-hackers enforces. A performance claim should instead be backed by a reproducible benchmark described in the -hackers thread, not by committing ad-hoc build/run harnesses that only work on the author's box. Recommend dropping the entire siu benchmark directory from the patch. (high confidence)
📄 src/test/benchmarks/siu/scripts/build.sh (L40-L40)
Footgun: this operates destructively on the live source repo. It runs 'git checkout --detach' on the user's working tree and relies solely on an EXIT trap to restore ORIG; any interruption (SIGKILL, power loss) between the checkout and the trap leaves the developer's tree parked on an unexpected revision. Combined with 'rm -rf' on BENCH-derived paths, a misconfigured BENCH/REPO can silently mutate or delete the wrong directory. For a script meant to be invoked in a live repo this is a POLA/data-loss hazard. If the benchmark tooling is kept at all, it should build from a throwaway git worktree (git worktree add) rather than mutating the caller's checkout. (moderate confidence)
📄 src/test/benchmarks/siu/scripts/hot_indexed_mixed.sql (L1-L1)
The comment mischaracterizes the workload mix. The SELECT on line 7 runs in every transaction (100%), not 80%. Only the UPDATE is conditional: \if :which > 80 with random(1, 100) is true for values 81..100, i.e. 20% of transactions. So the real mix is "100% selects, 20% of which also do an indexed-column update," not "80% selects, 20% updates." Fix the comment to describe what the script actually does. (moderate confidence)
💡 Suggested change
Before:
-- Mixed workload: 80% selects, 20% indexed-column updates.
After:
-- Mixed workload: every txn does a SELECT; 20% also do an indexed-column update.
📄 src/test/benchmarks/siu/scripts/hot_indexed_mixed.sql (L7-L7)
Minimalism/YAGNI concern (high confidence): this file introduces a brand-new src/test/benchmarks/ tree that does not otherwise exist in the source tree, is not wired into any Makefile/meson build or test schedule, and is not runnable by the buildfarm/cfbot. These are ad-hoc developer A/B benchmark scripts (they hardcode paths like /scratch/siu-bench, a tepid branch name, and expect a two-variant build harness). Committing throwaway benchmark scaffolding as part of a feature patch is a common -hackers rejection reason: it bloats the diff with code that must exist "as if it had always been written that way" and that nobody in-tree runs. Consider dropping the entire src/test/benchmarks/siu/ directory from the patch and instead posting the reproducible benchmark/results on the -hackers thread, or make the case for a permanent, wired-in benchmark harness separately.
📄 src/test/isolation/isolation_schedule (L131-L131)
Naming convention nit (low confidence): every other entry in this schedule uses hyphens as word separators (e.g. ddl-dependency-locking, cluster-toast-value-reuse). This is the only underscore-separated test name in the file. For consistency with the subsystem's existing pattern, consider renaming the spec/expected files to hot-indexed-adversarial and updating this line accordingly.
💡 Suggested change
Before:
test: hot_indexed_adversarial
After:
test: hot-indexed-adversarial
📄 src/test/benchmarks/siu/scripts/soak.sh (L99-L99)
The CSV column is labeled heap_pages but the value is pg_table_size('siu_table')/8192. pg_table_size includes the TOAST relation and the FSM/VM forks, not just the heap main fork, so this figure is not heap pages and the header is misleading. Since this benchmark backs HOT-indexed bloat/performance claims, use the correct relation/fork or fix the label. (Note: the sibling run.sh labels the identical expression bloat_pages, which is at least neutral.)
💡 Suggested change
Before:
heap_pages=$(psql_as "$v" -Atc "SELECT pg_table_size('siu_table')/8192")
After:
heap_pages=$(psql_as "$v" -Atc "SELECT pg_relation_size('siu_table', 'main')/8192")
📄 src/test/benchmarks/siu/scripts/soak.sh (L47-L49)
Fixed sleep 2 is used as a startup readiness signal, which can race on slow/loaded machines under set -e (the first psql in setup() would then fail opaquely). The sibling bloat.sh already uses the robust pattern pg_ctl ... -w start to block until the server accepts connections; use that here for consistency and to avoid the race.
💡 Suggested change
Before:
LD_LIBRARY_PATH="$(LD_of "$v")" "$(bin_of "$v")/pg_ctl" -D "$datadir" \
-o "-p $PORT" -l "$LOGDIR/pg_$v.log" start >/dev/null
sleep 2
After:
LD_LIBRARY_PATH="$(LD_of "$v")" "$(bin_of "$v")/pg_ctl" -D "$datadir" \
-o "-p $PORT" -l "$LOGDIR/pg_$v.log" -w start >/dev/null
📄 src/test/benchmarks/siu/scripts/soak.sh (L106-L108)
tps/hot_pct are derived from deltas of pg_stat_user_tables (n_tup_upd/n_tup_hot_upd). The cumulative stats system propagates these asynchronously (subject to PGSTAT_MIN_INTERVAL flushing), so per-tick deltas at a 60s cadence can be skewed at sample boundaries and the final ticks may miss counts committed just before pgbench exits. pgbench's own -P $SAMPLE progress report (already written to the log) gives an authoritative TPS; consider deriving tps_instant from it rather than the stats deltas, or at least document that these are approximate.
📄 src/test/benchmarks/siu/scripts/run.sh (L65-L65)
Bug: under set -euo pipefail, this line aborts the whole harness on the very first run when $datadir does not yet exist. [ -d "$datadir" ] returns non-zero, and since it is the last-evaluated command of the && list, the non-zero status propagates and set -e kills the script before the mkdir runs. The sibling bit14_ab.sh avoids this entirely with a plain rm -rf "$datadir". Suggest matching that.
💡 Suggested change
Before:
[ -d "$datadir" ] && find "$datadir" -mindepth 1 -delete && rmdir "$datadir"
After:
rm -rf "$datadir"
📄 src/test/benchmarks/siu/scripts/run.sh (L81-L83)
pg_ctl start is invoked without -w and followed by a fixed sleep 2, so workloads can start before the server accepts connections (or race a slow start). Fixed sleeps as synchronization are a known footgun and are inconsistent with the sibling bloat.sh, which starts with pg_ctl ... -w start. Use -w (and drop the sleep) so startup blocks until the server is ready.
💡 Suggested change
Before:
LD_LIBRARY_PATH="$(LD_of "$v")" "$(bin_of "$v")/pg_ctl" -D "$datadir" \
-o "-p $PORT" -l "$LOGDIR/pg_$v.log" start >/dev/null
sleep 2
After:
LD_LIBRARY_PATH="$(LD_of "$v")" "$(bin_of "$v")/pg_ctl" -D "$datadir" \
-o "-p $PORT" -l "$LOGDIR/pg_$v.log" -w start >/dev/null
📄 src/test/benchmarks/siu/scripts/run.sh (L209-L210)
Comment/behavior drift: this claims a "falls back to pgrep + per-pid ps" fallback, but no fallback path is implemented -- there is exactly one pgrep -P / ps -o pcpu=,rss= code path below. This is also a real portability gap: ps -o pcpu=,rss= and pgrep -P semantics differ on macOS/Solaris and are absent on Windows, so the sampler will silently yield wrong or zero values there. Either implement the claimed fallback or fix the comment to describe what the code actually does.
📄 src/test/benchmarks/siu/scripts/run.sh (L18-L18)
Comment/behavior drift: the header says peak CPU/RSS is "sampled via pidstat", but pidstat is never invoked -- sample_peak() uses pgrep/ps. Update the comment to match the implementation.
💡 Suggested change
Before:
# counts + WAL delta + peak CPU/RSS sampled via pidstat.
After:
# counts + WAL delta + peak CPU/RSS sampled via ps/pgrep.
📄 src/test/benchmarks/siu/scripts/run.sh (L341-L341)
Comment drift: this references "id % 1 so it's a no-op", but the emitted clause is id=id -- there is no modulo. The stale parenthetical is confusing; drop or correct it.
💡 Suggested change
Before:
# No indexed-col update; touch a non-indexed column (id % 1 so it's a no-op)
After:
# No indexed-col update; rewrite id to itself (a no-op assignment).
📄 src/test/regress/sql/tsearch.sql (L763-L764)
The comment names the wrong function. tsvector_update_trigger() writes the tsvector column via heap_modify_tuple_by_cols() (see tsvector_op.c), not heap_modify_tuple(). Since the whole point of this reworded comment is to precisely document how the trigger mutates an indexed column outside the SET clause, cite the actual function to avoid sending a reader looking for a call that isn't there. (moderate confidence)
💡 Suggested change
Before:
-- tsvector_update_trigger() uses heap_modify_tuple() to set column 'a'
-- without going through the executor's SET-clause tracking.
After:
-- tsvector_update_trigger() uses heap_modify_tuple_by_cols() to set column 'a'
-- without going through the executor's SET-clause tracking.
📄 src/test/recovery/t/055_hot_indexed_recovery.pl (L117-L120)
This assertion does not actually test the crossed-attribute bitmap filtering it claims to. The query runs in a fresh safe_psql session with default planner settings, so the planner will pick a seqscan or bitmap heap scan for c1 = 100. Both trivially return 0 (the live tuple has c1=105), so this passes even with the feature fully reverted. The feature's own SQL test (contrib/pageinspect/sql/hot_indexed_updates.sql lines 130-135) documents that to exercise the read-side bitmap you must force an IndexScan: SET enable_seqscan=off; SET enable_bitmapscan=off; and add a heap-fetch predicate (e.g. payload IS NOT NULL) so the IndexOnlyScan path is not taken. Set those GUCs in this same session and add the predicate, otherwise the test is worthless for this path. Confidence: high.
💡 Suggested change
Before:
my $stale_count = $node->safe_psql('postgres',
q{SELECT count(*) FROM hi_recov WHERE c1 = 100});
is($stale_count, '0',
'stale btree entries are filtered by the crossed-attribute bitmap');
After:
my $stale_count = $node->safe_psql('postgres', q{
SET enable_seqscan = off;
SET enable_bitmapscan = off;
SELECT count(*) FROM hi_recov WHERE c1 = 100 AND payload IS NOT NULL;
});
is($stale_count, '0',
'stale btree entries are filtered by the crossed-attribute bitmap');
📄 src/test/recovery/t/055_hot_indexed_recovery.pl (L88-L91)
This assertion cannot detect whether the prune/collapse actually happened. The live HOT-indexed version always keeps n_hot_indexed > 0 (the feature guarantees the live version can never be pruned; see contrib/pageinspect/sql/hot_indexed_updates.sql line 537 "The live version is a HOT-indexed member and cannot be pruned"). So n_hot_indexed > 0 here is identical to the $pre_prune check and proves nothing about collapse to LP_REDIRECT. If the goal is to confirm the chain collapsed, assert on n_chains (the count of LP_REDIRECT roots) instead. Confidence: high.
💡 Suggested change
Before:
my $post_prune = $node->safe_psql('postgres',
q{SELECT n_hot_indexed FROM pg_relation_hot_indexed_stats('hi_recov')});
cmp_ok($post_prune, '>', 0,
'live HOT-indexed version survives opportunistic prune');
After:
my $post_prune = $node->safe_psql('postgres',
q{SELECT n_chains FROM pg_relation_hot_indexed_stats('hi_recov')});
cmp_ok($post_prune, '>', 0,
'dead chain members collapsed to LP_REDIRECT forwarders after prune');
📄 src/test/recovery/t/055_hot_indexed_recovery.pl (L80-L84)
The prune here relies on opportunistic pruning (heap_page_prune_opt), which is best-effort and horizon-gated -- the comment itself admits "that's not enough on its own to trigger prune." The feature authors deliberately refused to assert deterministic collapse state in SQL for exactly this reason (contrib/pageinspect/sql/hot_indexed_updates.sql lines 514-523: collapse is "gated on the superseded versions falling below the global xmin horizon ... so the physical layout ... is not deterministic here" and is instead covered by the isolation spec). If collapse does not occur, the whole "recovery of a collapsed chain" scenario this test claims to cover is silently not exercised, yet the test still passes. Use a deterministic driver: a VACUUM (DISABLE_PAGE_SKIPPING) hi_recov (this cluster has autovacuum off and no concurrent snapshots) forces the collapse, then assert on n_chains. Confidence: high.
💡 Suggested change
Before:
$node->safe_psql('postgres', q{
SET enable_indexscan = off;
SELECT count(*) FROM hi_recov;
UPDATE hi_recov SET payload = 'pruned' WHERE id = 1;
});
After:
$node->safe_psql('postgres',
q{UPDATE hi_recov SET payload = 'pruned' WHERE id = 1});
$node->safe_psql('postgres',
q{VACUUM (DISABLE_PAGE_SKIPPING) hi_recov});
📄 src/test/recovery/t/055_hot_indexed_recovery.pl (L142-L145)
The "exactly two VACUUM (FREEZE) passes reach n_hot_indexed == 0" assumption is horizon-dependent and fragile. Reclamation of a collapsed HOT-indexed chain is gated on the dead versions falling below the global xmin horizon; the feature authors explicitly avoid asserting post-VACUUM reclaim counts in SQL for this reason (contrib/pageinspect/sql/hot_indexed_updates.sql lines 514-523). Hard-coding two passes encodes an implementation detail that can change (one pass or more depending on index-vacuum ordering). Prefer polling to a stable state via poll_query_until so the test is robust to pass-count changes. Confidence: moderate.
📄 src/test/recovery/t/055_hot_indexed_recovery.pl (L122-L123)
Comment is factually wrong: verify_heapam's default skip is 'none' (SKIP_PAGES_NONE), not 'all-frozen' (see contrib/amcheck/verify_heapam.c: skip_option = SKIP_PAGES_NONE;). The call correctly passes skip := 'none', so the parenthetical mis-describes the default. Fix the comment to describe current behavior. Confidence: high.
💡 Suggested change
Before:
# 2. verify_heapam reports no errors on the relation (skip_option =
# 'all-frozen' is the default; we want to scan everything).
After:
# 2. verify_heapam reports no errors on the relation. skip := 'none'
# (the default) scans every page including all-frozen ones.
📄 src/test/subscription/t/040_hot_indexed_replica_identity.pl (L83-L86)
This test is not discriminating: nothing asserts that the HOT-indexed apply path was actually taken. The subscriber-vs-publisher equality checks and the verify_heapam(=0) assertion would all pass even if hot_indexed_on_apply were reverted or eligibility silently regressed to plain non-HOT (a correct-but-non-HOT apply converges identically and leaves a corruption-free heap). A test that still passes with the feature disabled is worthless as a regression guard.
The sibling test 039_hot_indexed_apply.pl solves exactly this by polling pg_stat_user_tables.n_tup_hot_indexed_upd and asserting > 0 before the convergence/verify_heapam checks, and even documents the reasoning ("the convergence/verify_heapam asserts below could pass vacuously if eligibility silently regressed"). Add the same counter assertion here (after wait_for_catchup, before the equality checks) so this test actually exercises and would catch a regression in the new code. (high confidence)
📄 src/test/subscription/t/040_hot_indexed_replica_identity.pl (L16-L22)
This file substantially duplicates the "always-mode safety with indexed attrs beyond the replica identity" section of 039_hot_indexed_apply.pl, which already covers: REPLICA IDENTITY USING INDEX on a non-PK unique index, an rid ABA cycle, a replicated UPDATE and DELETE resolved via the RI with stale leaves, publisher/subscriber convergence, and verify_heapam clean -- and additionally asserts the HOT-indexed counter fired. The only net-new coverage here is REPLICA IDENTITY FULL (seqscan apply). Consider folding the FULL case into 039 rather than adding a near-duplicate cluster spin-up, or trim this file to the FULL-specific scenario; a patch that adds an overlapping second test wastes reviewer and buildfarm time. (moderate confidence)
📄 src/test/subscription/t/040_hot_indexed_replica_identity.pl (L100-L103)
The comment claims the subscriber tables are "structurally consistent", but verify_heapam is only run on tab_idx; tab_full (REPLICA IDENTITY FULL, also carrying extra secondary indexes on the subscriber) is never structurally verified. Either also run verify_heapam('tab_full') or narrow the comment to tab_idx. (moderate confidence)
📄 src/test/subscription/t/039_hot_indexed_apply.pl (L127-L130)
This hand-rolled poller silently swallows a timeout: when time() >= $deadline fires before the counter reaches $upd_target, the loop exits and returns whatever partial value was last read, with no failure signalled. On a slow/loaded buildfarm animal where the apply worker's asynchronous pgstat flush lags past 10s, this makes the downstream assertions nondeterministic -- cmp_ok($..., '>', 0) can fail spuriously, and is($..., 0) can pass vacuously (counter simply hasn't caught up yet). This is the "deadline as synchronization" anti-pattern.
The tree's established idiom is poll_query_until($db, "SELECT <bool condition>") which dies on timeout (see 026_stats.pl, 032_subscribe_use_index.pl). Prefer waiting on an explicit equality/>= condition against pg_stat_user_tables via poll_query_until and letting it die on timeout, then read the settled counters once. That both removes the flakiness and turns a lagging counter into a hard failure instead of a false pass.
📄 src/test/subscription/t/039_hot_indexed_apply.pl (L182-L185)
The apply-path assertions discard the classic-HOT and n_tup_upd deltas (undef) for most cases and only check the hot-indexed delta against 0. In the off and subset_only/tab_extra cases the test asserts only n_tup_hot_indexed_upd == 0; it never asserts that n_tup_upd actually advanced by the expected count, nor whether these updates landed as classic HOT vs plain non-HOT. A regression that silently demoted everything to non-HOT (or that failed to apply the updates at all) would still satisfy these is(..., 0) checks -- i.e. they can pass with the feature broken. Consider also asserting the n_tup_upd delta equals the number of rows updated so the test proves the updates were applied, and asserting the classic-HOT delta where its value is deterministic.
| SELECT blkno, offnum, attnum, msg | ||
| FROM verify_heapam('hot_indexed_check', | ||
| startblock := NULL, | ||
| endblock := NULL); |
There was a problem hiding this comment.
The test asserts only (0 rows) from verify_heapam, so it passes identically whether or not any UPDATE actually took the HOT-indexed path, kept its tuple on-page, or produced an LP_REDIRECT chain. Nothing here confirms the premise stated in the comments ("keeps the new tuple on the same page", "collapses dead members to LP_REDIRECT forwarders"). If a future change silently disabled the HOT-indexed path, or if these updates fall back to non-HOT/off-page updates (see the fillfactor note), this test would still pass green while exercising none of the artifacts it claims to. Consider asserting the on-page/redirect state (e.g. via pageinspect heap_page_items, as the dedicated hot_indexed_updates.sql tests do) so the test actually pins down what it documents. [moderate confidence]
| CREATE TABLE hot_indexed_check (id int PRIMARY KEY, c1 int, c2 int, c3 int) | ||
| WITH (fillfactor = 70); |
There was a problem hiding this comment.
fillfactor = 70 is a weak guarantee for keeping updated tuples on the same page: it leaves relatively little free space, and after three successive c2 updates the block may fill and force off-page (non-HOT) updates, defeating the scenario. The companion feature tests (contrib/pageinspect/sql/hot_indexed_updates.sql) deliberately use fillfactor = 50/40/10 for exactly this reason. Also, without autovacuum_enabled = false an autovacuum can prune/collapse between statements, making the intended chain-of-dead-intermediates state non-deterministic. Recommend a roomier fillfactor and autovacuum_enabled = false to make the exercised state reliable. [moderate confidence]
| -- Must return the row (previously a false negative before the SIU bitmap fix). | ||
| SELECT count(*) AS bloom_bitmapand FROM bloom_siu WHERE bcol = 11 AND changed = 22; |
There was a problem hiding this comment.
This test does not verify that a BitmapAnd over both indexes is actually chosen, so it may not guard the SIU code path it claims to. With a single-row table the planner may bitmap-scan only one index (e.g. bloom_siu_c on changed) and apply bcol = 11 as a heap filter/recheck, never routing the bloom index's fresh entry through the SIU may-be-stale path in tbm_add_tuples. In that case the query returns 1 whether or not the SIU fix is present, so the test can pass even if the fix regresses. The existing bloom tests in this same file use EXPLAIN (COSTS OFF) before each result query precisely to pin the access path (see the tst/tstu cases above). Add an EXPLAIN (COSTS OFF) that asserts a BitmapAnd of bloom_siu_b and bloom_siu_c (and record the plan in the expected output) so the test actually exercises and locks in the intended path. Confidence: high.
| -- Must return the row (previously a false negative before the SIU bitmap fix). | |
| SELECT count(*) AS bloom_bitmapand FROM bloom_siu WHERE bcol = 11 AND changed = 22; | |
| -- Confirm the plan really is a BitmapAnd over both indexes; otherwise this | |
| -- test does not exercise the SIU path it is meant to guard. | |
| EXPLAIN (COSTS OFF) | |
| SELECT count(*) FROM bloom_siu WHERE bcol = 11 AND changed = 22; | |
| -- Must return the row (previously a false negative before the SIU bitmap fix). | |
| SELECT count(*) AS bloom_bitmapand FROM bloom_siu WHERE bcol = 11 AND changed = 22; |
| * images match; restore it immediately afterward, since later | ||
| * checks in this loop still read itup from the (immutable) page | ||
| * image. Posting-list tuples can carry the marker too -- nbtree |
There was a problem hiding this comment.
Minor: calling the page copy "(immutable) page image" here is misleading, because the code in this very block mutates itup->t_tid in place (line 1515) and only restores it afterward (line 1559). itup points into state->target, a palloc'd page copy from palloc_btree_page(), not an immutable image. Consider dropping the "(immutable)" qualifier to avoid implying the tuple is never written.
| * images match; restore it immediately afterward, since later | |
| * checks in this loop still read itup from the (immutable) page | |
| * image. Posting-list tuples can carry the marker too -- nbtree | |
| * images match; restore it immediately afterward, since later | |
| * checks in this loop still read itup from the page copy. | |
| * Posting-list tuples can carry the marker too -- nbtree |
| OffsetNumber rdoffnum; | ||
| ItemId rditem; | ||
|
|
||
| /* Resolve the redirect's target offset. */ | ||
| rdoffnum = ItemIdGetRedirect(ctx.itemid); |
There was a problem hiding this comment.
Minimal-diff violation: this hunk splits the existing one-line OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid); into a bare declaration plus a separate assignment and adds a redundant comment ("Resolve the redirect's target offset."), on logic that is otherwise untouched by this feature. This is gratuitous churn on unrelated code -- one of the top rejection reasons on pgsql-hackers. Keep the original single-line initializer to keep the diff minimal. (moderate confidence)
| OffsetNumber rdoffnum; | |
| ItemId rditem; | |
| /* Resolve the redirect's target offset. */ | |
| rdoffnum = ItemIdGetRedirect(ctx.itemid); | |
| OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid); | |
| ItemId rditem; |
| List *indexoidlist = RelationGetIndexList(OldHeap); | ||
| bool siu_capable = (list_length(indexoidlist) > 1); |
There was a problem hiding this comment.
RelationGetIndexList counts every valid index (including partial and expression indexes) toward the > 1 SIU-capability threshold. Per HeapUpdateHotAllowable(), an UPDATE that touches an expression-index attribute always falls back to HEAP_UPDATE_ALL_INDEXES (never forms an SIU chain), and this list may also include indexes irrelevant to any given UPDATE. So list_length > 1 over-triggers the direct-scan degradation for tables that can never actually produce an SIU chain reachable-only-via-own-TID (e.g. a table whose only "second" index is an expression index). The forced seqscan/sort is a correctness-safe over-approximation, but it needlessly loses cluster order / forces a sort for such tables. Consider a tighter predicate if this degradation matters in practice, or document why the coarse proxy is intentional.
| rel = relation_open(relid, AccessShareLock); | ||
| if (rel->rd_rel->relkind != RELKIND_RELATION && | ||
| rel->rd_rel->relkind != RELKIND_MATVIEW && | ||
| rel->rd_rel->relkind != RELKIND_TOASTVALUE) |
There was a problem hiding this comment.
Missing guard against other sessions' temporary relations. This function scans buffers of the target relation via ReadBufferExtended(MAIN_FORKNUM), but temp-relation data lives in the owning backend's local buffers. Reading another session's temp table here yields wrong data or errors. Every comparable full-scan diagnostic (pgstattuple.c, pgstatapprox.c, pgstatindex.c, pageinspect/rawpage.c) rejects this with RELATION_IS_OTHER_TEMP. Add before scanning:
if (RELATION_IS_OTHER_TEMP(rel))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot access temporary tables of other sessions")));
| if (rel->rd_rel->relkind != RELKIND_RELATION && | ||
| rel->rd_rel->relkind != RELKIND_MATVIEW && | ||
| rel->rd_rel->relkind != RELKIND_TOASTVALUE) |
There was a problem hiding this comment.
relkind is checked but table AM is not. This code casts raw page items to HeapTupleHeader and interprets t_infomask2/t_ctid/line pointers as heap layout, yet a RELKIND_RELATION or RELKIND_MATVIEW may use a non-heap table AM, whose pages have a different format. Scanning such a relation produces garbage counts or crashes. Guard on the AM as pgstattuple does, e.g. reject when rel->rd_rel->relam != HEAP_TABLE_AM_OID.
|
|
||
| CHECK_FOR_INTERRUPTS(); | ||
|
|
||
| buf = ReadBufferExtended(rel, MAIN_FORKNUM, blk, RBM_NORMAL, NULL); |
There was a problem hiding this comment.
Full-relation sequential scan with no buffer access strategy. Passing NULL to ReadBufferExtended for every block of a potentially large relation will evict useful pages from shared_buffers. Comparable full-scan diagnostics use a BAS_BULKREAD strategy (GetAccessStrategy(BAS_BULKREAD)) and pass it here. Allocate one before the loop and pass it in.
| vmflags, | ||
| conflict_xid, | ||
| false, /* no cleanup lock required */ | ||
| false, /* no cleanup lock required: see below */ |
There was a problem hiding this comment.
Dangling cross-reference: this inline comment says "see below", but there is no explanation below it within lazy_vacuum_heap_page(). The rationale for passing cleanup_lock=false actually lives in lazy_vacuum_heap_rel() (where the lock is acquired) and in log_heap_prune_and_freeze(). Point the reader there instead, or drop the "see below".
| false, /* no cleanup lock required: see below */ | |
| false, /* no cleanup lock required (see lazy_vacuum_heap_rel) */ |
1 participant
No description provided.