Tepid: only update indexes that have changed attributes

[gburd]

No description provided.

[github-actions]

The stub-detection comment claims the stub is "permanently invisible (HEAP_XMIN_INVALID)", but HotIndexedHeaderIsStub() (in access/hot_indexed.h) recognizes a stub solely by HEAP_INDEXED_UPDATED set AND HeapTupleHeaderGetNatts() == 0 -- it does not inspect HEAP_XMIN_INVALID at all. The comment asserts an invariant the code here does not verify. Either drop the HEAP_XMIN_INVALID claim (moderate confidence it is set by prune, but this code does not check it) or note it as a documented property enforced elsewhere, so the comment describes what this code actually keys on.

[github-actions]

Confidence: moderate. This exemption deliberately suppresses the long-standing "HOT chains should not intersect" corruption report whenever the target has HEAP_INDEXED_UPDATED set and the recorded predecessor is a redirect or stub. As the comment itself warns, this must not "weaken corruption detection to the mere presence of HEAP_INDEXED_UPDATED" -- yet that single infomask2 bit plus predecessor shape is exactly the whole guard. On a genuinely corrupt page, two redirects that happen to point at a heap-only tuple whose HEAP_INDEXED_UPDATED bit is set (whether legitimately, or via bit corruption) will now be silently accepted as a valid collapse fan-in. There is no on-disk marker that distinguishes a real collapse survivor from an ordinary HOT-indexed tuple, so amcheck cannot fully validate the fan-in here; that is an inherent limitation, but it is worth calling out on the thread since amcheck's job is precisely to catch such intersections. At minimum, document explicitly that this exemption trades away detection of intersecting chains whenever the shared target is HOT-indexed.

[github-actions]

Confidence: moderate. When a collapse-survivor stub forwards to a target that is itself a redirect (a legitimate shape per README.HOT-INDEXED: stub -> redirect -> first survivor), ItemIdIsNormal(next_lp) is false, so this branch silently records no predecessor edge and performs no heap-only check on the target before falling through to continue. The comment says the target "must be heap-only (another stub or the live heap-only tuple)" but the redirect-target case is neither reported as corruption nor validated. Confirm this gap is intentional (matching how a plain tuple pointing to a redirect is dropped at the ItemIdIsRedirected(next_lp) check below); if a stub-to-redirect edge should propagate the predecessor to the redirect's ultimate target, this loses that linkage and the redirect's live target may be misclassified as a chain root.

[github-actions]

This regresses the parallel-safety marking of bt_page_items(text, int8). In pageinspect--1.10--1.11.sql this variant was deliberately made PARALLEL RESTRICTED because "Functions that fetch relation pages must be PARALLEL RESTRICTED ... otherwise they will fail when run on a temporary table in a parallel worker process." Dropping and recreating it here with PARALLEL SAFE silently reverts that fix, reintroducing the temp-table-in-parallel-worker failure after upgrading to 1.14. Use PARALLEL RESTRICTED for this (text, int8) variant. (The bytea variant below is correctly SAFE since it takes the page directly.) Confidence: high.

Suggested change

	AS 'MODULE_PATHNAME', 'bt_page_items_1_9'
	LANGUAGE C STRICT PARALLEL SAFE;
	AS 'MODULE_PATHNAME', 'bt_page_items_1_9'
	LANGUAGE C STRICT PARALLEL RESTRICTED;

[github-actions]

For a deduplicated posting-list leaf tuple, hot_indexed is hard-coded to false in the pivot/posting else-branch, but such a posting tuple can contain SIU-flagged heap TIDs (per this patch's design, dedup preserves the marker on each element). The result is misleading: hot_indexed = false is reported for a leaf whose posting elements actually carry the may-be-stale marker, and no output column reflects that. The comment above claims the flag can only apply to a plain leaf t_tid, but that ignores the posting-list heap-TID array, which the itemptr.h header explicitly documents as carrying the flag ("including each individual entry of a posting list's heap-TID array"). Reconcile the reporting (and comment) with the documented posting-list case. (high confidence)

[github-actions]

This assertion is horizon-dependent and risks intermittent buildfarm failures under installcheck. With fillfactor = 10 and no autovacuum_enabled = false, the 200-iteration loop produces ~200 dead tuple versions and relies entirely on opportunistic HOT pruning to reclaim them so the heap stays within 2 pages. But opportunistic prune can only reclaim versions below the global xmin horizon; a concurrent session in a running regression cluster that holds an old snapshot pins that horizon back, so the dead versions cannot be reclaimed and the heap balloons past 8192 * 2.

This is precisely the non-determinism that Section 12's own comment ("opportunistic HOT pruning ... [is] gated on the superseded versions falling below the global xmin horizon, which a snapshot held elsewhere in the running regression cluster can pin back indefinitely") deliberately avoids asserting. Section 11 contradicts that discipline. Note also that sections 12/13 set autovacuum_enabled = false while this table does not, making its layout even less controlled.

Consider either dropping the size assertion (keeping only the horizon-independent hot_idx > 0 check) or making the bound generously loose, consistent with how Section 12 handles the same physical-layout non-determinism. (moderate confidence)

[github-actions]

The second pg_stat_force_next_flush() (line 325) is redundant: the flush at line 323 already pushed all pending session stats to the shared stats, and the intervening SELECT get_hot_count('hot_test_part1') performs no updates, so no new HOT/update stats accrue. Drop the extra call to keep the test minimal. Confidence: high.

Suggested change

	SELECT pg_stat_force_next_flush();
	SELECT * FROM get_hot_count('hot_test_part1');
	SELECT pg_stat_force_next_flush();
	SELECT * FROM get_hot_count('hot_test_part2');
	SELECT pg_stat_force_next_flush();
	SELECT * FROM get_hot_count('hot_test_part1');
	SELECT * FROM get_hot_count('hot_test_part2');

[github-actions]

get_hot_count is duplicated verbatim (modulo whitespace) in the sibling file contrib/pageinspect/sql/hot_indexed_updates.sql (which defines get_hot_count and a superset get_hi_count). Two copies of a non-trivial PL/pgSQL helper will drift. Since both files run in the same pageinspect schedule, consider defining it once in the earlier-running file and not re-creating/dropping it here (or fold both into get_hi_count and derive hot-only from it). Confidence: high.

[github-actions]

The (ctid::text::point)[0]/[1] idiom to extract block/offset relies on round-tripping a tid through text and the point (float8) type. It is not used anywhere else in the pageinspect suite and is fragile: point components are float8, so this loses precision for block numbers > 2^53 (fine for these tiny tables, but a footgun if copied). A clearer, exact idiom is to parse the tid text directly, e.g. split_part(trim(both '()' from target_ctid::text), ',', 1)::int. Confidence: moderate.

[github-actions]

The comment attributes both hazards ("overwrite t_ctid and clear its xact bits") to both kill and freeze, but only the freeze path does that (t_ctid is rewritten at ItemPointerSet and HEAP_XACT_MASK is cleared). The kill path only sets the item LP_DEAD, which removes the stub from the chain entirely rather than overwriting its fields. Consider splitting the two cases so the comment matches the code: e.g. "A kill would drop the stub from the chain; a freeze would overwrite its t_ctid forward link and clear its xact bits."

[github-actions]

The \gset here silently leaves :'stub_off' unset if the SELECT returns zero rows. If a future prune/vacuum heuristic change (or an ADD COLUMN / posting-list variation) stops producing a stub on page 0, psql does not error at the \gset; instead the following heap_force_kill/heap_force_freeze lines interpolate an unset/leftover variable, yielding an opaque failure that is hard to diagnose on the buildfarm rather than a clear "no stub found". Consider asserting a row was found first (e.g. SELECT ... \gset guarded by a count, or \if :{?stub_off}), so a missing stub fails with an intelligible message. Confidence: moderate.

[github-actions]

This relaxation drops three previously-fatal ERRCODE_INDEX_CORRUPTED checks unconditionally, for every relation and every btree delete -- not just relations that actually took the HOT-indexed path. On an installation that never uses HOT-indexed updates, a genuinely corrupt index entry (offset past maxoff, LP_UNUSED target, or a heap-only tuple reached directly from an index page) will now be silently swallowed as "vacuumable" instead of being detected. That is exactly the class of silent-data-corruption masking the amcheck/index_delete corruption checks exist to catch. Consider gating the tolerance on a relation property that indicates the relation can contain HOT-indexed chains (e.g. only skip the error when HEAP_INDEXED_UPDATED tuples are possible for this rel), so ordinary corruption on non-HOT-indexed relations is still reported.

[github-actions]

Dead statement: elog(ERROR, ...) never returns, so this return; is unreachable. Remove it to avoid implying elog(ERROR) can fall through.

Suggested change

	elog(ERROR, "tuple concurrently deleted");
	return;
	elog(ERROR, "tuple concurrently deleted");

[github-actions]

Misleading comment. "see below" points to an explanation that does not exist below this line in lazy_vacuum_heap_page(); the reasoning for why an exclusive (non-cleanup) lock suffices actually lives in the caller lazy_vacuum_heap_rel() (the block above the ConditionalLockBufferForCleanup call), i.e. above this function in source order. Either drop "see below" and restore the original wording, or point the reader at lazy_vacuum_heap_rel().

Confidence: high.

Suggested change

	false, /* no cleanup lock required: see below */
	false, /* no cleanup lock required */

[github-actions]

DRY violation and drift hazard. This same "if the redirect target has HEAP_INDEXED_UPDATED set, clear set_all_visible/set_all_frozen" logic is now duplicated in three sites: here, heap_prune_record_unchanged_lp_redirect(), and heap_prune_record_stub(). Worse, the copies are not even structurally identical -- this one inspects the rdoffnum argument, while heap_prune_record_unchanged_lp_redirect() re-reads the on-page LP via ItemIdGetRedirect(). If the staleness condition ever drifts in one site but not the others, a page holding an SIU redirect could be wrongly marked all-visible, letting an index-only scan skip the crossed-attribute recheck and return stale rows -- a silent-wrong-results footgun. Extract a single helper (e.g. redirect_target_is_hot_indexed(page, rdoffnum)) and call it from all three sites.

[github-actions]

Stray/undefined term "PHOT". Nothing else in this feature (HEAP_INDEXED_UPDATED, "HOT-indexed", "HOT/SIU") uses "PHOT"; it reads as a leftover from an earlier naming. Comments must describe the code as it is now -- rename to the established "HOT/SIU" (or "HOT-indexed") term to avoid confusing a -hackers reviewer.

[github-actions]

📊 OCR posted 24/25 inline comment(s).

1 could not be posted

contrib/pageinspect/btreefuncs.c L639: Validation Failed: {"resource":"PullRequestReviewComment","code":"custom","field":"pull_request_review_thread.line","message":"could not be resolved"} - https://docs.github.com/rest/pulls/comments#create-a-review-comment-for-a-pull-request

[github-actions]

🔍 OCR found 106 issue(s).

• 25 inline, 81 in summary (inline capped at 25)

---

📄 src/backend/access/heap/heapam_handler.c (L2806-L2809)

Minor: this dedup is O(n^2) in the number of resolved tuples on a block once any hot-indexed hop is seen (nested scan of rs_vistuples for every subsequent resolution). It is bounded by TBM_MAX_TUPLES_PER_PAGE so not catastrophic, but on SIU-heavy pages it is pure overhead. Consider a small on-stack presence bitmap keyed by offset number (offsets are <= MaxHeapTuplesPerPage) to keep dedup O(1) per tuple while preserving insertion order.

---

📄 src/backend/access/heap/heapam_handler.c (L2616-L2617)

Clearing HEAP_INDEXED_UPDATED is correct, but the copy still carries the now-orphaned trailing inline modified-attrs bitmap bytes in t_len (heap_copytuple copies t_len verbatim). Readers are gated on the flag so this is not a correctness bug, but every rewritten SIU live tuple keeps ceil(natts/8) dead bytes forever after CLUSTER/VACUUM FULL, silently inflating the rewritten heap. Since the whole point of the rewrite is to reclaim space, consider re-forming these tuples (or trimming t_len by HotIndexedBitmapBytes) instead of leaving the padding.

---

📄 src/backend/access/heap/heapam.c (L8699-L8700)

This function's header comment is now stale and contradicts the body. It still states "We can be sure that the index is corrupt when htid points directly to an LP_UNUSED item or heap-only tuple, which is not the case during standard index scans" and "This is an ideal place for these checks," but the body no longer raises any error for those conditions -- every branch now returns silently and the function performs no check at all. Under the PostgreSQL comment-accuracy discipline, comments must describe what the code does now. Please rewrite this header to explain that, under HOT-indexed updates, a stale index entry can legitimately reach an out-of-range/LP_UNUSED/heap-only target and that deletability is decided by the caller's chain walk instead.

---

📄 src/backend/access/heap/heapam.c (L8711-L8717)

index_delete_check_htid() is called unconditionally for every relation (heapam.c:8948), not only relations that use HOT-indexed updates. Converting these three ERRCODE_INDEX_CORRUPTED checks (TID past maxoff, TID into an LP_UNUSED slot, index entry pointing at a heap-only tuple) into silent returns therefore globally disables genuine index-corruption detection, including for tables that never take the HOT-indexed path where these conditions still unambiguously indicate corruption. The caller's chain walk (heap_hot_search_buffer) will conclude such chains are vacuumable and delete the entries, so real corruption is masked rather than surfaced. Consider gating the tolerant behavior on the relation actually supporting HOT-indexed updates, and continue to raise the corruption error otherwise, so amcheck-style detection is not lost for the common case.

---

📄 src/backend/access/heap/heapam.c (L5038-L5041)

This return; is unreachable: the preceding elog(ERROR, ...) never returns (it longjmps via the error-handling machinery). Dead code after elog(ERROR) is not the tree's convention; drop the trailing return.

💡 Suggested change

Before:

		elog(ERROR, "tuple concurrently deleted");

		return;
	}

After:

		elog(ERROR, "tuple concurrently deleted");
	}

---

📄 src/backend/access/heap/hot_indexed_stats.c (L138-L145)

The chain walk mishandles collapse-survivor stubs, producing wrong counts. A stub is an LP_NORMAL item with HEAP_INDEXED_UPDATED set and natts==0; it is NOT a chain member and forwards to the next member via t_ctid.offnum (HotIndexedStubGetForward), not via HEAP_HOT_UPDATED. Both authoritative walkers (heap_prune_chain_find_live in pruneheap.c and the index-scan walker in heapam_indexscan.c) detect stubs with HotIndexedHeaderIsStub, do not count them, and follow the stub forward link.

Here the loop unconditionally does len++ for any ItemIdIsNormal item, then only continues when HEAP_HOT_UPDATED is set. A mid-chain (or redirect-target) stub has neither HEAP_HOT_UPDATED nor a meaningful t_ctid block (its ip_blkid half is repurposed to stash write-time natts via HotIndexedStubSetBitmapNatts). Result: the stub is (1) over-counted as a member, and (2) the walk stops at it, dropping the entire tail -- so avg_chain_len/max_chain_len are systematically wrong on any chain that survived a collapse. Handle the stub explicitly before the HEAP_HOT_UPDATED test: skip counting and follow HotIndexedStubGetForward(thdr). Confidence: high.

💡 Suggested change

Before:

					thdr = (HeapTupleHeader) PageGetItem(page, chain_lp);
					len++;
					if (!(thdr->t_infomask2 & HEAP_HOT_UPDATED))
						break;
					/* HOT chains stay on one page; stop if the link leaves it. */
					if (ItemPointerGetBlockNumber(&thdr->t_ctid) != blk)
						break;
					cur = ItemPointerGetOffsetNumber(&thdr->t_ctid);

After:

					thdr = (HeapTupleHeader) PageGetItem(page, chain_lp);

					/*
					 * A collapse-survivor stub is a forwarding node, not a
					 * chain member: step through it without counting, following
					 * its same-page forward link (its t_ctid block half stores
					 * write-time natts, not a block number).
					 */
					if (HotIndexedHeaderIsStub(thdr))
					{
						cur = HotIndexedStubGetForward(thdr);
						continue;
					}
					len++;
					if (!(thdr->t_infomask2 & HEAP_HOT_UPDATED))
						break;
					/* HOT chains stay on one page; stop if the link leaves it. */
					if (ItemPointerGetBlockNumber(&thdr->t_ctid) != blk)
						break;
					cur = ItemPointerGetOffsetNumber(&thdr->t_ctid);

---

📄 src/backend/access/heap/hot_indexed_stats.c (L129-L129)

The loop guard len < maxoff bounds iterations but does not prevent revisiting the same offset in a corrupt cyclic chain, and once the stub-skip (continue) path is added, len no longer advances on stub hops, so a corrupt stub cycle could spin without hitting the len bound. Prefer bounding by a step counter incremented every iteration (like loops/nstubhops in pruneheap.c) rather than by len, so both member and stub hops count toward the bound. Confidence: moderate.

---

📄 src/backend/access/heap/hot_indexed_stats.c (L73-L74)

The relkind check ereports before the ACL check, so a caller lacking SELECT privilege on the relation still receives the "is not a table..." error, leaking the object's kind/existence. Sibling relation-inspection functions perform pg_class_aclcheck first. Move the aclcheck ahead of the relkind test. Confidence: moderate.

---

📄 src/backend/access/heap/vacuumlazy.c (L2823-L2823)

Removing Assert(nunused > 0) is not needed for the SIU change and weakens an existing invariant. A block only enters dead_items when the first pass recorded LP_DEAD items for it, and nunused == num_offsets after the loop, so nunused is always > 0 here. If you keep the removal, note that the comment just below ("The page is guaranteed to have had dead line pointers") relies on exactly this invariant — dropping the assert removes its documented enforcement. Prefer keeping the assert unless a caller path can genuinely pass num_offsets == 0; if so, that path should be justified in the commit message.

---

📄 src/backend/access/index/indexam.c (L713-L714)

Performance: RelationGetIndexedAttrs() is invoked per fetched tuple whenever the HOT chain walk crossed a HOT-indexed hop (xs_hot_indexed_recheck). Its fast path returns bms_copy(rd_indattr) (see relcache.c: "return a copy of the cached bitmap"), so this allocates a fresh Bitmapset and then bms_free's it on every such tuple. On the HOT-indexed-update workload this feature targets, this branch is hit frequently, adding per-tuple palloc/bms_copy/pfree churn to the index-scan hot path.

The index's indexed-attrs set is stable for the lifetime of a scan, so compute it once (e.g. cache it on IndexScanDescData, freed in IndexScanEnd) rather than per tuple. The bms_free here is correct w.r.t. ownership; the concern is purely the repeated per-tuple allocation.

---

📄 src/backend/access/heap/pruneheap.c (L2669-L2670)

The comment claims a redirect targets "the first surviving member: a heap-only tuple", but the collapse logic sets root_target = next_survivor, and next_survivor may be a stub offset (a stub becomes the new survivor at the point it is recorded in the backward loop). So this redirect can legitimately target a collapse-survivor stub, which is an xid-free forwarding node rather than a surviving tuple. The assert still holds (a stub keeps HEAP_ONLY_TUPLE), but the comment is inaccurate and misleading for the next reader. Reword to state the target is a heap-only item that is either a surviving tuple or a forwarding stub.

---

📄 src/backend/access/heap/pruneheap.c (L3313-L3313)

The no-cleanup-lock WAL assertion was weakened from Assert(nredirected == 0 && ndead == 0) to Assert(ndead == 0) and the comment justifies it with vacuum's second pass "re-points the root redirect at first_live". But no producer in this series actually emits a redirect (nredirected > 0) without a cleanup lock: the execute-side assertion just above (Assert(!lp_truncate_only || (nredirected == 0 && ndead == 0))) was deliberately kept stricter with a comment stating exactly that. These two asserts now express contradictory assumptions, and the loosened one plus its narrative describe a code path that does not exist (dead flexibility / aspirational comment). Restore Assert(nredirected == 0 && ndead == 0) here (and the /* also, any items in 'unused' must've been LP_DEAD previously */ note) to match the actual producers, or wire up the described re-point-without-cleanup-lock path if it is truly intended.

---

📄 src/backend/access/nbtree/nbtree.c (L411-L419)

This 9-line comment is verbose and largely restates the new xs_index_only field documentation added in relscan.h (the "whereas a plain scan that sets xs_want_itup only to inspect the index tuple still fetches the heap and may drop pins" wording is nearly identical). Consider trimming to a couple of lines that state the why specific to this predicate and referencing the field, to keep the diff minimal and avoid two copies of the rationale drifting out of sync. Low confidence / non-blocking style note.

---

📄 src/backend/access/nbtree/nbtinsert.c (L18-L19)

The access/htup_details.h include appears unnecessary. The added code uses index_getattr (from access/itup.h, already pulled in via access/nbtree.h), index_getprocinfo (from the newly-added access/genam.h), and slot_getattr/TupleTableSlot/TTS_EMPTY/ExecClearTuple/table_slot_create (from executor/tuptable.h / access/tableam.h). No symbol requiring access/htup_details.h is referenced in this file's new code. Per minimal-diff / patch-hygiene discipline, drop the unused include (confidence: moderate -- transitive header inclusion can be subtle, please double-check with a clean build).

💡 Suggested change

Before:

#include "access/genam.h"
#include "access/htup_details.h"

After:

#include "access/genam.h"

---

📄 src/backend/access/nbtree/nbtinsert.c (L586-L588)

This lazy-init-inside-the-condition idiom is hard to read and easy to misparse: table_slot_create() never returns NULL (it errors instead), so the || branch is effectively (create-and-assign, always true), existing only for its side effect. Consider creating chain_walk_slot up front (or in a small helper) so the else if reads as a plain table_index_fetch_tuple_check(...) call. This keeps the conflict-detection condition readable and avoids a subtle side-effecting boolean expression on a correctness-critical path.

---

📄 src/backend/catalog/indexing.c (L26-L28)

These two new includes are unused. Grepping the full file shows no Bitmapset/bms_* symbols and no relcache-specific symbol (the Relation/RelationGetDescr uses are satisfied by the already-included utils/rel.h; the only relcache hits are in comment prose). Unused includes are needless churn and violate the minimal-diff discipline; drop both. (access/tableam.h is also suspect now that the TU_UpdateIndexes enum is gone and no table_* API is called here, but at least TupleTableSlot/MakeSingleTupleTableSlot come via the pre-existing executor/executor.h -- verify whether tableam.h is still needed at all.)

💡 Suggested change

Before:

#include "nodes/bitmapset.h"
 #include "utils/rel.h"
+#include "utils/relcache.h"

After:

 #include "utils/rel.h"

---

📄 src/backend/catalog/indexing.c (L144-L147)

Stale local variable / dead indirection. index_unchanged is just !update_all_indexes, is never mutated afterward, and is used exactly once on the next line. This adds no clarity over testing !update_all_indexes directly and reads as leftover scaffolding from the removed onlySummarized logic. Fold it into the if.

💡 Suggested change

Before:

		index_unchanged = !update_all_indexes;

		if (index_unchanged && !indexInfo->ii_Summarizing)
			continue;

After:

		if (!update_all_indexes && !indexInfo->ii_Summarizing)
			continue;

---

📄 src/backend/catalog/indexing.c (L141-L142)

Comment drift: this says summarizing indexes 'always get a chance to update their block-level summaries below', but the code below has no summarizing-specific branch -- it performs the same FormIndexDatum + index_insert(..., UNIQUE_CHECK_NO, ...) as any other index. The phrase 'block-level summaries' describes BRIN internals that this function does not touch; it will mislead future readers. State plainly that summarizing indexes are not skipped and get a normal index_insert call.

💡 Suggested change

Before:

		 * non-summarizing indexes are skipped (summarizing indexes always get
		 * a chance to update their block-level summaries below).

After:

		 * non-summarizing indexes are skipped (summarizing indexes are not
		 * skipped and still get an index_insert call below).

---

📄 src/backend/catalog/system_views.sql (L874-L875)

Backward-compatibility concern (moderate confidence): these two columns are inserted before the existing stats_reset column in pg_stat_all_indexes, shifting stats_reset's ordinal position. pg_stat_all_indexes is a long-established, widely-consumed view; any tool or query relying on positional column access (e.g. SELECT * ordinals) will break. Historically new columns are appended at the end of these stat views to preserve ordering. Recommend placing the new columns after stats_reset.

Also a design point worth calling out on-list: pg_stat_all_indexes covers every index (btree, gist, gin, brin, ...), yet these counters are meaningful only for the HOT-indexed feature. Confirm the counters read as zero/consistent for indexes that never participate, so the columns aren't misleading for the vast majority of rows.

---

📄 src/backend/executor/execTuples.c (L69-L69)

This #include "utils/datum.h" appears unused: none of the symbols declared in datum.h (datumCopy, datumGetSize, datumTransfer, datumIsEqual, datum_image_eq, datum_image_hash, datumEstimateSpace, datumSerialize, datumRestore) are referenced anywhere in execTuples.c. An unused header addition is non-minimal-diff churn that reviewers will flag; drop it unless a companion change in this file actually uses datum.h. (moderate confidence)

---

📄 src/backend/executor/execReplication.c (L971-L971)

This assertion is a can't-happen invariant that can actually happen and is user-reachable. ExecUpdateModifiedIdxAttrs returns RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_INDEXED) filtered by table_modified_attrs. When an index expression (or predicate) references a whole-row Var (varattno == 0), pull_varattnos records attribute 0 in the INDEXED bitmap, i.e. 0 - FirstLowInvalidHeapAttributeNumber == TableTupleUpdateAllIndexes. If that whole-row value changed, the sentinel survives table_modified_attrs and is present on input. The tableam.h contract (lines 1648-1652) and the parallel path in nodeModifyTable.c (lines 2662-2669) explicitly document this as legitimate, and neither nodeModifyTable.c nor repack.c adds such an assert. Replaying a logical-replication UPDATE on a table with a whole-row-Var index expression would trip this Assert in assert-enabled builds. Drop the assertion.

---

📄 src/backend/executor/execReplication.c (L36-L36)

Unnecessary include. utils/rel.h (included just above at line 35) already includes utils/relcache.h, and nothing newly added in this diff references a symbol that requires it (the RelationGetIndexAttrBitmap calls in this file predate this change). This is unrelated churn; drop it to keep the diff minimal.

---

📄 src/backend/executor/execIndexing.c (L917-L922)

The reworked self-tuple guard weakens genuine duplicate-TID corruption detection. Once any stale arrival is seen for a TID, found_self_siu_hit latches true, and every subsequent arrival for that TID is tolerated -- including a non-stale one that would previously have raised the "found self tuple multiple times" corruption error. So a real duplicate-TID index corruption that happens to be preceded by a legitimate stale chain-walk entry for the same TID is now silently swallowed. The prior code treated any repeat as "should not happen". Consider bounding tolerance to the exact expected count (one canonical + the observed stale entries) rather than latching an unbounded allow-list, so genuine corruption is still caught.

---

📄 src/backend/executor/execIndexing.c (L481-L488)

DRY: these two index_insert() calls are identical except for the TID argument (&siu_tid vs tupleid). Duplicating the full 7-argument call is error-prone -- a future signature change must be edited in both places. Prefer computing the TID pointer once and issuing a single 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);

---

📄 src/backend/executor/execIndexing.c (L367-L369)

Footgun: ii_IndexNeedsUpdate is read here to drive a skip decision, but it is populated only by a separate ExecSetIndexUnchanged() call that this function does not enforce. The old code self-guarded via ii_CheckedUnchanged and recomputed on demand; that guard is gone. Because makeNode zero-inits the flag to false, a caller that passes EIIT_IS_UPDATE but forgets ExecSetIndexUnchanged() would silently skip the index (missing index entries -> data loss / wrong results), rather than failing safe by inserting. All three current UPDATE callers pair the calls correctly, but the unsafe default plus the split responsibility is a silent-corruption hazard for future callers. Consider an Assert that the flags were populated for this row, or defaulting to "needs update".

---

📄 src/backend/executor/nodeIndexscan.c (L300-L304)

This relaxes a "should not happen" corruption guard. Once any stale self-arrival latches found_self_siu_hit=true, every subsequent duplicate self-TID arrival is tolerated unconditionally -- including non-stale ones that the original elog(ERROR) was there to catch. A genuine duplicate-TID index corruption on a row that also happened to have a stale HOT-indexed leaf would now be silently swallowed rather than reported. The expected count of legitimate self arrivals for a HOT-indexed chain is bounded (one direct + the stale chain-walk entries); consider bounding the tolerance to what the chain can actually produce, or at least asserting the stale entry rechecked to a different key, so real corruption is still surfaced.

---

📄 src/backend/replication/logical/worker.c (L6087-L6088)

The comment contradicts itself. The header says "Return the cached HOT-indexed apply mode", but the body comment below ("Derive directly from MySubscription rather than caching, so there is no second copy to keep in sync") states the opposite. The value is not cached; it is read live from MySubscription. Drop "cached" here (and in logicalworker.h line 28, which also says "cached") so the comment describes what the code actually does. [moderate confidence]

💡 Suggested change

Before:

 * Return the cached HOT-indexed apply mode of the current logical replication
 * worker's subscription.

After:

 * Return the HOT-indexed apply mode of the current logical replication
 * worker's subscription.

---

📄 src/backend/nodes/tidbitmap.c (L366-L367)

The 18-line header comment restates design context (BitmapAnd/BitmapOr semantics, the tbm_intersect_page proof, the "single choke point" argument, AM-independence) that is already documented in README.HOT-INDEXED and itemptr.h. On -hackers this length of narrative in a function header tends to draw "move the rationale to the README, keep the header terse" feedback, and it risks drifting out of sync with the three other copies. Consider trimming to the essential "what" plus a pointer to the README/itemptr.h for the "why". Low confidence / non-blocking style nit.

---

📄 src/backend/executor/nodeModifyTable.c (L2524-L2525)

Memory leak of modified_attrs on the retry and early-return paths (high confidence).

The bitmap is allocated in ExecUpdateAct via ExecUpdateModifiedIdxAttrs (a fresh copy from RelationGetIndexAttrBitmap, palloc'd in the statement-lifetime per-query context, per the comment at the bms_free site). It is only ever freed in ExecUpdateEpilogue, which is reached only on TM_Ok.

When table_tuple_update returns TM_Updated, control does EPQ and goto redo_act; ExecUpdateAct re-enters and this line resets the pointer to NULL without freeing the previous allocation. Every EPQ retry (unbounded under concurrent-update contention) leaks one Bitmapset. Likewise the TM_SelfModified/TM_Deleted cases return NULL without ever reaching the epilogue, leaking one Bitmapset per affected row for the statement's lifetime.

Free the previous bitmap before overwriting the pointer, and ensure the non-TM_Ok exit paths in ExecUpdate also free it.

💡 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 */
	bms_free(updateCxt->modified_attrs);
	updateCxt->modified_attrs = NULL;

---

📄 src/backend/executor/nodeModifyTable.c (L1997-L1999)

Related to the modified_attrs leak: several branches in this switch return NULL after ExecUpdateAct has already allocated updateCxt.modified_attrs (e.g. TM_SelfModified returning NULL, TM_Deleted, and the nested TM_Deleted/TM_SelfModified cases in the TM_Updated handler). None of these reach ExecUpdateEpilogue, which is the only place the bitmap is freed, so each such row leaks one Bitmapset in the statement-lifetime per-query context. Ensure the bitmap is freed on every non-TM_Ok exit path (e.g. free it right after the switch/before the early returns, or scope its lifetime to a per-tuple context).

---

📄 src/backend/utils/activity/pgstat_relation.c (L397-L397)

Memory leak on the EPQ retry path. ExecUpdateModifiedIdxAttrs() allocates modified_attrs (a fresh per-query-context bitmap via RelationGetIndexAttrBitmap()) before table_tuple_update(), and only ExecUpdateEpilogue() frees it -- and only on TM_Ok. On the TM_Updated concurrent-update path the code does goto redo_act, which re-enters ExecUpdateAct() and unconditionally sets updateCxt->modified_attrs = NULL here, orphaning the previously allocated bitmap. Every retry under contention thus leaks one bitmap into the statement-lifetime per-query context. The old TU_UpdateIndexes enum did not allocate, so this is a regression. Free before nulling.

💡 Suggested change

Before:

Assert(!(hot_indexed && !hot));

After:

	/* Reset any state left over from a previous call */
	bms_free(updateCxt->modified_attrs);
	updateCxt->modified_attrs = NULL;

---

📄 src/include/access/amapi.h (L30-L32)

Unrelated whitespace-only change. This diff removes a blank line between two forward-declaration blocks in amapi.h, which has nothing to do with the patch's stated purpose. Cosmetic churn on untouched lines is a top rejection reason on pgsql-hackers (it pollutes git blame, causes needless merge conflicts, and breaks the minimal-diff discipline). 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 hard-depends on the amcheck contrib extension: CREATE EXTENSION amcheck plus verify_heapam()/bt_index_check() drive every assertion. Unlike the amcheck contrib tests (contrib/amcheck/t/*.pl), a test under src/bin/pg_upgrade/t/ cannot assume amcheck is built/installed, and amcheck is not declared as a dependency of the pg_upgrade test suite. As written the test hard-fails (rather than skips) when amcheck is unavailable, violating the "skippable without optional prerequisites" rule.

The canonical peer test 006_transfer_modes.pl guards its contrib dependency by checking $node->check_extension('dummy_seclabel') on both the old and new clusters and degrading gracefully. Add an equivalent guard here, e.g. plan skip_all => 'amcheck not installed' unless $oldnode->check_extension('amcheck'); (checking both clusters), so the test skips cleanly instead of failing.

---

📄 src/bin/pg_upgrade/t/009_hot_indexed.pl (L103-L103)

The expected count is hardcoded as the literal '4' (PRIMARY KEY plus hi_k, hi_v, hi_big). This is brittle: a count(*) of one row per bt_index_check result cannot detect a missing index (an index silently absent from pg_index would just lower the count, and any accompanying schema tweak forces manual re-counting). Consider asserting that no index fails instead of counting successes -- e.g. select the indexes and assert that every bt_index_check returns without error -- which is robust to the exact index count and directly expresses intent.

---

📄 src/backend/utils/cache/relcache.c (L5630-L5642)

Predicate columns of a predicate-only partial index (a WHERE clause but no expression key, e.g. CREATE INDEX ON t (a) WHERE b > 0) are never added to exprindexattrs, because this whole block is gated on indexExpressions != NULL. Consequence: HeapUpdateHotAllowable's INDEX_ATTR_BITMAP_EXPRESSION disqualification (heapam.c ~L4752) will NOT fire for an UPDATE that changes only a predicate column. Column b is still in indexedattrs (added at the pull_varattnos(indexPredicate, 1, attrs) above), so the update takes the selective path and inserts a positive index entry -- even when the new tuple now fails the predicate and should have no entry at all. Deciding membership requires evaluating the partial-index predicate, which the recheck path (_bt_heap_keys_equal_leaf) explicitly does not do (see README.HOT-INDEXED sec 4, which claims INDEX_ATTR_BITMAP_EXPRESSION "captures every such attribute"). Either fold predicate vars into exprindexattrs unconditionally (i.e. also when indexPredicate != NULL), or confirm predicate re-evaluation is handled on the selective-maintenance path. Confidence: high (bitmap contents), moderate (downstream corruption depends on the selective path, out of this file).

---

📄 src/backend/utils/cache/relcache.c (L5376-L5380)

On a cache miss for an expression/partial index, the List produced by stringToNode() and the C string from TextDatumGetCString() are allocated in the caller's current memory context and never freed. Unlike the existing RelationGetIndexAttrBitmap (which runs during a controlled relcache-build path), this function is invoked from hot write/scan paths -- ExecSetIndexUnchanged (execIndexing.c) and the index_getnext recheck (indexam.c). The rd_indattr cache bounds this to one leak per index per relcache-validity window, but if that first call lands in a long-lived context (e.g. a per-query or session-lifetime index-scan context rather than a per-tuple one) the parse trees persist until that context resets. Consider parsing into (or immediately pfree'ing within) a short-lived context, matching the intent of the surrounding relcache code. Confidence: moderate.

---

📄 src/include/access/hot_indexed.h (L182-L184)

Add a defensive Assert that natts is within the valid attribute range before smuggling it into the block half of t_ctid. This function's contract (documented above) relies on natts being bounded by MaxHeapAttributeNumber and non-negative, and every consumer (heapam_indexscan.c, pruneheap.c) already Asserts bmnatts <= relnatts after reading it back. Asserting the invariant at the point of storage documents it and catches a bad caller closer to the source. (moderate confidence)

💡 Suggested change

Before:

HotIndexedStubSetBitmapNatts(HeapTupleHeaderData *tup, int natts)
{
	BlockIdSet(&tup->t_ctid.ip_blkid, (BlockNumber) natts);

After:

HotIndexedStubSetBitmapNatts(HeapTupleHeaderData *tup, int natts)
{
	Assert(natts >= 0 && natts <= MaxHeapAttributeNumber);
	BlockIdSet(&tup->t_ctid.ip_blkid, (BlockNumber) natts);

---

📄 src/include/access/hot_indexed.h (L99-L101)

This block comment opens with substantive text on the first line, inconsistent with the multi-line block-comment style used everywhere else in this file (opening /* on its own line). Also the wording says "OR the first nbytes of src", but the parameter is src_natts (nbytes is derived internally); referring to the actual parameter avoids drift. Reflow for consistency and pgindent cleanliness. (low confidence)

💡 Suggested change

Before:

/* OR the first nbytes of src into dst.  dst must be at least nbytes long; it
 * may be longer (sized for a larger natts) -- bit positions are attribute
 * based and identical across sizes, so OR-ing only src's bytes is correct. */

After:

/*
 * OR src (sized for src_natts) into dst.  dst must be at least as long as
 * src; it may be longer (sized for a larger natts) -- bit positions are
 * attribute based and identical across sizes, so OR-ing only src's bytes is
 * correct.
 */

---

📄 src/include/catalog/catversion.h (L60-L63)

This patch bumps CATALOG_VERSION_NO (and adds an explanatory comment block) in catversion.h. This is a committer-owned file: the catversion bump is done by the committer at push time, not in the patch. Including it here causes needless rebase/merge conflicts and is the single most common author mistake in catalog patches. Remove this entire hunk. The catalog column change to pg_subscription belongs in pg_subscription.dat/the header, and the committer will bump catversion when pushing.

💡 Suggested change

Before:

+/* XXX bump catversion -- this commit adds a catalog column
+ * (subhotindexedonapply) and touches pg_subscription; the committer
+ * sets the real value at commit time to avoid needless rebase churn. */
 #define CATALOG_VERSION_NO	202607171

After:

 #define CATALOG_VERSION_NO	202607171

---

📄 src/include/catalog/pg_subscription.h (L236-L236)

British spelling "behaviour" is inconsistent with PostgreSQL's American-English convention used throughout the tree (e.g., "behavior"). Use "behavior" to match the codebase style. (low confidence: style nit)

💡 Suggested change

Before:

 *		  behaviour before this option was introduced.

After:

 *		  behavior before this option was introduced.

---

📄 src/include/access/relscan.h (L160-L163)

These field comments encode cross-module invariants whose enforcement lives elsewhere (pruneheap.c chain collapse, heapam.c bitmap maintenance): "The union is complete (every crossed live hop and collapse-survivor stub contributes its bitmap, and collapse only reclaims members subsumed by surviving hops)". A struct-field comment in a public header cannot be kept in sync with that logic; if the collapse rule in pruneheap.c changes, this becomes silently stale/aspirational. Per PostgreSQL comment discipline (explain why, not restate an algorithm proven elsewhere), keep the field comment to the field's contract ("union of per-hop modified-attr bitmaps crossed after the entry's tuple; bit attnum-1; NULL for AMs without such chains; sized/owned by the table AM") and move the completeness/collapse-invariant proof to the single authority that maintains it (pruneheap.c / README.HOT-INDEXED). Same applies to the equally long xs_prefix_all_dead comment below.

---

📄 src/include/pgstat.h (L772-L781)

These two macros do not align their line-continuation backslashes with the surrounding macros (and the tail backslash on the counter-increment line is butted directly against the ; with no separating tab). This will not pgindent cleanly and produces whitespace churn inconsistent with every other macro in this block. The macro-name line is longer than the neighbors, so the \ on the #define line and on the long counts.tuples_hot_indexed_upd_*++; line sit at a different column than the do {/if lines. Run pgindent (or hand-align) so all backslashes in each macro line up at the same column, matching pgstat_count_index_tuples above.

---

📄 src/include/utils/rel.h (L221-L229)

This comment describes a consumer contract that does not exist. rd_indattr is a private relcache field: the only accessor, RelationGetIndexedAttrs(), always returns bms_copy(rd_indattr) (never the cached pointer), and its header in relcache.h explicitly states "We never hand out a borrowed pointer to the cached copy." No code outside relcache.c ever reads rd_indattr. Telling "consumers" to bms_copy the pointer is misleading and contradicts the actual API. Recommend describing it like the other cached bitmaps (e.g. rd_indexprs/rd_indpred), i.e. "cached in rd_indexcxt; do not access directly, use RelationGetIndexedAttrs()", and drop the AcceptInvalidationMessages sentence. (moderate confidence)

💡 Suggested change

Before:

	/*
	 * Bitmap of heap attribute numbers referenced by this index (simple keys,
	 * INCLUDE columns, expression columns, and partial-index predicate
	 * columns), offset by FirstLowInvalidHeapAttributeNumber. Lazily built by
	 * RelationGetIndexedAttrs() and cached in rd_indexcxt. Consumers must
	 * bms_copy before relying on the pointer beyond any potential
	 * AcceptInvalidationMessages() call.
	 */
	Bitmapset  *rd_indattr;

After:

	/*
	 * Bitmap of heap attribute numbers referenced by this index (simple keys,
	 * INCLUDE columns, expression columns, and partial-index predicate
	 * columns), offset by FirstLowInvalidHeapAttributeNumber.  Lazily built
	 * and cached in rd_indexcxt by RelationGetIndexedAttrs(); do not access
	 * directly, call RelationGetIndexedAttrs() instead.
	 */
	Bitmapset  *rd_indattr;

---

📄 src/include/utils/relcache.h (L94-L95)

The safer of the two new functions (RelationGetIndexedAttrs, which returns caller-owned memory) got a full header comment, while the genuinely dangerous one (RelationGetIndexAttrBitmapNoCopy) got none. This function returns a borrowed pointer into the relcache entry whose lifetime ends at the next AcceptInvalidationMessages() (any relation open / catalog lookup between fetch and last use is a use-after-free footgun). That safety contract currently lives only in relcache.c. Add at least a one-line header comment pointing callers to the lifetime/no-mutate rules so consumers reading only the header are warned. (low confidence on severity, high confidence the omission is inconsistent with the sibling declaration.)

---

📄 src/test/benchmarks/siu/scripts/bit14_ab.sh (L98-L99)

Non-portable parsing: grep -oP uses PCRE (\K), which is a GNU-grep-only extension and is unavailable on macOS/BSD/Solaris grep. This directly conflicts with PostgreSQL's portability gate. The sibling harness run.sh parses the exact same pgbench output portably with awk (tps=$(awk '/tps = /{print $3; exit}' ...) and lat=$(awk '/latency average = /{print $4; exit}' ...)). Use the same awk approach here for consistency and portability.

💡 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/include/storage/itemptr.h (L197-L197)

ItemPointerGetOffsetNumber is one of the hottest inline accessors in the backend (called from thousands of sites across heap/index/executor/replication scans). This change adds a compare-and-branch (raw >= MovedPartitionsOffsetNumber) plus a mask on EVERY call, on paths that never carry the SIU flag. For a feature whose flag is only ever set on index-tuple heap TIDs, taxing every offset read is a hot-path regression. Consider whether the strip belongs only in the small set of comparison/consumer sites (ItemPointerCompare/Equals and amgetbitmap) rather than in the universal accessor, and back the current choice with a benchmark on scan-heavy workloads.

---

📄 src/include/storage/itemptr.h (L83-L84)

This ~40-line design-notes block in a core, extremely-widely-included header exceeds what belongs here per PostgreSQL comment discipline (explain WHY concisely; keep full scenarios in a README/thread). More importantly, 'see the HOT-indexed design notes' is a dangling reference -- there is no in-tree document of that name to point a reader at. Trim the narrative to the essential invariant and replace the vague pointer with a concrete reference (e.g. src/include/access/hot_indexed.h, which the executor-side comment already cites, or the -hackers Message-Id for the design thread).

---

📄 src/include/storage/itemptr.h (L221-L221)

Style nit: Assert(pointer) here is a NULL-check on an inline setter, not a can't-happen invariant of the kind Assert is meant for -- every other accessor in this header (e.g. ItemPointerIsValid callers) relies on the caller passing a valid pointer without such a guard. It is inconsistent with the surrounding code and adds no value in production builds. Consider dropping it; the following offset assertion already documents the real precondition.

---

📄 src/test/benchmarks/siu/scripts/bitmap_and_mixed.sql (L24-L24)

The seed data makes this workload unable to exercise what the header comment claims. The table is seeded with a=b=c=d=i (see seed() in bit14_ab.sh: SELECT i, i, i, i, ...), so b and d hold identical values on every row. The predicate b BETWEEN :lo AND :lo+:hi_off AND d BETWEEN :lo AND :lo+:hi_off is therefore two logically identical clauses, and any BitmapAnd the planner builds ANDs a bitmap with an identical copy of itself. That is a degenerate case: it does not represent "a BitmapAnd across siu_b and siu_d" as two distinct index contributions, so the measurement of exact-vs-lossy transitions across different indexes is not actually exercised. Seed b and d with different value distributions (e.g. d = rows - i or a shuffled/independent series) so the two index bitmaps are genuinely distinct. Confidence: high.

---

📄 src/test/benchmarks/siu/scripts/build.sh (L12-L12)

This benchmark script is developer-local tooling, not committable PostgreSQL source. It hardcodes developer-specific absolute paths ($HOME/ws/postgres/tepid, /scratch/siu-bench, /scratch/pg) and a private branch name (tepid) that have no meaning in the upstream tree. src/test/benchmarks/ is a newly invented top-level test category with no precedent in the tree. On pgsql-hackers, performance claims need a reproducible benchmark, but the harness itself should not be committed here. Recommend dropping this entire directory from the patch (post benchmark results/scripts to the -hackers thread instead).

---

📄 src/test/benchmarks/siu/scripts/build.sh (L22-L23)

MASTER_REV resolution depends on the private branch name 'tepid' existing. On any machine other than the author's, TEPID_REV defaults to the non-existent ref 'tepid', so both git merge-base calls fail and MASTER_REV ends up empty (the later die fires) or, if only origin/master is missing, git checkout --detach "$rev" fails on the bogus tepid ref. The script is non-reproducible outside the author's environment, undermining its stated purpose. Defaults must not assume a private branch name.

---

📄 src/test/benchmarks/siu/scripts/build.sh (L47-L47)

meson install --destdir=/ combined with an absolute --prefix under $BENCH is a footgun: with set -e the EXIT trap restores the original ref, but a misconfigured BENCH could cause the install step to write into unexpected filesystem locations (the install writes to $destdir/$prefix). Additionally, if ORIG was captured via git rev-parse HEAD (started detached), the trap silently reattaches to a raw SHA. These are robustness concerns for a script that mutates the working tree via git checkout.

---

📄 src/test/benchmarks/siu/scripts/hot_indexed_mixed.sql (L7-L7)

The SELECT reads via the PK index on column a (siu_table's PRIMARY KEY, per run.sh's seed_siu_table), while the UPDATE dirties column b (index siu_b). Column a is never updated, so the PK index has no stale/may-be-stale entries and the read path never touches siu_b/siu_c/siu_d. Consequently this workload does NOT "exercise ... the crossed-attribute-bitmap reader" as the comment claims -- it only exercises the writer. To measure the reader overhead you'd need the SELECT to scan a secondary index that receives the HOT-indexed updates, e.g. SELECT * FROM siu_table WHERE b = :bid (forcing an index scan on siu_b) rather than the PK equality on a. As written the comment overstates what the benchmark covers. (moderate confidence)

---

📄 src/test/benchmarks/siu/scripts/bloat.sh (L26-L26)

Default PORT collides with the sibling bit14_ab.sh, which also defaults PORT to 57481. If a bit14 cluster is up (or leftover) on this port, bloat.sh's fresh cluster will fail to bind, or worse, psql here can connect to the other cluster. Each throwaway benchmark should pick a distinct default port. Confidence: high.

💡 Suggested change

Before:

PORT=${PORT:-57481}

After:

PORT=${PORT:-57482}

---

📄 src/test/benchmarks/siu/scripts/bloat.sh (L75-L76)

P() runs psql -At, but the final reporting P -c "SELECT ..." produces a single concatenated string; the -t (tuples-only) plus -A here is fine, yet the whole benchmark's key output relies on n_tup_hot_indexed_upd_skipped being non-zero. Because the query is wrapped in coalesce(..., 0), a future rename of this stats column (or an empty stat snapshot) would silently report idx_b_skips=0, i.e. a false negative that still looks like a successful run. Consider dropping the coalesce (or asserting the row exists) so a missing column/row surfaces as an error instead of masquerading as a real zero. Confidence: moderate.

💡 Suggested change

Before:

        || ' idx_b_skips=' || coalesce((SELECT n_tup_hot_indexed_upd_skipped
                                        FROM pg_stat_all_indexes WHERE indexrelname='${tbl}_b'), 0);"

After:

        || ' idx_b_skips=' || (SELECT n_tup_hot_indexed_upd_skipped
                               FROM pg_stat_all_indexes WHERE indexrelname='${tbl}_b');"

---

📄 src/test/benchmarks/siu/scripts/hot_indexed_update.sql (L4-L4)

:scale will resolve to pgbench's default of 1 here, not the intended SCALE (e.g. 20). pgbench only queries the real scale from pgbench_branches (GetTableInfo) when an internal/built-in script is used; for a pure custom script run with -f and no -D scale=..., the global scale stays at its default (1) and :scale is set from it. In run.sh this script is dispatched via the default *) case, which does NOT pass -D scale=$SCALE (unlike the wide_* and read_indexscan cases). As a result aid only spans [1, 100000] instead of [1, SCALE*100000], so the workload hammers the first 1/20th of siu_table, producing heavy artificial contention and misleading A/B numbers. Either pass -D scale=$SCALE for this workload in run.sh, or drive the row count via an explicit variable (e.g. :rows) like read_indexscan.sql does. The same latent issue affects hot_indexed_mixed.sql, which shares the default dispatch case.

---

📄 src/test/benchmarks/siu/scripts/bloat.sh (L1-L2)

Patch-hygiene / minimalism concern (most serious for a -hackers submission): this file is one of several new manual benchmark scripts under src/test/benchmarks/siu/scripts/ (bloat.sh, run.sh, soak.sh, build.sh, bit14_ab.sh, plus .sql drivers) that are not referenced by any Makefile, meson.build, or CI harness (confirmed: benchmarks/siu has zero matches outside these files). Combined with the stray dotfiles in the same change (.clangd, .gdbinit, pg-aliases.sh), this looks like developer-local tooling. A benchmark that backs a performance claim belongs in the mailing-list thread, not committed to the tree, unless it is wired into an agreed test framework. Recommend dropping these from the patch (or gating them behind an explicit, documented benchmark harness). Confidence: high.

---

📄 src/test/isolation/isolation_schedule (L131-L131)

Minor style nit (low confidence): every other test name in this schedule uses hyphens as word separators (e.g. lock-nowait, for-portion-of, ddl-dependency-locking), while this new entry uses underscores. For consistency with the file's convention, consider renaming the spec/expected files and this entry to hot-indexed-adversarial. Not a correctness issue.

💡 Suggested change

Before:

test: hot_indexed_adversarial

After:

test: hot-indexed-adversarial

---

📄 src/test/benchmarks/siu/scripts/soak.sh (L99-L99)

pg_table_size() includes the TOAST relation plus the FSM/VM forks, so dividing it by 8192 does not give the heap page count. The variable and CSV column are named heap_pages, which will mislead any analysis that treats this as the heap size. Use pg_relation_size('siu_table')/8192 for the actual main-fork heap pages (or rename the column, e.g. table_size_pages, to match what is measured). Note run.sh computes the same expression but names it bloat_pages, avoiding the heap-specific claim.

💡 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')/8192")

---

📄 src/test/benchmarks/siu/scripts/soak.sh (L107-L107)

tps_i is derived from the delta of n_tup_upd (a cumulative pgstat counter) over SAMPLE seconds. pgstat counters are reported asynchronously and lag behind actual activity, so this is update-throughput-as-seen-by-the-stats-collector, not transaction throughput. pgbench is already invoked with -P "$SAMPLE" (line 87), so its log contains real periodic TPS. Labeling the stats delta as tps overstates precision for a benchmark that will inform performance claims; either rename it (e.g. upd_per_sec) or parse pgbench's progress output.

---

📄 src/test/benchmarks/siu/scripts/soak.sh (L30-L31)

Footgun: with BENCH user-overridable, an empty or misconfigured BENCH makes $datadir = /_data_$v, and find "$datadir" -mindepth 1 -delete then recursively deletes everything under that path with no confirmation. Add a guard before the destructive delete, e.g. [[ "$datadir" == "$BENCH/_data_$v" && -n "$BENCH" ]] || { echo 'refusing to delete unexpected datadir' >&2; exit 1; }. (run.sh's start_pg() shares this hazard; a shared helper would fix both.)

---

📄 src/test/benchmarks/siu/scripts/run.sh (L22-L23)

This entire benchmark subtree (run.sh plus the sibling build.sh/soak.sh/bloat.sh/bit14_ab.sh and .sql scripts, and root-level .clangd/.gdbinit/pg-aliases.sh in the same change) is developer-local scaffolding, not committable tree content. It hardcodes an absolute path /scratch/siu-bench, a fixed port 57480, and the private variant names 'master'/'tepid'. For a pgsql-hackers/commitfest patch this is a minimal-diff violation: it is unrelated to the HOT-indexed feature and will draw immediate rejection while obscuring the real change. Drop it from the patch (or keep it out-of-tree). [high confidence]

---

📄 src/test/benchmarks/siu/scripts/run.sh (L263-L264)

pgbench is run inside a 'set +e' region and its exit status is discarded; tps/lat are then derived purely by grepping the log with a '${tps:-NA}' fallback. A pgbench crash, connection failure, or non-zero exit is therefore silently recorded as an 'NA' CSV row that looks like a completed run rather than aborting. This is inconsistent with 'set -euo pipefail' at the top of the file and can produce misleading benchmark output with no visible error. Capture and check the pgbench exit status (e.g. save $? after the case and skip/flag the row on failure) instead of swallowing it. [high confidence]

---

📄 src/test/benchmarks/siu/scripts/run.sh (L65-L65)

Recursive delete on a computed path is a footgun. datadir=$BENCH/data$v, and although BENCH is defaulted and -mindepth 1 limits blast radius, an empty/unset BENCH or empty $v (e.g. from a sourced environment) would target '/data' or similar and recursively delete an unintended tree. Add a hard guard before the destructive op, e.g. refuse to run when BENCH or v is empty: '[ -n "${BENCH:-}" ] && [ -n "$v" ] || { echo "refusing: BENCH/v unset" >&2; exit 1; }'. [moderate confidence]

---

📄 src/test/benchmarks/siu/scripts/run.sh (L82-L83)

Fixed 'sleep 2' after start is a synchronization anti-pattern: on a loaded/slow machine 2s may be insufficient and the first psql_as call will fail; on a fast machine it just wastes time. pg_ctl already supports blocking until the server is ready. Use 'pg_ctl ... -w start' (or poll pg_isready) and drop the sleep. [moderate confidence]

💡 Suggested change

Before:

    -o "-p $PORT" -l "$LOGDIR/pg_$v.log" start >/dev/null
  sleep 2

After:

    -o "-p $PORT" -l "$LOGDIR/pg_$v.log" -w start >/dev/null

---

📄 src/test/benchmarks/siu/scripts/run.sh (L18-L18)

Stale comment: the header claims CPU/RSS is 'sampled via pidstat', but sample_peak() actually samples with pgrep + ps in a shell loop; pidstat is never invoked. Comments must describe what the code does now. Update this line to reflect the pgrep/ps sampler. [moderate confidence]

💡 Suggested change

Before:

+#   counts + WAL delta + peak CPU/RSS sampled via pidstat.

After:

#   counts + WAL delta + peak CPU/RSS sampled via pgrep + ps.

---

📄 src/test/benchmarks/siu/scripts/run.sh (L341-L342)

The 'wide_0' step emits an SET clause of 'id=id', but the comment says 'id % 1 so it's a no-op'. The code does not do any modulo; 'id=id' is simply a self-assignment. Also, updating the PRIMARY KEY column to its own value is a poor stand-in for the intended 'touch a non-indexed column' case, since wide_table has no non-indexed columns other than id (the PK). Fix the comment to match the code, and reconsider whether id=id measures what the wide_0 baseline is meant to measure. [moderate confidence]

---

📄 src/test/benchmarks/siu/scripts/run.sh (L271-L271)

The 'sed "s/:wide_set_clause/$extra_set/"' substitution is fragile: if $extra_set ever contained a '/' or '&' it would corrupt the sed expression and silently produce a broken/incorrect UPDATE, invalidating the benchmark with no error. For the values build_wide_set_clause currently emits ('cN=:v' / 'id=id') this happens to be safe, but the pattern is a latent footgun. Prefer a delimiter unlikely to collide (e.g. sed "s|:wide_set_clause|$extra_set|") or pass the clause via a pgbench -D variable rather than textual substitution. [moderate confidence]

---

📄 src/test/benchmarks/siu/scripts/run.sh (L223-L223)

sample_peak enumerates only direct children of the postmaster via 'pgrep -P "$leader"'. Postgres backends are direct children of the postmaster, so this generally works, but any deeper descendants (e.g. helper processes spawned by extensions) are missed, and pcpu from ps is a lifetime average rather than an instantaneous value, so 'peak CPU' here is not a true peak. For a harness whose purpose is a CPU/RSS A/B comparison this can systematically understate/mis-measure usage; consider using a recursive process-tree walk and an interval-based CPU sampler. [low confidence]

---

📄 src/test/regress/pg_regress.c (L1246-L1246)

This execl -> execlp switch appears unrelated to the rest of the change set (HOT indexed updates), and should be split into its own patch per PostgreSQL patch hygiene. Substantively, the switch is a no-op in every supported build config: shellprog derives from SHELLPROG, which is $(SHELL) (an absolute path, e.g. /bin/sh) in the make build and hardcoded "/bin/sh" in meson. execlp only differs from execl when the program name contains no slash, in which case it searches PATH. So with the current definitions there is no behavioral change, and if the definition ever became a bare name this would silently introduce a PATH-based shell lookup (a footgun for a test harness). No functional justification is present in the diff for making this change. Confidence: high.

💡 Suggested change

Before:

		execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL);

After:

		execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);

---

📄 src/test/regress/sql/generated_virtual.sql (L130-L130)

This diverges from generated_stored.sql, which line 1 of this file explicitly requires staying aligned with ("keep these tests aligned with generated_stored.sql"). The corresponding line 130 in generated_stored.sql is still SELECT * FROM gtest1v; with no ORDER BY. Both files reach this point with the same 4 rows (a = 3, 5, 6, 7) inserted across separate statements, so the physical-order nondeterminism this ORDER BY a guards against applies identically to the stored variant (the view seqscans the real heap column a in both cases). Either apply the same ORDER BY a fix to generated_stored.sql line 130 (and update its expected .out) to keep the pair aligned, or drop this hunk. Confidence: high.

---

📄 src/test/recovery/t/055_hot_indexed_recovery.pl (L2-L2)

The file starts with a spurious blank line, pushing the copyright header to line 2. Every sibling test in src/test/recovery/t/ (e.g. 054_unlogged_sequence_promotion.pl) begins the copyright header on line 1. Remove the leading blank line so the header is on line 1; git diff --check / pgindent conventions expect no leading blank.

💡 Suggested change

Before:

# Copyright (c) 2026, PostgreSQL Global Development Group

After:

# Copyright (c) 2026, PostgreSQL Global Development Group

---

📄 src/test/recovery/t/055_hot_indexed_recovery.pl (L122-L123)

Comment/code drift: the comment claims "skip_option = 'all-frozen' is the default", but the call passes skip := 'none'. The stated default is also wrong -- verify_heapam's SkipPages default is SKIP_PAGES_NONE (skip_option = SKIP_PAGES_NONE in verify_heapam.c), i.e. 'none', not 'all-frozen'. Fix the comment to match the code (scanning everything via skip := 'none').

💡 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.  Pass skip :=
#    'none' so every page is scanned regardless of visibility-map state.

---

📄 src/test/regress/sql/triggers.sql (L664-L664)

This added DELETE targets a row that does not exist at this point in the test. Tracing the data: the only a=21 row in main_view was inserted as (21,31) (line 648) and updated to (21,32) (line 657) after before_upd_a_row_trig was dropped; the a=20 row is (20,31). No row (21,10) is ever created, so this statement matches zero rows and should print only the BEFORE/AFTER DELETE STATEMENT notices with DELETE 0 (no INSTEAD OF DELETE ROW/OLD: notice).

The committed expected/triggers.out (lines 968-973), however, asserts this deletes (21,10) with DELETE 1 and an OLD: (21,10) notice, which is inconsistent with the actual behavior of the query above -- the regression test will fail. If the intent was to delete the existing a=21 row, that is already covered by the following WHERE a = 21 AND b = 32 statement; this line looks like a copy-paste error (wrong b value) and should be corrected or removed. (high confidence)

---

📄 src/tools/pgindent/pgindent (L1-L1)

This shebang change is unrelated to the patch's purpose (the HOT-indexed feature) and violates the minimal-diff discipline. It should be reverted.

Beyond being out-of-scope, it also breaks the tree's established convention: every other Perl script in the PostgreSQL source (e.g. genbki.pl, gen_node_support.pl, copyright.pl, mark_pgdllimport.pl, and all the generate-*.pl scripts) uses #!/usr/bin/perl. Switching this one file to #!/usr/bin/env perl introduces an inconsistency and would need a separate, justified discussion on -hackers if intended at all.

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

📄 src/test/subscription/t/040_hot_indexed_replica_identity.pl (L91-L91)

This test is a vacuous regression guard: it only compares final table contents between publisher and subscriber, so it would still pass with the HOT-indexed-on-apply feature reverted (plain non-HOT apply also converges). The whole point -- that RI lookups tolerate stale index leaves left by HOT-indexed updates -- is never actually asserted to have been exercised. The companion 039_hot_indexed_apply.pl already demonstrates the required pattern: poll pg_stat_user_tables.n_tup_hot_indexed_upd and assert it advanced, e.g.

my $hotidx = $subscriber->safe_psql('postgres',
q{SELECT n_tup_hot_indexed_upd FROM pg_stat_user_tables WHERE relname = 'tab_idx'});
cmp_ok($hotidx, '>', 0, 'apply took the HOT-indexed path on tab_idx');

(note the apply worker flushes pgstat asynchronously, so this needs a poll loop like 039's poll_counters, not a bare SELECT). Without this the test provides zero confidence that the new apply-path index lookup is being tested. [high confidence]

---

📄 src/test/subscription/t/040_hot_indexed_replica_identity.pl (L97-L98)

This scenario substantially duplicates the tab_ri block already present in 039_hot_indexed_apply.pl (REPLICA IDENTITY USING INDEX on a non-PK unique index, a secondary index NOT covered by the RI, an rid ABA cycle under hot_indexed_on_apply=always, followed by RI-driven UPDATE/DELETE convergence and a verify_heapam check). 039 additionally verifies n_tup_hot_indexed_upd fired. Please justify what 040 adds beyond 039, or fold the genuinely new coverage (the REPLICA IDENTITY FULL / seqscan path) into 039 and drop the redundant USING-INDEX repetition. The community rejects patches that do more than one thing or repeat existing coverage. [moderate confidence]

---

📄 src/test/subscription/t/040_hot_indexed_replica_identity.pl (L102-L105)

The comment above says the goal is that "stubs [are] recognised" and stale index leaves are tolerated, but the verification only runs verify_heapam. The stale leaves this test targets live in the indexes (tab_idx_k, tab_idx_v, tab_idx_w). Index-side corruption (duplicate/dangling leaf pointers from HOT-indexed apply) would go completely undetected here. Add verify_nbtree over the relevant indexes with heapallindexed => true, e.g.

SELECT bt_index_check('tab_idx_k', true);

for each secondary index, to actually exercise the corruption class the feature risks. [moderate confidence]

---

📄 src/test/subscription/t/040_hot_indexed_replica_identity.pl (L77-L77)

The claim that each single-column UPDATE "stays HOT-indexed on the subscriber and leaves stale leaves" rests solely on fillfactor=50 and the specific update sequence; nothing pins tuple placement or blocks a non-HOT update. If page layout differs (alignment/architecture) or the chain fills the page, an update may silently become non-HOT, degrading this into a plain replication-equality test. Combined with the missing n_tup_hot_indexed_upd assertion above, the coverage becomes non-deterministic across the buildfarm. Asserting the HOT-indexed stat delta (as 039 does) also fixes this by making the precondition explicit and checked. [moderate confidence]

---

📄 src/test/subscription/t/039_hot_indexed_apply.pl (L136-L138)

MODERATE confidence: poll_counters exits the loop on the fixed 10s deadline regardless of whether $upd_target was reached, then callers make hard assertions (is/cmp_ok) on the returned deltas. On a slow/loaded buildfarm animal a missed deadline turns cmp_ok($..., '>', 0) into a spurious failure. The user rules forbid time-based fallback as a synchronization primitive; prefer poll_query_until gating on the observed counter (as neighboring subscription TAP tests do) so the test either observes the condition or fails with a clear timeout rather than reading a stale value and asserting on it.

---

📄 src/test/subscription/t/039_hot_indexed_apply.pl (L324-L324)

LOW confidence / style: PostgreSQL comment discipline prefers impersonal descriptions of intent over person-named annotations. "Amit's corner:" reads as an informal review artifact rather than an explanation of what the scenario verifies; reword to describe the case (e.g. "Case: always-mode with an indexed attribute not covered by the replica identity").

[github-actions]

This entire file is a personal developer debugging artifact and must not be committed to a PostgreSQL patch destined for pgsql-hackers/commitfest. It is unrelated to the feature implementation and violates the 'minimal diff' discipline -- one of the top rejection reasons on -hackers. It is added alongside other stray developer-only files in this same change (.clangd, pg-aliases.sh, src/test/benchmarks/siu/scripts/build.sh), which reinforces that these all belong in a local/ignored setup, not in the tree. Remove .gdbinit from the patch entirely.

[github-actions]

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

[github-actions]

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

[github-actions]

Breakpoints set on hard-coded absolute source line numbers are extremely brittle: any edit to these files (or a rebase) immediately makes them point at the wrong statement, silently misleading anyone who sources the file. The accompanying comments (e.g. 'Line 4019: pure HOT') hardcode line placement that will drift. This applies to every file:NNN breakpoint in the file (heapam.c:3210/4019/4024/4033/4101/4147, heapam_indexscan.c:182/250/297, indexam.c:299, execIndexing.c:370, pruneheap.c:1802/1836/1863/1287/2936). Another reason to drop the file entirely.

[github-actions]

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

[github-actions]

pg-format only prints an error when pgindent is missing but does not return non-zero, so callers/scripts cannot detect the failure. Consider return 1 in the error branch (matching the return 1 guards used in the valgrind/asan build helpers).

Suggested change

	echo "Error: pgindent not found at $PG_SOURCE_DIR/src/tools/pgindent/pgindent"
	echo "Error: pgindent not found at $PG_SOURCE_DIR/src/tools/pgindent/pgindent" >&2
	return 1

[github-actions]

basename $CC is unquoted, so it breaks if $CC ever contains spaces (e.g. a compiler-launcher form like ccache gcc). Quote it: $(basename "$CC"). Minor, since shell.nix currently sets CC to a bare path, but cheap to harden.

Suggested change

	local current_compiler="$(basename $CC)"
	local current_compiler="$(basename "$CC")"

[github-actions]

Arriving directly at a collapse-survivor stub (at_chain_start == true) skips arming *hot_indexed_recheck and skips OR-ing the stub's bitmap. But a stub is created in place at a dead key tuple's own offset, and the btree entry planted for that key tuple still points directly at that offset after collapse. When a walk starts at such an entry, at_chain_start is true, the stub branch forwards to a later, different live tuple, yet the arriving entry's key reflects the now-dead segment -- not the live tuple. Treating this as "already reflects this segment's value" is wrong: the entry is stale relative to the live tuple the walk ends at, so recheck must be armed (and the stub's bitmap OR-ed) even at chain start. Unlike a live HOT-indexed tuple reached at chain start (whose entry was planted for that very tuple), a stub reached at chain start never has its entry pointing at the returned tuple. As written this can return a row through a stale entry without flagging it, defeating the staleness filter. Confidence: high.

[github-actions]

The clamp bounds the write into crossed_bitmap (sized for relnatts), but HotIndexedGetModifiedBitmap() still reads from the tuple at (t_data + t_len - HotIndexedBitmapBytes(bmnatts)). Clamping bmnatts up to relnatts does not bound this read against t_len: if a corrupt stub reports a large stashed natts (clamped to relnatts) while t_len is small, t_len - HotIndexedBitmapBytes(relnatts) underflows and the subsequent HotIndexedBitmapUnion reads before the tuple's storage. The Assert is compiled out in production builds, so on a corrupt page this is an out-of-bounds read under a buffer share-lock. Consider additionally verifying HotIndexedBitmapBytes(bmnatts) <= t_len (accounting for the header/data) before dereferencing, matching how other on-disk readers validate item length. Confidence: moderate.

[github-actions]

This in-AM override of use_sort/OldIndex desyncs the caller's already-emitted diagnostic. repack.c logs "repacking ... using index scan on "%s"" (repack.c ExecUpdate path, if (OldIndex != NULL && !use_sort)) before calling table_relation_copy_for_cluster with that same use_sort. When this block flips use_sort = true (btree) the run actually does seqscan+sort, and when it sets OldIndex = NULL (non-btree) it silently does a physical-order copy and loses cluster ordering -- yet the user was already told an index scan on the named index would be used. The log message now lies, and the non-btree case drops the requested ordering with no NOTICE (POLA violation). The SIU-capability decision should be made where the scan strategy is chosen (repack.c) so the logged message matches what runs, or the AM should signal the chosen strategy back to the caller for logging.

[github-actions]

🔍 OCR found 102 issue(s).

• 25 inline, 77 in summary (inline capped at 25)
• ⚠️ 1 warning(s) during review

---

📄 src/backend/access/heap/heapam_indexscan.c (L289-L290)

Continuation calls (non-MVCC snapshot, *heap_continue == true) re-arm the recheck flag for the entry's own tuple. On the first call, arriving directly at a heap-only HEAP_INDEXED_UPDATED tuple correctly does NOT set *hot_indexed_recheck (its producing hop is not a crossed hop). But on a continuation call first_call is false, so the flag/bitmap are not cleared, at_chain_start is false, and the loop re-walks starting at that same previously-returned tuple -- which now falls into this else-if branch, sets *hot_indexed_recheck = true and ORs its own bitmap into crossed_bitmap. The index-access layer may then compute xs_hot_indexed_stale = true for a version the entry legitimately supplies. Since heap_continue is only true under non-MVCC snapshots, this affects dirty-snapshot index scans that return multiple chain members. Please confirm the entry's own tuple is excluded from the crossed union across continuation calls (e.g. by remembering the entry offset, or by not treating the walk's starting offset as a crossed hop on continuation).

---

📄 src/backend/access/heap/vacuumlazy.c (L2869-L2869)

The "see below" cross-reference is dangling: there is no explanation of why a cleanup lock is not required anywhere below this call within lazy_vacuum_heap_page. The actual justification lives above, in the caller's comment at the ConditionalLockBufferForCleanup site ("this pass only turns LP_DEAD items into LP_UNUSED; it does NOT ... re-point its redirects"), and in pruneheap.c's relaxed WAL assertion. Point the reader at the real location or drop the misleading suffix.

💡 Suggested change

Before:

								  false,	/* no cleanup lock required: see below */

After:

								  false,	/* no cleanup lock required for LP_DEAD -> LP_UNUSED */

---

📄 src/backend/access/heap/vacuumlazy.c (L2823-L2823)

Removing Assert(nunused > 0) drops a still-valid invariant without a demonstrated need in this file. The second pass only iterates blocks recorded in pass 1 (each with >=1 dead offset), TidStoreGetBlockOffsets returns num_offsets > 0 for such blocks, and every one of those offsets passes the Assert(ItemIdIsDead ...) in the loop above, so nunused == num_offsets > 0 always holds here. The change is unnecessary churn against the minimal-diff discipline; note also the unchanged comment right below ("The page is guaranteed to have had dead line pointers") relies on this same invariant, so weakening it here reads inconsistently. If the intent is that nunused can legitimately be 0 now, the all-visible/WAL path would emit a no-op prune record and the downstream comment would be false; if it cannot, keep the assert.

---

📄 src/backend/access/index/indexam.c (L610-L612)

Comment/code mismatch. The comment says "Reset the HOT-indexed recheck flag", but the code resets scan->xs_hot_indexed_stale (the stale flag). The recheck flag is xs_heapfetch->xs_hot_indexed_recheck, which lives on the table-AM fetch state and is reset inside heap_hot_search_buffer, not here. Per project convention comments must describe what the code does now; reword to refer to the stale flag.

💡 Suggested change

Before:

	/*
	 * Reset the HOT-indexed recheck flag: it is set by the heap AM during
	 * index_fetch_heap and is per-fetched-tuple, not per-index-entry. For

After:

	/*
	 * Reset the HOT-indexed stale flag: it is set by index_fetch_heap (from
	 * the heap AM's recheck report) and is per-fetched-tuple, not
	 * per-index-entry. For

---

📄 src/backend/access/index/indexam.c (L713-L714)

Hot-path allocation churn. RelationGetIndexedAttrs() returns bms_copy(rd_indattr), i.e. a freshly palloc'd Bitmapset on every call, and it is bms_free'd again here. In a HOT-indexed-heavy scan this recheck fires per fetched tuple, so this palloc/pfree pair runs in the innermost index-fetch loop. The index's referenced-attrs set is invariant for the scan; compute it once (e.g. cache it on IndexScanDesc / the fetch state and free at endscan) instead of per tuple. (The bms_free itself is safe since it frees the copy, not the cached rd_indattr.)

---

📄 src/backend/access/nbtree/nbtinsert.c (L586-L588)

The lazy slot creation is embedded inside the else if short-circuit condition, which is subtle and error-prone: a reader must reason that table_slot_create() is only evaluated when chain_walk_slot == NULL, and that its result is both assigned and truthy (it never returns NULL; it errors instead). This mixes an allocation side effect into a boolean guard that also drives the duplicate-detection branch. Consider hoisting the slot creation to a plain statement immediately before the else if for readability, e.g.

if (chain_walk_slot == NULL)
    chain_walk_slot = table_slot_create(heapRel, NULL);
else if (table_index_fetch_tuple_check(heapRel, &htid, &SnapshotDirty, &all_dead, &hi_recheck, chain_walk_slot))

(restructured so the fetch is still the branch condition). This keeps the hot-path allocation intent obvious and avoids a future editor accidentally breaking the short-circuit invariant.

---

📄 src/backend/access/nbtree/nbtinsert.c (L944-L948)

_bt_heap_keys_equal_leaf is reached for every SnapshotDirty hit where hi_recheck is true, and is on the unique-check insert path. When a key column's collation is non-default and the ordering proc is expensive (e.g. ICU/varstr under a nondeterministic collation), this adds a FunctionCall2Coll per key column per candidate. This is acceptable because it only fires after table_index_fetch_tuple_check already returned a live tuple AND the table AM reported a HOT/SIU hop, so classic HOT and non-SIU indexes pay nothing. Worth confirming there is a benchmark backing the claim that the extra comparison does not regress high-duplicate unique-insert workloads on SIU-enabled tables; the review context requires a reproducible benchmark for performance-sensitive hot-path additions.

---

📄 src/backend/catalog/system_views.sql (L733-L733)

New columns are inserted in the middle of these existing stats views (n_tup_hot_indexed_upd between n_tup_hot_upd and n_tup_newpage_upd; the two index columns before stats_reset), which shifts the ordinal position of all following columns. Any client doing SELECT * by ordinal, or code relying on column order, sees a behavior change. Confirm this placement is deliberate and matches the corresponding doc/monitoring.sgml column tables; the existing precedent (e.g. n_tup_newpage_upd) was also inserted mid-view, so this is likely acceptable, but it is worth an explicit note in the commit message. Confidence: moderate.

---

📄 src/backend/catalog/indexing.c (L21-L21)

Unnecessary header churn (minimal-diff violation). None of these three new includes is used by the code in this file:

• access/tableam.h: was only needed for the now-removed TU_UpdateIndexes enum; grep shows it appears only on this include line, no table_*/TU_* symbol remains.
• nodes/bitmapset.h: no Bitmapset/bms_* symbol is used in this file.
• utils/relcache.h: no relcache-only symbol (e.g. RelationGetIndexList) is used; utils/rel.h (already included, unchanged) covers what's needed.

Adding unused headers on an otherwise minimal refactor is a common -hackers rejection reason. Drop all three; keeping the include set as it was after removing the TU_UpdateIndexes dependency is the correct minimal change. (high confidence)

---

📄 src/backend/catalog/indexing.c (L144-L147)

index_unchanged is a single-use local that just aliases !update_all_indexes and is consumed only on the next line. It adds no clarity over inlining and expands the diff. Prefer inlining to keep the change minimal.

💡 Suggested change

Before:

		index_unchanged = !update_all_indexes;

		if (index_unchanged && !indexInfo->ii_Summarizing)
			continue;

After:

		if (!update_all_indexes && !indexInfo->ii_Summarizing)
			continue;

---

📄 src/backend/catalog/toasting.c (L310-L311)

Removing ii_CheckedUnchanged here is required (the field no longer exists in IndexInfo), but removing ii_IndexUnchanged = false is unnecessary: that field still exists in IndexInfo (execnodes.h). It happens to stay false here only because this function uses makeNode(IndexInfo) (palloc0). Dropping just this one explicit init while the surrounding code still redundantly sets ii_Concurrent = false, ii_BrokenHotChain = false, etc., is an inconsistent, non-minimal change. Keep the ii_IndexUnchanged = false line to match local style and limit the diff to what the field removal actually requires.

💡 Suggested change

Before:

-	indexInfo->ii_CheckedUnchanged = false;
-	indexInfo->ii_IndexUnchanged = false;

After:

	indexInfo->ii_IndexUnchanged = false;

---

📄 src/backend/executor/execTuples.c (L69-L69)

This added #include "utils/datum.h" is unused: no symbol declared in that header (e.g. datumGetSize, datumCopy, datumTransfer, datumIsEqual, datum_image_eq, datum_image_hash) is referenced anywhere in execTuples.c. The Datum/DatumGet*/att_addlength_datum usages here come from postgres.h, access/tupmacs.h, and utils/expandeddatum.h, not from this header. It appears to be a leftover from development. Remove it to keep the diff minimal. (high confidence)

---

📄 src/backend/executor/execReplication.c (L971-L971)

This assertion is too strict and will fire in assert-enabled builds. ExecUpdateModifiedIdxAttrs() returns a bitmap that may already contain TableTupleUpdateAllIndexes on input when the target table has an index whose expression references a whole-row Var: RelationGetIndexAttrBitmap(INDEX_ATTR_BITMAP_INDEXED) records attribute 0 via pull_varattnos(), and heapam_modified_attrs() keeps attribute 0 (the attrnum == 0 case does continue, it is not deleted). Attribute 0's bitmap index equals TableTupleUpdateAllIndexes by construction.

This directly contradicts the documented contract in table_tuple_update() (tableam.h: "*modified_attrs may already contain TableTupleUpdateAllIndexes on input when an index expression references a whole-row Var") and the parallel ExecUpdateAct() path in nodeModifyTable.c, which deliberately does NOT assert this and comments that the sentinel's presence is harmless. Note the code even reads the sentinel back at line 981 (all_indexes = bms_is_member(TableTupleUpdateAllIndexes, ...)), so the assert contradicts the function's own downstream handling.

Logical replication apply into a table with a whole-row-Var index expression will crash an assert build here. Remove the assertion. (high confidence)

---

📄 src/backend/executor/execIndexing.c (L1172-L1175)

Performance: ExecSetIndexUnchanged() is invoked once per updated row on the hot UPDATE path (see the callers in nodeModifyTable.c, execReplication.c, repack.c). For each index it calls RelationGetIndexedAttrs(), which returns a fresh bms_copy() of the relcache-cached rd_indattr, then immediately bms_free()s it after a read-only bms_overlap(). That is a palloc/pfree pair per index per row purely to feed a non-mutating bms_overlap. On a bulk UPDATE of N rows with M indexes this is N*M allocation churn where the copy is never needed. Consider exposing a read-only accessor that returns the cached bitmap without copying (or reusing the existing INDEX_ATTR_BITMAP_INDEXED set already computed for modified_attrs) so the hot path avoids the copy+free entirely.

---

📄 src/backend/executor/nodeIndexscan.c (L162-L166)

Counting HOT-indexed stale drops with InstrCountFiltered2 conflates them with lossy-index recheck removals. nfiltered2 is surfaced in EXPLAIN ANALYZE as "Rows Removed by Index Recheck" (see explain.c show_instrumentation_count(..., 2, ...)), but these rows were dropped for HOT-indexed staleness, not by a qual recheck. This is misleading in plans and can perturb tests/monitoring that read that counter. Consider a distinct counter or at least document/justify the reuse. Same concern applies to the IndexNextWithReorder hunk below and the IOS path. (moderate confidence)

---

📄 src/backend/nodes/makefuncs.c (L847-L847)

Removing ii_CheckedUnchanged = false is correct since that field no longer exists in the tree. However, removing ii_IndexUnchanged = false is questionable: that field is still live (execnodes.h:224) and, together with the newly added ii_IndexNeedsUpdate (execnodes.h:230), now relies solely on makeNode/palloc0 for its initial value. Every other boolean in this function is initialized explicitly (ii_Unique, ii_NullsNotDistinct, ii_ReadyForInserts, ii_BrokenHotChain), so dropping the explicit init here is inconsistent with the surrounding convention and leaves these two skip-decision flags as the odd ones out. Since these gate the UPDATE index-skip path (execIndexing.c:367-383, 461-462), keep them explicit. Suggest replacing the removed line with explicit initialization of both current flags rather than deleting it. (moderate confidence)

💡 Suggested change

Before:

	n->ii_ReadyForInserts = isready;

After:

	n->ii_ReadyForInserts = isready;
	n->ii_IndexUnchanged = false;
	n->ii_IndexNeedsUpdate = false;

---

📄 src/backend/executor/nodeModifyTable.c (L2524-L2525)

Memory leak on the redo_act retry path. When table_tuple_update() returns TM_Updated, modified_attrs was already populated above (line 2649) but is never freed before ExecUpdate() does goto redo_act. Re-entering ExecUpdateAct() overwrites the pointer with NULL here, dropping the previously palloc'd Bitmapset without freeing it. Because RelationGetIndexAttrBitmap() allocates in es_query_cxt (not a per-tuple context that gets reset), the leaked bitmap survives for the whole statement -- and the loop can iterate multiple times under contention. This is exactly the accumulation the ExecUpdateEpilogue comment claims to prevent. Free the previous bitmap before resetting, e.g. bms_free(updateCxt->modified_attrs); then set NULL.

💡 Suggested change

Before:

	/* Reset any state left over from a previous call */
	updateCxt->modified_attrs = NULL;

After:

	/* Reset any state left over from a previous call */
	bms_free(updateCxt->modified_attrs);
	updateCxt->modified_attrs = NULL;

---

📄 src/backend/replication/logical/worker.c (L6087-L6088)

The comment contradicts the implementation. The leading line says "Return the cached HOT-indexed apply mode", but the body comment just below explicitly 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 on every call. Drop "cached" so the comment describes what the code actually does. (The header comment in logicalworker.h has the same stale "cached" wording.)

💡 Suggested change

Before:

 * Return the cached HOT-indexed apply mode of the current logical replication
 * worker's subscription.

After:

 * Return the HOT-indexed apply mode of the current logical replication
 * worker's subscription.

---

📄 src/backend/executor/nodeModifyTable.c (L2728-L2729)

The bms_free here is the ONLY place modified_attrs is freed, but ExecUpdateEpilogue is reached only on the TM_Ok path. On every non-TM_Ok result the bitmap was already populated in ExecUpdateAct (before table_tuple_update) and then leaks: in ExecUpdate() the TM_SelfModified (return NULL), TM_Deleted (return NULL), and TM_Updated give-up branches all return without calling the epilogue; in the MERGE path (lmerge_matched) the epilogue is called only if (result == TM_Ok). Since RelationGetIndexAttrBitmap() allocates in es_query_cxt, each such row leaks one Bitmapset for the statement's lifetime -- the exact leak this comment claims to prevent. Free modified_attrs on the failure paths too (or move the bms_free so it runs regardless of result).

---

📄 src/backend/nodes/tidbitmap.c (L406-L406)

unlikely() is questionable here. For a scan that uses a HOT-indexed index, SIU-flagged fresh entries are precisely the common case for updated rows, not a rare event — potentially a large fraction of the TIDs fed into this loop from that index. Hinting the compiler to lay out the flagged branch as cold (and biasing the branch predictor) pessimizes the exact workload this feature targets. Since the test is a single cheap bit-and either way, consider dropping unlikely() so neither path is penalized. Confidence: moderate.

💡 Suggested change

Before:

		if (unlikely(ItemPointerIsSIUMaybeStale(tids + i)))

After:

		if (ItemPointerIsSIUMaybeStale(tids + i))

---

📄 src/backend/storage/page/itemptr.c (L67-L68)

high confidence: This changes SQL-visible semantics of the tid type. ItemPointerCompare is not index-internal only -- it backs the user-facing tid comparison operators (tideq, tidne, tidlt, bttidcmp, tidlarger, ...) in src/backend/utils/adt/tid.c, and bttidcmp is the support proc for the tid_ops btree opclass. Stripping bit 14 here means any two tid values (blk, off) and (blk, off | 0x4000) now compare equal, and the flagged one sorts as if the bit were absent. At BLCKSZ >= 16KB real heap ctid offsets can exceed 16383 (the header comment cites a worst case of 0x6000 at 32KB), so this silently breaks =/</ORDER BY/DISTINCT/unique-and-btree indexes on tid columns for legitimate, non-HOT-indexed TIDs. The comment's premise -- "the bit is a hint to a handful of bitmap-scan call sites, not part of a TID's identity for ordering/equality" -- is false for this general-purpose comparator's role as the tid-type comparator. The masking belongs in the nbtree/bitmap TID paths that actually deal with index-stored heap TIDs, not in the shared ItemPointerCompare.

---

📄 src/include/access/amapi.h (L30-L32)

This is a whitespace-only change unrelated to the patch's purpose (the HOT-indexed work). It removes a blank line in amapi.h, a file otherwise untouched by this feature. Cosmetic churn on unrelated lines is a top rejection reason on -hackers: it pollutes the diff, causes needless merge conflicts, and forces reviewers to explain why the hunk exists. Drop this hunk so the patch stays minimal. (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 runs CREATE EXTENSION amcheck (and uses verify_heapam/bt_index_check), but amcheck is not made available to the pg_upgrade test harness. src/bin/pg_upgrade/Makefile line 6 sets EXTRA_INSTALL=contrib/test_decoding src/test/modules/dummy_seclabel src/test/modules/test_extensions (no contrib/amcheck), and src/bin/pg_upgrade/meson.build lists 'deps': [test_ext] for the pg_upgrade tests (no amcheck). Under make check/meson, amcheck will not be installed into the temp install, so this safe_psql call dies (safe_psql fails hard on error) and the whole test fails instead of skipping. The user rules require tests to be skippable without optional prerequisites. Compare src/bin/pg_amcheck/Makefile which uses EXTRA_INSTALL=contrib/amcheck contrib/pageinspect for exactly this reason. Add contrib/amcheck to pg_upgrade's EXTRA_INSTALL and amcheck to the meson deps (those build-file edits belong with this patch). High confidence.

---

📄 src/bin/pg_upgrade/t/009_hot_indexed.pl (L105-L105)

The comment claims a "forced index scan", but SET enable_seqscan = off; SET enable_bitmapscan = off; only add cost penalties (disable_cost); they do not guarantee an index scan is chosen. For this 20-row table the planner may still pick a seqscan, in which case the count is still correct but the assertion is no longer exercising the index-scan/chain-walk path the comment advertises. Either verify the plan with EXPLAIN, or soften the comment to avoid claiming the scan method is guaranteed. Moderate confidence.

---

📄 src/backend/utils/cache/relcache.c (L5771-L5773)

Dev-context leakage in a durable header comment: "never on the bench hot path" references benchmark development context, not stable documentation. Per PostgreSQL comment discipline, comments should explain durable rationale (why) rather than transient development observations. Suggest dropping the benchmark reference.

💡 Suggested change

Before:

 * the second pass.  The first-use path is rare and never on the bench hot
 * path, so the simplicity is preferred over open-coding the populate-only
 * variant.

After:

 * the second pass.  The first-use path is rare, so the simplicity is
 * preferred over open-coding a populate-only variant.

---

📄 src/include/access/htup_details.h (L292-L297)

The bit choice is correct (0x0800 does not overlap HEAP_NATTS_MASK 0x07FF, HEAP2_XACT_MASK 0xE000, or any other infomask2 flag; HeapTupleHeaderSetNatts masks with ~HEAP_NATTS_MASK so it is preserved). One style point for pgsql-hackers: the trailing half of the comment describes downstream index-scan recheck behavior ("index scans that reach it via a chain recheck the arriving leaf key against the live tuple"), which is subsystem behavior already documented in README.HOT-INDEXED, not the meaning of the flag itself. A bit definition comment should state what the bit records; consider trimming to the first sentence and leaving the recheck protocol to the README to keep the header comment proportionate and avoid comment drift. (low confidence, style)

💡 Suggested change

Before:

#define HEAP_INDEXED_UPDATED	0x0800	/* HOT tuple produced by an UPDATE
										 * that also changed an indexed
										 * attribute (HOT/SIU); index scans
										 * that reach it via a chain recheck
										 * the arriving leaf key against the
										 * live tuple. */

After:

#define HEAP_INDEXED_UPDATED	0x0800	/* HOT tuple produced by an UPDATE
										 * that also changed an indexed
										 * attribute (see
										 * README.HOT-INDEXED) */

---

📄 src/include/access/hot_indexed.h (L86-L86)

Minor consistency and robustness nit. The read path here uses a plain int literal 1 << (bit % 8) while the write path in HotIndexedSetAttrModified casts to (uint8). Since bit % 8 is always < 8 there is no shift UB, but the two accessors should be consistent (use 1U). More importantly, both accessors only Assert(attnum >= 1) and never bound attnum against the tuple's natts; an out-of-range attnum would silently index past the bitmap. Callers currently keep attnum in range (e.g. heap_form_hot_indexed_tuple derives it from indexed relation attributes), so this is not a live bug, but an upper-bound assert would make the footgun harder to trip.

💡 Suggested change

Before:

	return (bitmap[bit / 8] & (1 << (bit % 8))) != 0;

After:

	return (bitmap[bit / 8] & (1U << (bit % 8))) != 0;

---

📄 src/include/access/hot_indexed.h (L99-L99)

pgindent style nit: this block comment starts its body on the same line as the /* opener, unlike the other comments in this file. pgindent will reformat multi-line comments to put the opening /* on its own line. Move the text to the next line to keep the file pgindent-clean.

💡 Suggested change

Before:

/* OR the first nbytes of src into dst.  dst must be at least nbytes long; it

After:

/*
 * OR the first nbytes of src into dst.  dst must be at least nbytes long; it

---

📄 src/include/catalog/pg_subscription.h (L236-L236)

American spelling is the tree-wide convention; "behaviour" should be "behavior". This comment is user-facing (mirrored in catalogs.sgml). [low confidence, style]

s/behaviour/behavior/

💡 Suggested change

Before:

 *		  behaviour before this option was introduced.

After:

 *		  behavior before this option was introduced.

---

📄 src/include/catalog/pg_subscription.h (L180-L180)

This trailing comment is indented with a single space after the field, while adjacent fields (e.g. conflictlogrelid; above) align the comment with a tab. This will not survive pgindent cleanly. Align with a tab. [moderate confidence, style]

💡 Suggested change

Before:

	char		hotindexedonapply; /* Per-subscription gating of the

After:

	char		hotindexedonapply;	/* Per-subscription gating of the

---

📄 src/include/access/tableam.h (L1348-L1353)

The doc comment doesn't state the NULL-handling contract for the two new out-parameters. hot_indexed_recheck_out may be NULL (the nbtinsert.c caller at line 707 passes NULL, and tableam.c:264 checks for it), and the comment is silent about what *keep_slot / *hot_indexed_recheck_out hold on a negative (false) return. Since this is an exported table-AM API that out-of-tree AMs and extensions must implement/call correctly, document that hot_indexed_recheck_out is optional (may be NULL) and that on a false return the recheck flag is not meaningful and keep_slot is left empty. (moderate confidence)

---

📄 src/include/replication/logicalworker.h (L28-L28)

The word "cached" is inaccurate and contradicts the implementation. GetHotIndexedApplyMode() in worker.c explicitly derives the value directly from MySubscription and documents that it deliberately does NOT cache ("Derive directly from MySubscription rather than caching, so there is no second copy to keep in sync"). Calling it a "cached" mode here is a comment/code mismatch. Suggest dropping "cached". (moderate confidence)

💡 Suggested change

Before:

 * Accessor for the cached hot_indexed_on_apply mode of the current apply

After:

 * Accessor for the hot_indexed_on_apply mode of the current apply

---

📄 src/include/utils/relcache.h (L73-L75)

This header comment misstates where the cache lives. rd_indexcxt is the memory context; the cached copy is stored in the rd_indattr field (which happens to be allocated inside rd_indexcxt). As written it reads as if rd_indexcxt itself is the cached bitmap. Suggest: "...caches its own copy in rd_indattr (allocated in rd_indexcxt)...".

Also, "subsequent calls only pay for the final bms_copy" is misleading: per the implementation (relcache.c:5316-5318) every call, including the cached fast path, pays a bms_copy of rd_indattr, not just a "final" one. Confidence: high.

💡 Suggested change

Before:

 * caches its own copy in rd_indexcxt so subsequent calls only pay for the
 * final bms_copy.
 */

After:

 * caches its own copy in rd_indattr (allocated in rd_indexcxt) so subsequent
 * calls only pay for a bms_copy of the cached bitmap.
 */

---

📄 src/include/utils/rel.h (L227-L229)

The comment tells consumers they "must bms_copy before relying on the pointer beyond any potential AcceptInvalidationMessages() call", but the only accessor, RelationGetIndexedAttrs(), never hands out this raw pointer -- it always returns bms_copy(rd_indattr) (relcache.c:5317-5318), and its own docstring explicitly states "We never hand out a borrowed pointer to the cached copy". No code outside relcache.c reads rd_indattr. The bms_copy instruction here is therefore contradictory and misleading; it implies a supported borrow-the-pointer path that does not exist. Drop that sentence and instead state that the field is private to relcache.c and only exposed via RelationGetIndexedAttrs(), which returns a caller-owned copy.

💡 Suggested change

Before:

	 * AcceptInvalidationMessages() call.
	 */
	Bitmapset  *rd_indattr;

After:

	 * RelationGetIndexedAttrs(), which returns a caller-owned copy.
	 */
	Bitmapset  *rd_indattr;

---

📄 src/include/utils/rel.h (L229-L229)

rd_indattr (per-index heap attrs, this field) and rd_indexedattr (heap-relation aggregate of all index cols, added above at line 165) differ by only two characters yet mean different things at different relation levels. This near-collision is a maintainability footgun and easy to mix up. Consider a more distinct name such as rd_indexattr or rd_idxheapattr for the per-index field to avoid confusion with rd_indexedattr.

---

📄 src/test/benchmarks/siu/scripts/bit14_ab.sh (L1-L2)

This whole file (and the sibling src/test/benchmarks/siu/ tree, plus the .clangd/.gdbinit/pg-aliases.sh added in this changeset) is an ad-hoc developer benchmarking harness. src/test/benchmarks/ does not currently exist anywhere in the tree and there is no build-system integration (no meson.build, not referenced by any test schedule), so this is unmaintained scaffolding, not a supported artifact. A patch destined for pgsql-hackers/commitfest should carry only the change needed for the stated bit14 fix; a personal A/B harness bloats the tree and reliably draws a 'does more than one thing / unrelated scaffolding' rejection. Confidence: high. Recommendation: drop this from the patch and post the benchmark method + numbers in the -hackers thread instead.

---

📄 src/test/benchmarks/siu/scripts/bit14_ab.sh (L98-L99)

Portability: grep -oP uses GNU/PCRE options that are absent on BSD/macOS/Solaris grep, so this parsing silently yields empty tps/lat there and every iteration gets counted as FAILED. The sibling run.sh extracts the same fields portably with awk ('/tps = /{print $3; exit}' and '/latency average = /{print $4; exit}'); use the same approach here for consistency and portability. Confidence: high.

💡 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 (L62-L64)

sleep 2 is used as startup synchronization after pg_ctl start; on a slow/loaded host this races and seed()/pgbench then fail to connect. Use pg_ctl -w (wait for ready) or poll pg_isready instead of a fixed sleep. Confidence: moderate.

---

📄 src/test/benchmarks/siu/scripts/bit14_ab.sh (L12-L12)

The header promises 'compute the median downstream from the collected rows', but no downstream consumer of this CSV exists in the changeset (search across src/test/benchmarks/ finds only this file's own references to bit14_/latency_avg_ms). The median-computation contract is left dangling; either add the consumer or drop the claim. Confidence: high.

---

📄 src/test/benchmarks/siu/scripts/bit14_ab.sh (L21-L21)

Hardcoded default PORT (57481) and fixed datadir path ($BENCH/data_bit14$v) risk collisions with a concurrently running instance (e.g. another user, or run.sh which defaults PORT 57480 under the same $BENCH). start_pg also unconditionally rm -rf's the datadir, so a stray path/collision could destroy an unrelated data directory. Confidence: low.

---

📄 src/test/benchmarks/siu/scripts/bitmap_and_mixed.sql (L13-L15)

The comment references reset_state's layout, but this script is only wired into bit14_ab.sh (via pgbench -f bitmap_and_mixed.sql), whose seeder is seed() -- not reset_state. reset_state lives in run.sh, which never runs this workload. The cited function does not seed this table, so the explanation is inaccurate. Reference bit14_ab.sh's seed() instead.

Also, the parenthetical "(a and b both increasing with row order)" names the wrong columns: the BitmapAnd predicate below is on b and d, and column a (the PK) is not part of it. It should read b and d.

💡 Suggested change

Before:

-- contributions for unrelated rows sharing that page, which is exactly what
-- reset_state's freshly-clustered layout (a and b both increasing with row
-- order) puts multiple rows per heap page under.

After:

-- contributions for unrelated rows sharing that page, which is exactly what
-- bit14_ab.sh's freshly-clustered seed() layout (b and d both increasing with
-- row order) puts multiple rows per heap page under.

---

📄 src/include/storage/itemptr.h (L197-L197)

This changes a pervasively-used, hot-path accessor. ItemPointerGetOffsetNumber is called on essentially every heap tuple fetch in the backend, and it now unconditionally executes an extra branch (raw >= MovedPartitionsOffsetNumber) plus a mask on every call, even for the overwhelmingly common case where no SIU bit is or ever will be present. For a feature whose flagged TIDs only exist inside index tuples of HOT-indexed relations, taxing every single ItemPointerGetOffsetNumber caller in the tree is a broad regression that will draw objections on -hackers. Consider confining the strip to the specific index-scan/bitmap consumers that can actually observe a flagged TID, and leaving the universal accessor unchanged, rather than making the base accessor pay for a niche invariant. (moderate confidence on perf impact; the correctness of the strip itself is fine.)

---

📄 src/include/storage/itemptr.h (L209-L209)

This predicate silently trusts that pointer is a genuine heap-row TID and never an nbtree pivot/posting alt-TID (whose offset field is overloaded via INDEX_ALT_TID_MASK/BT_STATUS_OFFSET_MASK, where bit 14 has an unrelated meaning). The contract is enforced only by prose in the header. The two in-tree callers currently guard correctly (tidbitmap only sees plain heap TIDs; verify_nbtree gates on !BTreeTupleIsPosting), but this is an easy-to-misuse API: any future caller that passes an alt-TID will get a false positive and spuriously force lossy bitmap contributions. Consider an Assert that documents/enforces the caller must not pass an alt-TID field, or restructure so the flag can only be tested via a heap-TID-typed path.

---

📄 src/test/benchmarks/siu/scripts/hot_indexed_mixed.sql (L3-L3)

:scale is undefined for this workload. In run.sh, hot_indexed_mixed runs through the default run_one case, which invokes pgbench -f "$script" without any -D scale=.... pgbench only auto-populates the scale variable for its built-in scripts; for a custom -f script it is not defined, and this benchmark also disables the pgbench_* schema query path (-n). As a result pgbench aborts with an "undefined variable scale" error on the first \set, so this workload never runs. Either add -D "scale=$SCALE" to the default case in run.sh (the wide_* case already does this) or drive the row count via :rows the way read_indexscan.sql does.

---

📄 src/test/benchmarks/siu/scripts/build.sh (L12-L12)

This whole benchmark harness (build.sh and its siblings under src/test/benchmarks/siu/, plus the co-added .clangd, .gdbinit, pg-aliases.sh) is author-local developer scaffolding, not something the PostgreSQL build/test system runs. It is not wired into any Makefile or meson.build (grep for 'benchmarks/siu' finds no build hookup), and it hardcodes personal paths ($HOME/ws/postgres/tepid, /scratch/siu-bench, /scratch/pg) and a private branch name ('tepid'). Committing personal scaffolding like this is a standard rejection reason on -hackers. If a benchmark harness is intended to ship, it must be de-personalized (no absolute paths, no branch names, no 'siubench' extra_version), documented, and justified; otherwise it should be dropped from the patch. (high confidence)

---

📄 src/test/benchmarks/siu/scripts/build.sh (L40-L40)

Footgun: this mutates the developer's working repo. git checkout --quiet --detach "$rev" switches HEAD inside $REPO, and the original ref is only restored via trap ... EXIT. On SIGINT/SIGTERM (Ctrl-C, killed build), or if the meson build aborts mid-run, the restore still runs on EXIT in bash, but if the final restore checkout itself fails (e.g. because meson left generated files that conflict), the developer is silently left on a detached HEAD at an arbitrary revision. Building in-place by checking out revisions in the user's live source tree is inherently unsafe; prefer git worktree add (isolated tree per variant) so the working repo is never touched. (moderate confidence)

---

📄 src/test/benchmarks/siu/scripts/build.sh (L52-L52)

The trap restores the original ref but does not restore any stashed/dirty state, and if the checkout inside build_variant aborts the script (set -e) after modifying the tree, git checkout --quiet "$ORIG" may fail against generated build artifacts, leaving the repo detached. The trap also does not report or propagate a restore failure. Given the pre-flight guard already refuses to run on a dirty tree, using an isolated git worktree per variant would eliminate the need for this fragile save/restore-on-EXIT dance entirely. (moderate confidence)

---

📄 src/test/benchmarks/siu/scripts/build.sh (L15-L15)

Portability/robustness: the default REPO candidate list embeds environment-specific absolute paths (/scratch/pg) and a personal path ($HOME/ws/postgres/tepid). These are meaningful only on the author's machine and make the script non-reusable. (moderate confidence)

---

📄 src/test/benchmarks/siu/scripts/build.sh (L6-L6)

Doc/code drift: build.sh defaults BENCH to /scratch/siu-bench, but the sibling README.md documents the default as /scratch/tepid-bench (twice). Whichever is correct, the two disagree and will confuse anyone following the README. (low confidence)

---

📄 src/test/benchmarks/siu/scripts/build.sh (L23-L23)

Baseline correctness for the A/B comparison: MASTER_REV defaults to the merge-base of the tepid branch with origin/master, i.e. the point where tepid diverged, not current master tip. Benchmarking tepid against a stale divergence point rather than master HEAD can attribute unrelated upstream changes (or their absence) to the tepid feature and produce misleading A/B numbers. If the intent is 'feature on vs feature off', the baseline should be master tip (or an explicitly stated, reproducible revision). (moderate confidence)

---

📄 src/test/benchmarks/siu/scripts/bloat.sh (L24-L26)

This whole developer benchmark tree (bloat.sh, build.sh, run.sh, soak.sh, plus the *.sql fixtures) is ad-hoc scaffolding bundled with a core feature patch and should not be part of the committable diff. It is not wired into any Makefile/meson build (confirmed: no reference to benchmarks/siu anywhere), so from the tree's perspective it is dead code that only inflates the diff and will draw a 'patch does more than one thing' rejection on -hackers. Duplicated cluster-bootstrap logic here (initdb/pg_ctl/postgresql.conf) reimplements what PostgreSQL::Test::Cluster already provides for TAP tests. If a reproducible benchmark must ship, it belongs in a separate patch/thread, not folded into the feature. [low confidence on whether the maintainers want it at all; high confidence it is unintegrated]

---

📄 src/test/benchmarks/siu/scripts/bloat.sh (L39-L39)

Hardcoded, environment-specific defaults hurt portability and are a POLA footgun: /scratch/siu-bench is a Linux-only absolute path, -h /tmp assumes a Unix-domain socket dir (fails on Windows/MSVC and clusters with a non-/tmp unix_socket_directories), and PORT=57481 can collide on a shared host. The script also silently depends on a pre-built 'tepid' variant produced by build.sh and on the pgstattuple contrib extension. None of this is discoverable or checked before use.

---

📄 src/test/benchmarks/siu/scripts/bloat.sh (L58-L59)

Table names and numeric knobs are interpolated unescaped into SQL via shell string expansion ($tbl, ${tbl}_a, $ROWS, $UPDATES). With the fixed literals (t_vac/t_novac) this works, but it is a SQL-injection-shaped pattern: overriding a name-bearing var, or a non-numeric ROWS/CYCLES/UPDATES override, produces malformed SQL and a hard abort under set -euo pipefail. There is no validation that ROWS/CYCLES/UPDATES are integers before they flow into generate_series/loop bounds.

---

📄 src/test/benchmarks/siu/scripts/hot_indexed_update.sql (L4-L4)

:scale is not populated as intended here. run.sh invokes this script via the default pgbench -f case, which passes neither -s nor -D scale=. For a custom -f script pgbench does not call GetTableInfo() (it is gated on internal_script_used), so :scale falls back to the global default of 1 rather than $SCALE. As a result random(1, :scale * 100000) resolves to random(1, 100000), while seed_siu_table populates SCALE * 100000 rows (2,000,000 at the default SCALE=20). The workload then only ever touches the first 1/20th of the table, concentrating updates on a tiny hot set and skewing the HOT-indexed measurements this benchmark is meant to produce. Pass -D scale=$SCALE for this workload in run.sh (as is already done for wide_update.sql), or hardcode the row count via -D rows= like read_indexscan.sql does. (high confidence)

---

📄 src/test/benchmarks/siu/scripts/soak.sh (L85-L87)

pgbench workload hot_indexed_update.sql references :scale (\set aid random(1, :scale * 100000)), but this pgbench invocation never defines it: setup() here only creates siu_table and never runs pgbench -i, and no -D scale=... is passed. pgbench auto-defines scale only when the pgbench-initialized branches table is present, which it is not here. The soak run will abort immediately with an "undefined variable scale" error for every client. In the sibling run.sh, setup_schemas() runs pgbench -i -s "$SCALE", which is why the same workload works there. Pass -D "scale=$SCALE" to make this self-contained. (high confidence)

💡 Suggested change

Before:

  pgbench_as "$v" -f "$BENCH/scripts/hot_indexed_update.sql" \
    -c "$CLIENTS" -j "$THREADS" -T "$DURATION" \
    -P "$SAMPLE" -n postgres >"$LOGDIR/pgbench_$v.log" 2>&1 &

After:

  pgbench_as "$v" -f "$BENCH/scripts/hot_indexed_update.sql" \
    -c "$CLIENTS" -j "$THREADS" -T "$DURATION" \
    -D "scale=$SCALE" \
    -P "$SAMPLE" -n postgres >"$LOGDIR/pgbench_$v.log" 2>&1 &

---

📄 src/test/benchmarks/siu/scripts/soak.sh (L30-L31)

Data-loss footgun: this unconditionally deletes the entire contents of $datadir and rmdir's it. Under set -euo pipefail, if $BENCH or $v were empty/misconfigured the target could be unintended, and because stop_pg is best-effort (|| true) a still-running server's data dir could be wiped. This exact line is copy-pasted from run.sh:65; the pattern should be factored into a shared helper and guarded (e.g. verify the dir looks like a PG datadir via a PG_VERSION check, and refuse to run if $v is empty). (moderate confidence)

---

📄 src/test/benchmarks/siu/scripts/soak.sh (L47-L49)

sleep 2 is used as a server-readiness signal instead of pg_ctl ... -w start (or pg_isready). On slow/loaded storage the subsequent setup() psql can connect before the server accepts connections and fail. Prefer pg_ctl -w start so readiness is deterministic; this same anti-pattern is duplicated from run.sh. (moderate confidence)

💡 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 (L99-L99)

Hardcoded block size. heap_pages divides pg_table_size by the literal 8192, which is wrong on builds with a non-default BLCKSZ. Additionally pg_table_size() includes the TOAST relation and FSM/VM forks, so labeling this metric heap_pages is misleading. Use current_setting('block_size') and/or pg_relation_size(..., 'main') if only the heap main fork is intended. (moderate confidence)

---

📄 src/test/benchmarks/siu/scripts/soak.sh (L98-L98)

The captured LSN (wal_now) is interpolated into SQL without validation. If a psql_as call returns empty (e.g. transient connection failure), the query becomes pg_wal_lsn_diff(''::pg_lsn, ...) which errors, and under set -e aborts the whole soak mid-run, discarding the CSV progress. Consider validating the captured value or tolerating a single failed sample rather than aborting. (low confidence)

---

📄 src/test/benchmarks/siu/scripts/run.sh (L280-L283)

Functional bug (high confidence): the hot_indexed_update and hot_indexed_mixed workloads fall through to this default *) case, which does not pass -D scale=$SCALE. However hot_indexed_update.sql uses \set aid random(1, :scale * 100000) and hot_indexed_mixed.sql uses \set aid random(1, :scale * 100000) / :scale * 100000. pgbench aborts with an undefined-variable error, so these two core workloads never run and their CSV rows report tps=NA. Pass -D scale=$SCALE here (or in the case labels for those workloads).

💡 Suggested change

Before:

    *)
      pgbench_as "$v" -f "$script" -c "$CLIENTS" -j "$THREADS" -T "$DURATION" \
        -n postgres >"$out" 2>&1
      ;;

After:

    *)
      pgbench_as "$v" -f "$script" -c "$CLIENTS" -j "$THREADS" -T "$DURATION" \
        -D "scale=$SCALE" -n postgres >"$out" 2>&1
      ;;

---

📄 src/test/benchmarks/siu/scripts/run.sh (L22-L22)

Portability/reproducibility (high confidence): hardcoded -h /tmp for the socket directory, a fixed default PORT=57480, and BENCH defaulting to the absolute /scratch/siu-bench make this harness non-portable and collision-prone on shared/CI hosts. PostgreSQL's benchmark/test conventions forbid hardcoded ports/paths precisely because they break reproducibility outside the author's machine. Derive the socket dir from $BENCH (or a mktemp dir), honor PGPORT, and avoid an absolute default root.

---

📄 src/test/benchmarks/siu/scripts/run.sh (L82-L84)

Synchronization anti-pattern (moderate confidence): using a fixed sleep 2 to wait for the server to accept connections is unreliable on slow/loaded hosts and is exactly the pattern PostgreSQL test guidelines forbid. pg_ctl start already supports -w (wait for readiness). Drop the sleep 2 and add -w to pg_ctl start, or poll with pg_isready.

💡 Suggested change

Before:

    -o "-p $PORT" -l "$LOGDIR/pg_$v.log" start >/dev/null
  sleep 2
}

After:

    -o "-p $PORT" -l "$LOGDIR/pg_$v.log" -w start >/dev/null
}

---

📄 src/test/benchmarks/siu/scripts/run.sh (L182-L184)

Robustness (moderate confidence): swallowing errors with 2>/dev/null and falling back to 0 makes a genuine failure (typo, permission error, connection drop) indistinguishable from "column absent on master". For a benchmark whose entire purpose is measuring HOT-indexed updates, this can silently emit misleading zeros. Consider probing column existence once against pg_stat_user_tables/information_schema.columns and only defaulting to 0 when the column truly does not exist, letting real query errors surface.

---

📄 src/test/benchmarks/siu/scripts/run.sh (L271-L271)

Correctness (moderate confidence): sed "s/:wide_set_clause/$extra_set/" uses / as the sed delimiter while $extra_set is built from build_wide_set_clause. If a future SET clause ever contains a / (or &, \), the substitution silently corrupts the SQL. Also, SCALE/WIDE_COLS/WIDE_STEPS are interpolated into SQL heredocs and arithmetic without any numeric validation; a non-numeric value produces broken SQL or wrong row counts rather than a clear error. Validate these are integers up front and use a sed delimiter unlikely to collide (e.g. s#:wide_set_clause#...#).

---

📄 src/test/regress/pg_regress.c (L1246-L1246)

This execl -> execlp change is unrelated to the HOT-indexed feature this patch set implements, and should not be in this patch (out-of-scope hunk; committers reject unrelated changes).

Beyond scope, the change is at best a no-op and at worst a footgun:

• SHELLPROG is always defined as an absolute path (/bin/sh in meson.build; $(SHELL) in GNUmakefile/ecpg, normally absolute). When the program name contains a /, execlp behaves exactly like execl (no PATH search), so nothing is gained in the normal case.
• The only added behavior is that if shellprog were ever a bare name, execlp would search $PATH for the shell. PATH-based lookup of the shell to exec is a security hazard (a caller-controlled PATH could point at a malicious sh) and is inconsistent with the established convention in the tree (e.g. pg_ctl.c uses execl("/bin/sh", ...)).

Recommend reverting this line unless there is a documented, tested reason to allow a non-absolute shell path. (high confidence)

💡 Suggested change

Before:

		execlp(shellprog, shellprog, "-c", cmdline2, (char *) NULL);

After:

		execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);

---

📄 src/test/regress/sql/tsearch.sql (L763-L764)

This comment is factually incorrect and describes backend internals in a regression SQL file. tsvector_update_trigger() calls heap_modify_tuple_by_cols() (see tsvector_op.c), not heap_modify_tuple(). Naming a specific backend function in a test's comment is fragile (it drifts from the implementation, as already happened here) and belongs in the design/commit message rather than a .sql test. If this test isn't functionally changed by the HOT-indexed-updates patch, this reworded comment is unrelated churn on an untouched line and should be dropped to keep the diff minimal. If it is needed, restore an accurate, high-level note.

💡 Suggested change

Before:

-- tsvector_update_trigger() uses heap_modify_tuple() to set column 'a'
-- without going through the executor's SET-clause tracking.

After:

--trigger

---

📄 src/test/recovery/t/055_hot_indexed_recovery.pl (L90-L91)

The prune step is not verified to have collapsed the chain. post_prune asserts n_hot_indexed > 0, which is identical to the pre_prune assertion above; the live HOT-indexed member survives any prune outcome, so this check passes whether or not dead members were collapsed to LP_REDIRECT. Opportunistic pruning (heap_page_prune_opt) is heuristic and timing-dependent — the sibling pageinspect test (contrib/pageinspect/sql/hot_indexed_updates.sql, section 12) explicitly notes "this holds regardless of how much opportunistic pruning has happened" and defers deterministic collapse validation to the hot_indexed_adversarial isolation spec for exactly this reason. As written, this test's stated purpose ("crash-recover the collapsed chain from WAL") is not actually exercised: if the prune does not fire, there is no collapse to recover and the test still passes green. Assert the collapse deterministically (e.g. n_chains > 0 via pageinspect, or an explicit forced-prune primitive) so the crash-recovery path is genuinely covered and the test would fail if the feature regressed. Confidence: high.

---

📄 src/test/recovery/t/055_hot_indexed_recovery.pl (L122-L123)

The header comment claims "skip_option = 'all-frozen' is the default" but the call passes skip := 'none'. The stale/contradictory note is misleading; verify_heapam's parameter is skip with default 'none' (not 'all-frozen'). Drop or correct the parenthetical so the comment matches the actual call. Confidence: moderate.

---

📄 src/test/recovery/t/055_hot_indexed_recovery.pl (L144-L145)

This step-3 comment narrates a precise two-pass VACUUM reclamation sequence ("the first removes ... reduces its LP; the second reclaims ... re-points the redirect"). This exact-count coupling is fragile: reclamation of collapse stubs is snapshot-horizon and page-state dependent, and if the implementation reclaims in one pass (or needs more), the is($final, '0', ...) assertion breaks or the comment becomes inaccurate. Prefer asserting the end state without over-specifying the internal pass mechanics, or poll until n_hot_indexed reaches 0 rather than hardcoding "two passes". Confidence: moderate.

---

📄 src/test/regress/sql/updatable_views.sql (L128-L128)

Adding ORDER BY a here is a correct determinism fix (the preceding UPDATE rw_view16 SET aa=-3 WHERE a=3 reorders the heap), but the fix is inconsistent within the same file. The structurally identical rw_view15 block just above (line 121, SELECT * FROM base_tbl;) is also preceded by a key-changing update (UPDATE rw_view15 SET a=4 WHERE a=3) and line 82 similarly follows an update, yet both were left without ORDER BY. If the row order became nondeterministic under this patch (e.g. HOT-indexed updates changing tuple placement), lines 82 and 121 have the same exposure and would flap under the parallel schedule. Either they don't actually flip order today (in which case this one-line change is unrelated churn and should be dropped to keep the diff minimal), or they do and this fix is incomplete. Confirm which and make it consistent. Confidence: moderate.

---

📄 src/tools/pgindent/pgindent (L1-L1)

Unrelated change: this shebang edit does not belong to this patch series and breaks a tree-wide convention. Every other Perl script in the PostgreSQL tree uses #!/usr/bin/perl (including src/tools/git_changelog, src/tools/copyright.pl, src/backend/catalog/genbki.pl, etc.). Switching only pgindent to #!/usr/bin/env perl is inconsistent and looks like a leaked local-environment convenience edit. Revert it to keep the diff minimal and the codebase uniform.

💡 Suggested change

Before:

#!/usr/bin/env perl

After:

#!/usr/bin/perl

---

📄 src/test/subscription/t/039_hot_indexed_apply.pl (L127-L130)

The polling loop silently returns stale counter values on timeout, which is a footgun under the parallel/buildfarm schedule. Because the apply worker flushes pgstat asynchronously, if the flush hasn't happened within the 10 s deadline this returns the last-read (possibly zero/partial) row and the downstream cmp_ok('>',0) and is(...,0) assertions compare against incomplete counters -- producing false passes or intermittent failures. PostgreSQL TAP discipline forbids sleep/timeout as synchronization: on timeout the test must die loudly, not proceed with stale data. The established idiom (see 026_stats.pl) is poll_query_until($db, qq[SELECT ... >= target ...]) or die "..."; then read the exact values. Recommend replacing this loop with poll_query_until on a boolean condition (with or die) and only then reading the counters.

---

📄 src/test/subscription/t/039_hot_indexed_apply.pl (L414-L416)

The test never stops the publisher/subscriber nodes before done_testing(). The sibling test 040_hot_indexed_replica_identity.pl ends with $subscriber->stop; $publisher->stop;. For consistency and clean teardown, add explicit node stops before done_testing().

💡 Suggested change

Before:

$subscriber->safe_psql('postgres', 'DROP SUBSCRIPTION sub_ri');

done_testing();

After:

$subscriber->safe_psql('postgres', 'DROP SUBSCRIPTION sub_ri');

$subscriber->stop;
$publisher->stop;

done_testing();

---

📄 src/test/subscription/t/039_hot_indexed_apply.pl (L324-L324)

Personal-name / private-review reference ("Amit's corner") is informal commentary that will be read on pgsql-hackers. Comments should explain the rationale in neutral terms rather than cite a private discussion. Reword to describe the scenario (always-mode with an indexed attribute not covered by the replica identity) without the name. The sibling file 040 does not have this issue.

💡 Suggested change

Before:

# Amit's corner: under hot_indexed_on_apply = 'always' the apply worker may

After:

# Under hot_indexed_on_apply = 'always' the apply worker may

---

📄 src/test/subscription/t/039_hot_indexed_apply.pl (L2-L2)

Unlike the sibling test 040_hot_indexed_replica_identity.pl, this file begins with a blank line before the copyright comment. PostgreSQL TAP file headers conventionally start directly with the # Copyright ... line. Remove the leading blank line for consistency.

💡 Suggested change

Before:

# Copyright (c) 2026, PostgreSQL Global Development Group

After:

# Copyright (c) 2026, PostgreSQL Global Development Group

---

📄 src/test/subscription/t/040_hot_indexed_replica_identity.pl (L97-L98)

high confidence: This RI-USING-INDEX assertion does not actually exercise the stale-leaf-skip it claims to test (the if (scan->xs_hot_indexed_stale) continue; added to RelationFindReplTupleByIndex). The apply worker searches by the old-tuple's RI value (the pre-image), which always equals the current live key on the subscriber before each apply. A stale leaf only produces a WRONG match when a search key matches a stale leaf that chain-resolves to a live tuple holding a DIFFERENT current key. In this sequence every lookup uses the current key (k=1, k=1, k=3, k=3, delete k=2), so the stale key=1 leaf left after k=1->3 is never on the search path for a k=3 lookup, and vice versa. Consequently, if the xs_hot_indexed_stale skip were reverted, this test would STILL PASS -- it is worthless for catching a regression in that code. To exercise the failure path, arrange for a lookup whose key matches a stale leaf while the live row carries a different key (e.g. two live rows that swap identity values, or query the pre-cycle value while the row currently holds a different one).

---

📄 src/test/subscription/t/040_hot_indexed_replica_identity.pl (L102-L105)

moderate confidence: The assertions only compare final table state. verify_heapam checks the heap but amcheck's nbtree verification was not extended for HOT-indexed stale leaves, so the b-tree indexes (tab_idx_k/v/w) that are the actual site of stale leaves are not verified. Combined with the search-key issue above, none of the three assertions would fail if the apply-side stale-leaf handling regressed. Consider adding a lookup that would return the wrong row on regression, or a query that forces an index-only path over the affected key.

[github-actions]

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

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

[github-actions]

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

[github-actions]

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

[github-actions]

Non-ASCII em-dash characters (U+2014) appear in the comment headers throughout this file (lines 1, 15, 38, 52, 81, 98, 106, 117, 140, 148). PostgreSQL requires ASCII-only content in source and diffs; em-dashes, smart quotes and ellipses are disallowed. If any of this were retained, replace the em-dashes with a plain ASCII hyphen/dash. Confidence: high.

[github-actions]

Continuation-line indentation here is inconsistent with the seven identical verify_heapam(...) calls already in this file (lines 96-139), which align the startblock/endblock arguments with pure tab indentation. These new lines use tabs followed by spaces to align under the opening paren of verify_heapam(, mixing tabs and spaces. For a minimal, consistent diff, match the existing style in this file.

Confidence: high (verified against the other calls in the same file).

Suggested change

	SELECT blkno, offnum, attnum, msg
	FROM verify_heapam('hot_indexed_check',
	startblock := NULL,
	endblock := NULL);
	SELECT blkno, offnum, attnum, msg
	FROM verify_heapam('hot_indexed_check',
	startblock := NULL,
	endblock := NULL);

[github-actions]

Confirmed defect (high confidence): the no-reform fast path clears the flag but does not shrink t_len. A HOT-indexed live tuple stores its modified-attrs bitmap in the final HotIndexedBitmapBytes(natts) bytes of the item, and heap_form_hot_indexed_tuple() sets t_len = orig_len + bmbytes (heapam.c ~L4943). heap_copytuple() copies the full t_len, so this copy is written into the new heap with t_len still inflated by the trailing bitmap bytes -- a malformed tuple carrying ceil(natts/8) bytes of meaningless trailing data per rewritten row, unlike an equivalent freshly-formed tuple. Clearing the flag alone is not enough; the copy must also strip the bitmap bytes (reduce t_len and, if TupleHasNulls, adjust as needed). Consider reforming instead of copytuple for such tuples, e.g. force needs_reform when HEAP_INDEXED_UPDATED is set so the deform/heap_form_tuple path produces a clean tuple.

[github-actions]

Moderate performance concern (moderate confidence): this dedup is O(noffsets * ntup) once any hot-indexed hop is seen on a page, and page_had_hot_indexed latches true for the rest of the block, so a single stale entry forces the quadratic scan over all remaining tuples on that page. noffsets/ntup are bounded by MaxHeapTuplesPerPage (~291 on 8KB pages), so the worst case is a per-page ~40K-iteration inner loop on the bitmap-heap-scan hot path for dense SIU pages. Given the entries chain-resolve to a bounded set of live offsets, a small sorted/bitset check would avoid the linear rescan. Worth benchmarking before commit; a reproducible number should accompany the claim either way.

[github-actions]

The return; here is unreachable dead code: the preceding elog(ERROR, ...) longjmps out via the error-recovery machinery and never returns. PostgreSQL style does not put a dead return after elog(ERROR) in a void function (see the many elog(ERROR) sites that fall through, or add a pg_unreachable() if the compiler complains). Drop it.

Separately: this new failure path throws "tuple concurrently deleted" directly, whereas previously heap_update() classified the situation and the switch below produced the same message. Confirm the two are not both reachable for the same event (double-report risk is low here since heap_fetch failing short-circuits before heap_update, but the duplicated string is a maintenance smell).

Suggested change

	elog(ERROR, "tuple concurrently deleted");
	return;
	elog(ERROR, "tuple concurrently deleted");

[github-actions]

newtupsize = MAXALIGN(newtup->t_len) is now dead: a few lines below it is unconditionally overwritten by newtupsize = MAXALIGN(newtup->t_len + hi_bmbytes) (with hi_bmbytes == 0 in the non-SIU case, so the value is identical). Either fold the reservation into this single assignment or drop this line to avoid the redundant computation and the reader confusion of assigning newtupsize twice back-to-back.

[github-actions]

Assert(natts == relnatts) guards what the surrounding comment itself calls a data-corruption-class invariant (a divergence misplaces or overruns the trailing bitmap). In a non-assert (production) build a mismatch would silently write the modified-attrs bitmap at the wrong offset -- on-page heap corruption -- rather than failing safe. Since relnatts is passed in only to be asserted against HeapTupleHeaderGetNatts(tup->t_data), consider either dropping the redundant parameter (compute size purely from the tuple's own natts, which is what the reader side does) or hardening this into an unconditional elog(ERROR) so a violated invariant cannot silently corrupt the page in release builds.