Tepid: only update indexes that have changed attributes

[gburd]

No description provided.

[github-actions]

Stray blank line added after XLogRegisterBufData(0, &xlhdr, SizeOfHeapHeader); is unrelated cosmetic churn; per minimal-diff discipline it should be dropped unless functionally required.

[github-actions]

n_chains does not match its documented definition. The comment states it counts LP_REDIRECT items and "Matches the number of distinct HOT chains that have survived the most recent prune." But n_chains is only incremented when len > 0. If a redirect's immediate target is not an LP_NORMAL item (e.g. an LP_DEAD/LP_UNUSED slot, which can be produced by a concurrent prune between reading lp and walking the target, or by an intentionally short-lived redirect target), len stays 0 and the redirect is silently dropped. As a result n_chains can be smaller than the actual LP_REDIRECT count, contradicting the header comment and misleading a diagnostic user. Either count every LP_REDIRECT unconditionally, or fix the comment to state that only redirects with at least one live target tuple are counted.

[github-actions]

This comment is inaccurate. HOT-indexed staleness is not signalled via xs_recheck: index_getnext_tid()/index_fetch_heap() set the separate xs_hot_indexed_stale flag (handled in the block just below at line 782), while xs_recheck is set only by the AM's amgettuple based on scan-key lossiness. Since CLUSTER opens the scan with nkeys==0 (index_beginscan(..., NULL, 0, 0, ...)), a btree no-key scan never sets xs_recheck for a HOT-indexed hop, so this new continue branch never fires for that reason. Rewrite the comment to describe the actual xs_recheck semantics (defensive lossy-index guard) and stop attributing HOT-indexed behaviour to it, or drop the HOT-indexed rationale here entirely and keep it only on the xs_hot_indexed_stale block.

[github-actions]

Clearing only the HEAP_INDEXED_UPDATED bit does not remove the trailing inline modified-attrs bitmap bytes: heap_copytuple() copies tuple->t_len bytes verbatim, t_len for a HOT-indexed tuple includes HotIndexedBitmapBytes(natts) trailing bytes (see heap_form_hot_indexed_tuple: newlen = t_len + bmbytes), and raw_heap_insert()/rewrite_heap_tuple() re-insert with heaptup->t_len unchanged. This is not corruption (deform stops at natts, and no consumer reads the trailing bytes once the flag is clear), but it defeats the compaction goal of CLUSTER/VACUUM FULL by carrying dead trailing bytes into the freshly rewritten heap. Prefer stripping the bytes (adjust t_len down by HotIndexedBitmapBytes) so the rewritten tuple matches what the fresh-form path would produce.

[github-actions]

Once page_had_hot_indexed is set, every subsequent resolved tuple does an O(ntup) linear scan of rs_vistuples, making per-page dedup O(n^2) with n bounded by MaxHeapTuplesPerPage (~291 on 8KB pages). On a dense page full of HOT-indexed hops this is a measurable regression on the bitmap-heap-scan hot path. Consider a cheaper dedup keyed by offset (e.g. a small bitmap/bool array indexed by offnum on the page) instead of a repeated linear scan; that keeps it O(n) and still preserves insertion order.

[github-actions]

This forces the direct-scan copy (seqscan+sort for btree, plain seqscan otherwise) for EVERY heap with more than one index on EVERY CLUSTER / VACUUM FULL, regardless of whether any SIU update ever occurred, and pays an extra RelationGetIndexList() catalog/relcache walk each time purely to count indexes. For the very common multi-index-but-no-SIU table this can regress CLUSTER performance/ordering. If the relcache exposes (or could cheaply expose) a per-relation SIU-used indicator, gate on that instead of the coarse list_length > 1 heuristic; at minimum, reuse an already-retrieved index list rather than fetching and freeing one here.

[github-actions]

Redundant parenthetical: "a HOT-indexed (hot-indexed) hop" repeats the same term. Drop the parenthetical.

[github-actions]

Redundant, misleadingly-named local. index_unchanged is derived solely from the tuple-level update_all_indexes and is identical for every index in the loop; it is used exactly once. More importantly, the name collides with the established per-index concept ii_IndexUnchanged/indexUnchanged in execIndexing.c/genam.h, which is a genuine per-index key-change hint passed to aminsert() (note this call passes false for that hint, not this variable). Inline the expression to avoid the misleading name and the single-use indirection.

Suggested change

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

[github-actions]

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

2 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
src/backend/access/heap/heapam.c L8433: 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 12 issue(s).

• 12 inline, 0 in summary
• ⚠️ 72 warning(s) during review

[github-actions]

This comment overstates coverage as a guarantee. The feature's own tests document that VACUUM collapse to LP_REDIRECT and stub creation are gated on the global xmin horizon, which a concurrent snapshot in the running regression cluster can pin back indefinitely (see contrib/pageinspect/sql/hot_indexed_updates.sql test 12: "whether the chain has collapsed to an LP_REDIRECT is not deterministic here"). So this VACUUM does not reliably "collapse them to LP_REDIRECT forwarders" or "exercise chain collapse" on any given run; the new verify_heapam stub / multi-predecessor / redirect paths this test is meant to guard may not be reached. The test still correctly passes (verify_heapam reports 0 rows regardless of layout), but the comment asserts horizon-dependent behavior as fact. Reword to reflect that collapse is best-effort here, and note that deterministic collapse coverage lives in the isolation spec. Confidence: high.

[github-actions]

Minimal-diff violation: splitting OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid); into a bare declaration plus a separate assignment with a new comment is cosmetic churn unrelated to the SIU feature. This block's logic is untouched by the change. On pgsql-hackers, reformatting otherwise-untouched code invites pushback; keep the initialization inline and drop the added comment. (low confidence this is functionally required -- it isn't.)

Suggested change

	OffsetNumber rdoffnum;
	ItemId rditem;
	/* Resolve the redirect's target offset. */
	rdoffnum = ItemIdGetRedirect(ctx.itemid);
	OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid);
	ItemId rditem;

[github-actions]

Comment/code drift: this branch fires on next_htup->t_infomask2 & HEAP_INDEXED_UPDATED, but that bit is set on collapse-survivor stubs too (a stub is HEAP_ONLY_TUPLE | HEAP_INDEXED_UPDATED, natts==0, per pruneheap.c). Since a redirect can legitimately forward to a stub, the converged target is not necessarily "the same live tuple" as the comment states. Reword to say the target is any HOT-indexed item (live tuple or stub) so the comment matches the actual predicate.

[github-actions]

This relaxation weakens a core amcheck invariant ("HOT chains should not intersect"): any page where two redirects converge on an item carrying HEAP_INDEXED_UPDATED is now silently accepted, including genuinely corrupt pages where two unrelated chains erroneously point at one such item. amcheck's job is to detect exactly this. Consider gating the exception more tightly -- e.g. only when the target is a valid collapse survivor reached via legitimate forwarding -- rather than on the infomask2 bit alone, so real corruption is still reported. (moderate confidence; this appears to be the feature's intended behavior, but the loss of detection should be justified against the design thread.)

[github-actions]

This comment is inaccurate. Verified against the SQL files: hot_updates.sql uses only pageinspect (no amcheck, no btree_gist). Only hot_indexed_updates.sql runs CREATE EXTENSION ... amcheck and ... btree_gist. As written the comment overstates the dependency for hot_updates. Per comment-accuracy discipline, describe what the code does now. Suggest attributing the extra extensions to the correct test. (moderate confidence)

Suggested change

	# hot_updates / hot_indexed_updates verify HOT and HOT-indexed (SIU) heap
	# behaviour using pageinspect, amcheck and btree_gist together, so they live
	# here (where pageinspect is built) and pull in the other two extensions.
	EXTRA_INSTALL = contrib/amcheck contrib/btree_gist
	# The hot_indexed_updates test verifies HOT-indexed (SIU) heap behaviour
	# using pageinspect together with amcheck and btree_gist, so it lives here
	# (where pageinspect is built) and pulls in the other two extensions.
	EXTRA_INSTALL = contrib/amcheck contrib/btree_gist

[github-actions]

This reverts the parallel-safety fix from pageinspect--1.10--1.11.sql. That script deliberately set ALTER FUNCTION bt_page_items(text, int8) 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 this function as PARALLEL SAFE silently reintroduces that bug for anyone who upgrades to 1.14. It must be recreated as PARALLEL RESTRICTED.

(Note: the bytea variant below takes a page image, not a relation, so PARALLEL SAFE is correct there and should stay.)

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]

This is an upgrade script, not a base install. Every other pageinspect--X--Y.sql upgrade script says "rather than via ALTER EXTENSION"; only the base 1.5.sql uses "CREATE EXTENSION". This looks like a copy-paste slip -- the following \echo correctly says ALTER EXTENSION. Fix for consistency.

Confidence: high.

Suggested change

	-- complain if script is sourced in psql, rather than via CREATE EXTENSION
	-- complain if script is sourced in psql, rather than via ALTER EXTENSION

[github-actions]

get_hot_count() is dead code in this file: it is defined here and dropped at the cleanup section (DROP FUNCTION get_hot_count(text)), but it has zero call sites in between -- every measurement in this file uses get_hi_count() (which already reports the hot column). This is exactly the kind of unused scaffolding that fails the minimalism bar on -hackers. Drop the definition here and the matching DROP FUNCTION get_hot_count(text); in the cleanup section.

(Confidence: high -- verified there is no get_hot_count( call between its definition and its DROP.)

[github-actions]

Scope contradiction (moderate confidence). The file header (lines 4-11) states every UPDATE here "either leaves all indexed attributes unchanged or touches only summarizing-index (BRIN) attributes, so the HOT vs non-HOT choice does not depend on whether ... hot-indexed ... is enabled." This GIN section violates that invariant: tags is a non-summarizing GIN-indexed attribute, and the expected output records hot=1. On a pre-hot-indexed server this same UPDATE would be non-HOT (hot=0), so the outcome DOES depend on hot-indexed being enabled -- the exact thing this file claims to exclude. This test belongs in hot_indexed_updates.sql, or the file header must be corrected to reflect that section 9 is hot-indexed-specific.

[github-actions]

Comment drift (low confidence). The comment says the chain is "rooted at start_ctid", but the function's first loop looks for a predecessor tuple whose t_ctid points to start_ctid and, if found, begins emitting from that predecessor -- i.e. it does not root at start_ctid, and it walks back only one level rather than to the true chain head. For chains longer than two members this will not emit from the actual root. Given the tests only exercise short chains this is not a correctness problem here, but the comment does not match the code.

[github-actions]

🔍 OCR found 100 issue(s).

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

---

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

Trailing double blank line after the function body. Reduce to a single blank line to keep the diff pgindent-clean.

💡 Suggested change

Before:

+	return true;
+}
+
+
 
 /*
  * Record an unused line pointer that is left unchanged.

After:

+	return true;
+}
+
 /*
  * Record an unused line pointer that is left unchanged.

---

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

Comment inaccuracy: the header says "a live HOT-indexed ... heap-only tuple", but this predicate is also called on DEAD chain members in the all-dead branch (ndeadchain == nchain, heap_prune_record_dead_or_unused(prstate, chainitems[i], true)) and on dead-prefix members in the collapse branch. It returns true for a committed-then-dead tuple (not XMIN_INVALID), not only live ones. The word "live" mis-describes actual usage; state that it identifies any HOT-indexed member whose LP may still be referenced by a not-yet-swept index entry, regardless of visibility.

---

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

Minimal-diff violation: this hunk only adds a trailing period to an otherwise-unchanged comment ("vacuum's second pass" -> "...second pass."). Gratuitous rewording of untouched lines is a common rejection reason on pgsql-hackers. Revert this line to its original form.

💡 Suggested change

Before:

+			/* Setting LP_DEAD to LP_UNUSED in vacuum's second pass. */

After:

			/* Setting LP_DEAD to LP_UNUSED in vacuum's second pass */

---

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

Two consecutive blank lines were introduced here. PostgreSQL style (pgindent) uses a single blank line between function definitions; git diff --check/pgindent will flag this as whitespace churn. Remove the extra blank line.

💡 Suggested change

Before:

Assert(nunused > 0);

After:

	return true;
}

/*
 * Record an unused line pointer that is left unchanged.

---

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

Pure comment reword (adds a trailing period) on an otherwise-untouched line. This is gratuitous churn that violates the minimal-diff discipline and is a common rejection reason on -hackers; revert to the original comment text.

💡 Suggested change

Before:

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

After:

			/* Setting LP_DEAD to LP_UNUSED in vacuum's second pass */

---

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

An extra blank line was introduced before the heap_page_prune_execute comment, producing two consecutive blank lines. pgindent expects a single blank line between functions; remove the added blank line to avoid whitespace churn flagged by git diff --check.

💡 Suggested change

Before:

Assert(nunused > 0);

After:

}

/*
 * Perform the actual page changes needed by heap_page_prune_and_freeze().

---

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

Confidence: high. This relaxes pin-drop safety to depend on a new invariant: "any caller that sets xs_want_itup but not xs_index_only always fetches/visibility-checks the heap." That invariant is enforced only by convention, and it is a footgun. get_actual_variable_range() in selfuncs.c is a live counter-example: it sets xs_want_itup (not xs_index_only) and skips index_fetch_heap() when VM_ALL_VISIBLE is true (see selfuncs.c ~7298-7311), i.e. it does rely on the VM without fetching the heap. That caller happens to be safe here only because it uses SnapshotNonVacuumable, so the separate IsMVCCLikeSnapshot() guard on the next line forces dropPin=false. A future caller that sets xs_want_itup with an MVCC snapshot and skips the heap based on the VM (without setting xs_index_only) would silently get unsafe eager pin dropping and expose the concurrent TID-recycling race. Recommend the comment explicitly credit the IsMVCCLikeSnapshot() guard as the backstop for such non-index-only, VM-relying callers, rather than asserting all plain xs_want_itup scans "still fetch and visibility-check the heap" (that blanket claim is contradicted by selfuncs.c).

---

📄 src/backend/access/nbtree/nbtinsert.c (L906-L908)

Aspirational/future-tense comment. Per PostgreSQL comment discipline, comments should describe current behavior, not speculative future work. The justification for the keycol != 0 Assert (expression-index attributes are disqualified by HeapUpdateHotAllowable) is legitimate WHY-documentation and should stay, but the trailing clause narrating where a not-yet-implemented expression path "would be added" is speculative scaffolding that reviewers on -hackers routinely reject. Trim it.

💡 Suggested change

Before:

 *	number).  We assert that rather than handle a keycol == 0 case that cannot
 *	occur; if expression-index selective maintenance is implemented in the
 *	future, this is where an expression-evaluating comparison would be added.

After:

 *	number).  We assert that rather than handle a keycol == 0 case that cannot
 *	occur.

---

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

index_unchanged is a single-use inversion of !update_all_indexes and is referenced only on the next line. It arguably obscures rather than clarifies: update_all_indexes == false already carries the exact meaning. Consider inlining the condition (if (!update_all_indexes && !indexInfo->ii_Summarizing) continue;) and folding the explanation into the existing comment, per minimalism discipline. Low priority / style only. (low confidence)

💡 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/access/nbtree/nbtinsert.c (L945-L948)

This helper runs while the nbtree leaf buffer is exclusive-locked (and possibly a right-sibling nbuf read-locked). heap_datum comes straight from a BufferHeapTuple slot, so for a varlena key column it may be a short-header, inline-compressed, or out-of-line (EXTERNAL) TOAST pointer. The btree comparator (via PG_GETARG_*_PP) will detoast it -- an EXTERNAL value triggers a TOAST-table index scan and buffer reads while holding the index leaf LWLock. That both extends the time a contended index buffer lock is held and introduces a new lock-ordering edge (index leaf lock -> TOAST relation buffers) on the unique-insert hot path that the pre-existing code never took. A unique key can legitimately be out-of-line TOASTed in the heap even when its inline-compressed index form fits. Consider detoasting the heap datum with the buffer lock released, or documenting/justifying why holding it is acceptable here. (moderate confidence)

---

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

Embedding the slot allocation with an assignment side-effect inside the else if boolean short-circuit is a footgun. It works only because table_slot_create() never returns NULL (it errors internally); if that ever changed, the && would short-circuit, skip table_index_fetch_tuple_check() entirely, and silently treat a real conflict as non-conflicting -- i.e. admit a duplicate into a unique index. This couples uniqueness correctness to an unrelated module's non-NULL guarantee in a non-obvious spot. Prefer creating the slot on a plain statement line before the if/else if chain (it is created at most once per call anyway) so the fetch's return value alone drives the branch. (moderate confidence)

---

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

DRY: these two index_insert() calls are identical eight-argument blocks that differ only in the TID pointer passed (&siu_tid vs tupleid). Keeping two full copies in sync is error-prone; a future change to the argument list (or a new AM parameter) must touch both. Prefer selecting the pointer into a single local and issuing one call. The comment about not mutating the slot's tts_tid is valid (line 303 confirms tupleid = &slot->tts_tid), so keep the copy -- just don't duplicate the whole call.

e.g.:
ItemPointerData siu_tid;
const ItemPointerData *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 (L917-L922)

This weakens the long-standing duplicate-TID corruption check. Once found_self_siu_hit is latched true by any stale self-arrival, every subsequent self-arrival for this TID -- including a genuinely-corrupt second non-stale canonical entry -- is silently tolerated via continue, because index_scan->xs_hot_indexed_stale || found_self_siu_hit is satisfied by the latched flag alone. So a real duplicate-TID corruption that happens to coexist with a legitimate stale chain entry will no longer be detected. The tolerance is necessary for the feature, but consider tightening it so only the stale arrivals are tolerated (i.e. still raise when a non-stale repeat arrives after the one legitimate non-stale self entry has already been seen), rather than blanket-tolerating all repeats once any stale one is observed.

---

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

This assertion is user-reachable and can fire in assert-enabled builds. ExecUpdateModifiedIdxAttrs() -> ExecCompareSlotAttrs() keeps attribute 0 (the whole-tuple reference, whose bitmap index equals TableTupleUpdateAllIndexes) whenever the target relation has an index whose expression references a whole-row Var (e.g. CREATE INDEX ON t ((row_to_json(t.*)))). A logical-replication subscriber applying an UPDATE to such a table will therefore enter here with the sentinel already present in modified_idx_attrs, tripping this Assert. The sibling path in nodeModifyTable.c (ExecUpdateAct, lines 2658-2666) explicitly documents that the sentinel may legitimately be present on input and is harmless. Per PostgreSQL convention, Assert() is only for can't-happen invariants, not user-reachable states. Drop this assertion (the all_indexes check below already handles the sentinel correctly). Confidence: high.

---

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

Missing bounds assertion before TupleDescCompactAttr(tupdesc, attrnum - 1). This is an exported public API (declared in executor.h, callable cross-module), and TupleDescCompactAttr does no bounds checking -- it directly indexes tupdesc->compact_attrs[i]. A positive attrnum beyond tupdesc->natts (e.g. a caller passing a Bitmapset that doesn't match the descriptor) yields an out-of-bounds read.

The counterpart this comment claims to be functionally equivalent to, heap_attr_equals() in heapam.c, guards exactly this with Assert(attrnum <= tupdesc->natts) before its TupleDescCompactAttr(tupdesc, attrnum - 1). This is a legitimate can't-happen invariant; add the matching assert to harden the API and mirror the heap-side path.

Confidence: moderate.

💡 Suggested change

Before:

		att = TupleDescCompactAttr(tupdesc, attrnum - 1);

After:

		Assert(attrnum <= tupdesc->natts);
		att = TupleDescCompactAttr(tupdesc, attrnum - 1);

---

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

The header comment says this returns the "cached" HOT-indexed apply mode, but the body comment two lines below explicitly states "Derive directly from MySubscription rather than caching", and the code just reads MySubscription->hotindexedonapply on every call with no caching. The two comments contradict each other. Fix the header wording so it describes what the code does now (a direct read), not a cache. (Same stale "cached" wording is also in the logicalworker.h declaration comment.)

💡 Suggested change

Before:

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

After:

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

---

📄 src/backend/executor/nodeModifyTable.c (L2646-L2647)

Memory leak on the concurrent-update retry and early-return paths. modified_attrs is allocated here (RelationGetIndexAttrBitmap returns a bms_copy in the current, per-query context) and is only freed in ExecUpdateEpilogue, which runs solely on TM_Ok. On TM_Updated the switch does goto redo_act, which re-enters ExecUpdateAct; its updateCxt->modified_attrs = NULL reset (line ~2522) orphans the bitmap allocated by the previous attempt without freeing it. Likewise, the TM_Deleted / TM_SelfModified early return NULL paths exit before the epilogue. Under contention a bulk UPDATE thus accumulates one Bitmapset per affected row in the per-query context. Free the previous bitmap on retry (e.g. bms_free it in the ExecUpdateAct reset) and on the early-return paths, or move the free to a common exit.

---

📄 src/backend/executor/nodeModifyTable.c (L2521-L2522)

This reset silently drops the bitmap allocated by a prior ExecUpdateAct call on the same UpdateContext (the TM_Updated -> redo_act retry path re-enters this function). Since heap_update does not free modified_idx_attrs (it is caller-owned) and the epilogue only runs on TM_Ok, the earlier allocation leaks. bms_free the existing set before overwriting it.

💡 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/executor/nodeModifyTable.c (L219-L219)

Header comment for this exported helper has typos that hurt readability at the declaration site: 'slot datum that are in the UPDATE statment' -- 'statment' should be 'statement', and 'datum' reads awkwardly (use 'slot data/datums'). Also 'form the index datum have changed' is ungrammatical.

💡 Suggested change

Before:

 * are in the UPDATE statment and are known to be referenced by at least one

After:

 * are in the UPDATE statement and are known to be referenced by at least one

---

📄 src/backend/executor/nodeModifyTable.c (L2658-L2666)

This 8-line comment block restates the whole-row / TableTupleUpdateAllIndexes rationale already covered by the ExecUpdateModifiedIdxAttrs header comment and the epilogue. It sits between the 'Call into the table AM' comment and the table_tuple_update call, producing two large adjacent comment blocks about the same point. Per PostgreSQL comment discipline (explain why, concisely), consider trimming or folding this into the existing note to keep the diff minimal.

---

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

Field-name drift: this references tuples_hot, but the actual counter is tuples_hot_updated (the field name spelled out two lines above). Since this comment enumerates precise field names, keep it accurate.

s/tuples_hot:/tuples_hot_updated:/

💡 Suggested change

Before:

		 * advance them.  tuples_hot_indexed_updated is counted in *addition* to
		 * tuples_hot: every hot-indexed update is also a HOT update.

After:

		 * advance them.  tuples_hot_indexed_updated is counted in *addition* to
		 * tuples_hot_updated: every hot-indexed update is also a HOT update.

---

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

Unconditionally stripping the SIU bit in ItemPointerCompare silently changes the SQL-visible tid type ordering/equality. ItemPointerCompare backs tidcmp and the tid <,<=,=,>=,> operators and the tid btree opclass (utils/adt/tid.c). tidin accepts any offset up to USHRT_MAX, so a user can create e.g. tid '(0,16384)' (bit 14 set) and tid '(0,0)'. After this change tidcmp('(0,16384)','(0,0)') returns 0 and the equality operator returns true, while tidout still prints them differently. Any btree index / ORDER BY / DISTINCT / GROUP BY on a tid column now collapses (0,0), (0,16384), (0,32768), (0,49152) into one class, breaking uniqueness/total-order guarantees and yielding wrong results and amcheck failures. ItemPointerCompare cannot distinguish a user's genuine offset 16384 from an internally-flagged offset 0 -- the bytes are identical. The strip needs to be confined to the internal callers that actually know a TID is a HOT-indexed heap-item TID (the design notes say the flag only ever lands on such TIDs), not applied in the general-purpose comparator exposed to SQL. High confidence.

---

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

Whitespace-only churn unrelated to this change. This file otherwise has no functional modification for the feature; removing this blank line is a cosmetic edit to an untouched section and violates the minimal-diff discipline (needless noise, merge-conflict risk). Drop this hunk entirely so amapi.h is not touched by the patch. (high confidence)

💡 Suggested change

Before:

 typedef struct IndexInfo IndexInfo;
 
 /*

After:

 typedef struct IndexInfo IndexInfo;
 
 
 /*

---

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

Memory leak on the cache-miss path. TextDatumGetCString(datum) allocates a fresh CString and stringToNode() allocates a node tree, both in the caller's current memory context (a per-query/per-tuple context in the executor via ExecSetIndexUnchanged), and neither is ever freed. Compare RelationGetIndexExpressions/RelationGetIndexPredicate just above, which carefully pfree(exprsString) after stringToNode. Free the CString after parsing (and ideally list_free the transient trees). While the leak is bounded to one occurrence per relcache lifetime (rd_indattr caches the result afterwards), it still leaks into whatever context is current at first use, which for the executor callers is not a throwaway context.

💡 Suggested change

Before:

			indexprs = (List *) stringToNode(TextDatumGetCString(datum));
			pull_varattnos((Node *) indexprs, 1, &attrs);

After:

			char	   *exprsString = TextDatumGetCString(datum);

			indexprs = (List *) stringToNode(exprsString);
			pfree(exprsString);
			pull_varattnos((Node *) indexprs, 1, &attrs);
			list_free_deep(indexprs);

---

📄 src/backend/utils/cache/relcache.c (L5384-L5385)

Same leak here for the predicate branch: the CString from TextDatumGetCString and the parsed indpred tree are never freed. Free them after pull_varattnos, mirroring RelationGetIndexPredicate's pfree of predString.

💡 Suggested change

Before:

			indpred = (List *) stringToNode(TextDatumGetCString(datum));
			pull_varattnos((Node *) indpred, 1, &attrs);

After:

			char	   *predString = TextDatumGetCString(datum);

			indpred = (List *) stringToNode(predString);
			pfree(predString);
			pull_varattnos((Node *) indpred, 1, &attrs);
			list_free_deep(indpred);

---

📄 src/backend/utils/cache/relcache.c (L5289-L5293)

Stale/contradictory header comment. This claims the bitmap is built from RelationGetIndexExpressions/RelationGetIndexPredicate, but the function body (see the block below) explicitly and deliberately does NOT call them, parsing the raw catalog trees instead. Per project comment discipline, the header must describe what the code does now; update it to reference rd_index->indkey plus the raw indexprs/indpred trees to avoid misleading future readers.

---

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

This test uses CREATE EXTENSION amcheck plus verify_heapam()/bt_index_check(), but pg_upgrade's build does not install amcheck into the temp install used by make check/meson (the Makefile's EXTRA_INSTALL lists only test_decoding and two test modules, and meson deps lists only test_ext). Sibling tests that need amcheck add EXTRA_INSTALL = contrib/amcheck (see src/bin/pg_amcheck/Makefile, src/test/recovery/Makefile, src/test/modules/nbtree/Makefile) and the corresponding meson dep. As written, CREATE EXTENSION amcheck will fail and the test dies hard rather than being runnable. Either wire the amcheck install dependency into the pg_upgrade Makefile/meson.build, or guard the extension creation and skip when amcheck is unavailable (as amcheck's own TAP tests can rely on being co-located, this test cannot).

---

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

The hi table runs with default autovacuum enabled, so the collapse-stub state this test is meant to preserve is non-deterministic: a concurrent autovacuum can reclaim/collapse chains before the explicit VACUUM, or hold back the xmin horizon so the intended xid-free forwarding stubs never form. Every sibling test that depends on precise chain/stub shape disables autovacuum -- 055_hot_indexed_recovery.pl sets autovacuum = off, and the pageinspect hot_indexed_updates.sql tables use autovacuum_enabled = false. Add autovacuum_enabled = false to the WITH clause (or autovacuum = off on the node) so the test reliably exercises collapse stubs instead of silently passing without them.

💡 Suggested change

Before:

	CREATE TABLE hi (id int PRIMARY KEY, k int, v int, big text)
	  WITH (fillfactor = 50);

After:

	CREATE TABLE hi (id int PRIMARY KEY, k int, v int, big text)
	  WITH (fillfactor = 50, autovacuum_enabled = false);

---

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

The expected index count is a bare magic literal '4' (hi_pkey + hi_k + hi_v + hi_big). If the HOT-indexed feature or a later series in this series alters index bookkeeping, this silently misvalidates or fails for the wrong reason with no diagnostic about which index was added/dropped. Prefer asserting per-index (e.g. compare the set of index names, or assert each bt_index_check returns without error) so a mismatch names the offending index rather than a count.

---

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

The comment claims the ABA chain is validated "through a forced index scan", but SET enable_seqscan/enable_bitmapscan = off only discourages those paths; nothing here verifies an index scan was actually chosen (the planner may still fall back, e.g. to a seqscan when no index path is viable). If the index-scan path silently isn't taken, these assertions pass via a non-index path and no longer test collapsed-chain index-scan correctness. Add an EXPLAIN check that the plan uses an Index Scan on hi_k, matching how the feature's other tests assert the forced path.

---

📄 src/include/access/heapam_xlog.h (L353-L356)

This comment is inaccurate/incomplete about how the stub survives replay without carrying the bitmap in WAL. heap_page_prune_execute() overwrites the tuple's natts with the stub sentinel 0 (HeapTupleHeaderSetNatts(tup, 0)) and stashes the write-time natts into the block half of t_ctid via HotIndexedStubSetBitmapNatts() -- readers need it to locate the trailing bitmap. This comment claims only t_ctid.offnum is set and that the bitmap simply survives untouched, but the bitmap's location is only recoverable because the block half of t_ctid is also rewritten to hold bitmap_natts. Since this WAL layout comment is the authoritative description of the on-disk stub format, omitting the natts-in-t_ctid.blkid detail is misleading (moderate confidence). Suggest documenting that t_ctid's block half carries the preserved write-time natts.

---

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

This block comment deviates from the PostgreSQL comment style used everywhere else in this file (and the tree): text should not start on the opening /* line, and continuation lines should be aligned with a leading *. As written, pgindent will reflow this and the CI pgindent check will complain. Match the style of the neighboring comments.

💡 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 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.
 */

---

📄 src/include/access/heapam.h (L509-L510)

pgindent alignment: these two adjacent declarations align their continuation parameters differently. Under pgindent the continuation must align to the column just after the opening (, which differs between HeapUpdateHotAllowable( and HeapUpdateDetermineLockmode( (different name lengths). As written, line 510's const Bitmapset *modified_idx_attrs is not aligned under its function's paren, so this will not be pgindent-clean. Run pgindent on the file.

---

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

Do not bump CATALOG_VERSION_NO in the patch, and do not add a comment placeholder for it either. Bumping the catversion (and touching catversion.h at all) is the committer's job at push time. Including it here guarantees needless merge/rebase conflicts with every other in-flight catalog patch and is the single most common author mistake in catalog patches. Revert this file entirely; the catalog change (subhotindexedonapply) belongs in the pg_*.dat files, not here.

---

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

British spelling: PostgreSQL sources use American English. s/behaviour/behavior/. (low confidence, style nit)

💡 Suggested change

Before:

 *		  behaviour before this option was introduced.

After:

 *		  behavior before this option was introduced.

---

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

This comment contradicts the implementation. GetHotIndexedApplyMode() in worker.c does not cache anything; its own comment explicitly states it "[d]erive[s] directly from MySubscription rather than caching, so there is no second copy to keep in sync". Describing the returned value as "cached" here is misleading comment drift. Drop the word "cached". (low confidence: documentation accuracy nit, not a functional defect.)

💡 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/pgstat.h (L772-L781)

The continuation backslashes in these two new macros are misaligned relative to the surrounding block (lines 747-771), where every \ is tab-aligned to the same column. Here the #define line's backslash sits further right, and on the body lines (...upd_skipped++;\ / ...upd_matched++;\) the backslash abuts the code with no separating whitespace. This does not match the existing style and will not survive pgindent cleanly. Re-align all continuation backslashes to the block's column.

---

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

This comment describes a discipline for the return value of RelationGetIndexedAttrs(), not for this field. rd_indattr is a private relcache cache: the only reader is RelationGetIndexedAttrs(), which always returns a fresh bms_copy() (fast path) and never hands out the raw pointer. There are no direct "consumers" of rd_indattr, so the "Consumers must bms_copy ... beyond any potential AcceptInvalidationMessages() call" sentence is misplaced here and misleading. Keep the field comment terse (matching the neighboring rd_indexprs/rd_indpred/rd_exclops trailing comments) and move the copy/lifetime discipline to the accessor's header comment (where it already exists). Confidence: high.

💡 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:

	Bitmapset  *rd_indattr;		/* heap attrs referenced by this index */

---

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

Placement nit: rd_indattr is derived from rd_index->indkey, rd_indexprs, and rd_indpred, but it is placed after rd_opcoptions, separated from those logically-related fields. Consider grouping it with rd_indexprs/rd_indpred (lines 213-214) so the index-derived caches sit together, matching how the file otherwise organizes cache fields. Confidence: low.

---

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

The public ItemPointerOffsetNumberMask macro is a footgun. Applied directly (raw & ItemPointerOffsetNumberMask) without the sentinel guard that ItemPointerOffsetNumberStrip() provides, it silently corrupts SpecTokenOffsetNumber (0xfffe -> 0xbffe) and MovedPartitionsOffsetNumber (0xfffd -> 0xbffd) -- exactly the regression your own comment above says was caught by the isolation suite. It is currently used only inside ItemPointerOffsetNumberStrip(), so it does not need to be exposed at all. Consider inlining the mask into the strip helper and dropping this macro to prevent future misuse. (moderate confidence)

---

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

These header block comments are extremely long and document design rationale spanning nbtree, bitmap scans, execIndexing, and even an isolation-suite regression. itemptr.h is low-level foundational infrastructure; this level of cross-subsystem narrative belongs in the HOT-INDEXED design notes (README.HOT-INDEXED, which you already reference), with only a terse pointer here. Per project style, header comments should explain the invariant concisely, not re-derive the whole feature. Consider trimming to the essential contract (bit 14 is free below MaxOffsetNumber; set only on index-tuple heap TIDs; sentinels must not be masked) and referencing the design notes for the rest. (low confidence)

---

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

Fragile synchronization: sleep 2 is not a reliable way to wait for the postmaster to accept connections. On a loaded machine or slow filesystem, the subsequent seed() psql can connect before the server is ready and fail; since stdout/stderr are redirected via pg_ctl -l, that failure is easy to miss and silently corrupts the iteration's TPS numbers. Use pg_ctl start -w (blocks until the server is ready) instead of a fixed sleep. Confidence: high.

💡 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/bit14_ab.sh (L47-L49)

rm -rf on a path derived entirely from the BENCH env var (default /scratch/siu-bench) plus a variant name is a footgun: if BENCH is empty or unexpectedly set, this recursively deletes an unintended directory. The sibling run.sh deliberately avoids this by using find "$datadir" -mindepth 1 -delete && rmdir. Match that safer pattern, or at minimum assert datadir is non-empty and under $BENCH before removing. Confidence: moderate.

💡 Suggested change

Before:

  local datadir=$BENCH/_data_bit14_$v
  rm -rf "$datadir"
  mkdir -p "$datadir"

After:

  local datadir=$BENCH/_data_bit14_$v
  [ -d "$datadir" ] && find "$datadir" -mindepth 1 -delete && rmdir "$datadir"
  mkdir -p "$datadir"

---

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

DRY: bin_of, LD_of, psql_as, pgbench_as, start_pg, stop_pg, and seed() are near-verbatim copies of the helpers in the sibling run.sh (with minor divergences: _data_bit14_ vs _data_, rm -rf vs find -delete, missing log_destination). Copy-pasted harness logic drifts out of sync over time. Factor the shared helpers into a common sourced file (e.g. scripts/common.sh) and source it from both run.sh and this script. Confidence: high.

---

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

The harness silently depends on preconditions produced by build.sh ($BENCH/{master,tepid}/usr/local/pgsql, with master=bit14-reverted and tepid=bit14-present) but never validates them. If a variant tree is missing, bin_of/LD_of still expand to a non-existent path and the script fails late (or, if only one variant is missing, produces a one-sided CSV). Worse, if the two build revisions were swapped the entire A/B conclusion inverts with no diagnostic. Add an up-front check that both $(bin_of master)/postgres and $(bin_of tepid)/postgres exist (and ideally log postgres --version for each) before running. Confidence: high.

---

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

Whole-tree concern: this is a developer-local benchmark harness with hardcoded absolute paths (/scratch/siu-bench, -h /tmp), a hardcoded default PORT=57481 that can collide with a concurrent instance, and LD_LIBRARY_PATH juggling against pre-installed build variants. Ad-hoc benchmark scaffolding like this is normally kept out of the PostgreSQL source tree and will draw a "does this belong in-tree?" objection on pgsql-hackers. Confirm whether the src/test/benchmarks/siu/ tree is intended for commit or should be excluded from the patch (it is not needed to build or test the SIU feature itself). Confidence: high.

---

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

The header mischaracterizes the workload's read/write mix. Every transaction runs the SELECT (line 7) unconditionally, and 20% (which values 81-100) additionally run the UPDATE. So the true ratio is 100% selects / 20% updates (5 reads per write), not the "80% selects, 20% updates" (4:1) the comment implies. Since these scripts exist to interpret benchmark numbers, the description should state the actual mix to avoid misreading results. Confidence: high.

💡 Suggested change

Before:

-- Mixed workload: 80% selects, 20% indexed-column updates.

After:

-- Mixed workload: every txn does a select; 20% also do an indexed-column update.

---

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

Purpose/YAGNI (high confidence): This entire script -- along with its siblings under src/test/benchmarks/siu/scripts/ (run.sh, soak.sh, bit14_ab.sh, bloat.sh and the *.sql pgbench files) -- is a personal A/B benchmark harness, not a regression or TAP test. It is not wired into any Makefile, meson.build, test schedule, or CI job (confirmed: nothing in the tree references siu), so it is dead scaffolding as far as the build/test system is concerned. It also hardcodes developer-specific paths ($HOME/ws/postgres/tepid, /scratch/siu-bench, /scratch/pg) and a private branch name tepid, so no reviewer can reproduce it. PostgreSQL does not commit throwaway benchmarking harnesses into the tree. This belongs on the -hackers thread as an attachment supporting the performance claim, not in the patch. Recommend dropping the whole src/test/benchmarks/siu/ directory from this change.

---

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

Data-loss/repo-mutation footgun (high confidence): this script cd "$REPO" into the developer's live working tree and runs git checkout --detach on it as a side effect of a benchmark build. Combined with meson install --destdir=/ writing to an absolute prefix, running this in the wrong repo silently rewrites HEAD. The EXIT trap only restores state if $ORIG resolved correctly; there is no verification that the checkout actually succeeded or that ORIG is non-empty. Mutating a user's working repo to build a benchmark is unsafe by design.

---

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

Robustness (moderate confidence): the dirty-repo guard git status --porcelain | grep -v '^??' | grep -q . only skips untracked files; it does not distinguish staged-but-committable states cleanly and, more importantly, does not stash/restore the working tree, so the subsequent git checkout --detach still discards nothing but leaves the user detached until the trap fires. Given the script performs destructive git checkout on the live repo, the guard is not a sufficient safety net.

---

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

No prerequisite check for the build. If the tepid variant (BINDIR default $BENCH/tepid/usr/local/pgsql/bin) does not exist -- e.g. build.sh was never run -- the script proceeds and fails deep inside pg_ctl/initdb with a confusing 'No such file or directory'. The sibling build.sh uses an explicit die() precondition helper for exactly this. Add an early check that BINDIR/initdb (or the whole BINDIR) exists and error out with an actionable message pointing at build.sh, so it skips/fails cleanly when the prerequisite is missing.

💡 Suggested change

Before:

base=$(dirname "$BINDIR")

After:

if [ ! -x "$BINDIR/initdb" ]; then
  echo "bloat: $BINDIR/initdb not found; run build.sh first (or set BINDIR)" >&2
  exit 1
fi
base=$(dirname "$BINDIR")

---

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

The trailing echo claims idx_a growth is 'bounded' vs 'unbounded' and idx_b_skips reflects HOT-indexed skips, but these depend on updates actually taking the HOT-indexed path. With fillfactor=50 + a 40-byte pad and 20 full-table UPDATEs per cycle (no WHERE), whether a given update stays HOT-indexed or falls back to non-HOT depends on per-page free space, which this script neither controls nor asserts. The comparison is therefore not deterministic run-to-run and the stated conclusion may not hold. Consider printing the HOT-indexed vs non-HOT update counts (n_tup_hot_indexed_upd / n_tup_upd) alongside the sizes so the reader can verify the demonstrated split rather than relying on the assumed narrative.

---

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

Cross-backend stats staleness: idx_b_skips is read via pg_stat_all_indexes from a fresh psql connection immediately after the update transactions. PostgreSQL rate-limits cumulative-stats flushing (PGSTAT_MIN_INTERVAL, 1000ms), and idle/commit flush is best-effort, so the last cycle's skip count may not yet be visible to the reader, under-reporting idx_b_skips. If exact accounting matters here, either add a short settle before reading, or reset+drive the workload such that the reader observes a fully-flushed value.

---

📄 src/test/benchmarks/siu/scripts/wide_update.sql (L1-L1)

This file is part of the private A/B benchmark harness under src/test/benchmarks/siu/, which hardcodes developer-specific concepts and paths in its siblings (variant name "tepid", /scratch/siu-bench, $HOME/ws/postgres/tepid in run.sh/build.sh). A throwaway performance harness like this is not commit-ready material for the PostgreSQL tree: such scripts are normally attached to the -hackers thread as supporting evidence for a benchmark claim, not committed under src/test/. It also isn't wired into any Makefile/meson build and won't be run by the buildfarm, so it adds maintenance surface with no CI coverage. Recommend dropping the whole siu/ benchmark directory from the patch (and posting the numbers/scripts on the list instead). Confidence: high.

---

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

Comment inaccuracy: this says the workload UPDATEs "a configurable number of those indexed columns", but the default WIDE_STEPS includes 0, and build_wide_set_clause(0) in run.sh emits the SET clause "id=id" -- a PK self-assignment that updates zero indexed columns. The comment should note that N can be 0 (no indexed-column update, PK no-op touch), otherwise it drifts from the actual behavior of the harness that drives this script. Confidence: moderate.

---

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

Committability/scope (high confidence). This entire src/test/benchmarks/siu/ tree is developer-only benchmark scaffolding: it is not wired into any Makefile/meson build (a search across all build files finds zero references to benchmarks), it is not a PostgreSQL::Test TAP test, and it hardcodes machine-specific assumptions — an absolute default BENCH=/scratch/siu-bench, a fixed PORT=57503, -h /tmp, and a pre-built two-variant layout ($BENCH/master, $BENCH/tepid). It cannot run in the buildfarm or under the standard schedules. Per PostgreSQL patch hygiene, ad-hoc benchmark harnesses with absolute paths and machine-specific layout belong on the -hackers thread as supporting material, not committed to the tree. Recommend excluding the whole siu benchmark tree from the patch.

---

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

Data-loss footgun (medium confidence). This recursively wipes $BENCH/_data_$v. $BENCH defaults to an absolute path and $v comes from the outer loop; a misconfigured BENCH or an empty $v expands the path and deletes unintended data with no confirmation. Under set -e, a partial find -delete failure can also leave the directory half-removed. Note the identical idiom exists in run.sh, so it is an established (bad) pattern in this tree rather than unique to soak.sh. Also, find -mindepth/-delete is GNU-specific and not portable to the BSD/macOS/Solaris platforms PostgreSQL targets.

---

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

Missing cleanup trap (medium confidence). stop_pg is only reached at the end of run_soak. With set -euo pipefail, any earlier failure (missing binary, failed setup SQL, pgbench error) aborts the script leaving a running postmaster and a populated data dir behind, and the next variant's server never gets stopped. Sibling scripts bloat.sh and build.sh install a trap ... EXIT to guarantee cleanup; soak.sh diverges. Add an EXIT trap that stops any started server.

💡 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 (L49-L50)

Fixed sleep 2 used as server-readiness synchronization (low confidence). This is racy on a slow/loaded machine (initdb+start may not be accepting connections in 2s, causing the subsequent setup psql to fail) and is exactly the anti-pattern PostgreSQL test guidance forbids. pg_ctl start -w waits for readiness deterministically; sibling bloat.sh already uses -w start.

---

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

Misleading metric name (medium confidence). The column is named heap_pages and the header comment claims it samples heap/table bloat, but pg_table_size() includes the TOAST relation plus FSM/VM forks, so this is not a heap-only page count. For the wide e text column with repeat('x', 20) this may not toast, but the label still drifts from what is measured and will mislead readers of the CSV. Either rename to table_pages or use pg_relation_size('siu_table', 'main')/8192 for an actual heap main-fork page count.

---

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

Sampling delta can silently report zero (low confidence). pg_stat_user_tables counters are cumulative and updated asynchronously by the stats subsystem, so a sample taken right at a tick can lag; if now_tot <= prev_tot the delta falls into the else branch reporting tps=0/hot=0 even while pgbench is actively driving updates, masking real activity in the CSV. Consider tolerating stale samples (skip the row) rather than emitting a spurious zero.

---

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

Minor style/consistency nit (low confidence, non-blocking): every other test in this schedule uses hyphen-separated names (e.g. ddl-dependency-locking, cluster-conflict). This is the only entry using underscores. Consider renaming the spec/expected/schedule triple to hot-indexed-adversarial to match the established convention in this file. Functionally fine either way.

💡 Suggested change

Before:

+test: hot_indexed_adversarial

After:

+test: hot-indexed-adversarial

---

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

This benchmark harness hardcodes an absolute bench root (/scratch/siu-bench), a fixed default port (57480), psql -h /tmp, and depends on prebuilt variant trees under $BENCH/{master,tepid}/usr/local/pgsql that only exist on the author's machine. As committed under src/test/, it cannot run in the buildfarm/cfbot, is not wired into any Makefile/meson target, and will collide with other processes sharing the port/socket dir. PostgreSQL test conventions forbid hardcoded ports/paths; benchmark scaffolding that only runs on the author's box is WIP and will draw a rejection on -hackers. Either integrate with PostgreSQL::Test infrastructure (allocate a port, use a temp datadir) or keep this out of the committed patch.

---

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

Env vars flow unvalidated into SQL/DDL and a sed replacement. WIDE_COLS/SCALE are interpolated into DDL and seq 1 "$WIDE_COLS"; WIDE_STEPS values become the sed "s/:wide_set_clause/$extra_set/" replacement. A non-numeric or metacharacter-laden value silently corrupts the generated SQL/DDL or the pgbench script (/, &, \ in the sed RHS are footguns). Add a numeric guard (e.g. [[ "$WIDE_COLS" =~ ^[0-9]+$ ]] || die and likewise per WIDE_STEPS token) before using these in DDL/sed.

---

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

sample_peak launches the sampler as ( ... ) & inside a command substitution sampler_pid=$(sample_peak ...). The backgrounded subshell is therefore a grandchild of the main shell (child of the $(...) subshell), so the later wait "$sampler_pid" cannot reap it -- which is exactly why line ~264 tolerates failure with || true. The consequence is that wait does not actually block until the sampler finishes writing the .cpu file, so cat "$cpu_rss_file" can read a truncated/empty file and record NA (or a partial peak) for every workload. Launch the sampler directly in the calling shell (not via command substitution) so its PID is a real child that wait can synchronize on.

---

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

Stale/incorrect comment: the code emits id=id but the comment describes id % 1. Also note id=id is not a genuine no-op UPDATE in PostgreSQL (it still creates a new heap tuple version, and since id is the primary key the update is not HOT-eligible), so the wide_0 workload measures a PK self-assignment, not an untouched-column update. Fix the comment to match, and consider touching an unindexed column if the intent is a truly HOT-eligible baseline.

---

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

sleep 2 is used as startup synchronization after pg_ctl start. pg_ctl start already blocks until the server is ready by default (it performs its own readiness wait), so this sleep is redundant and, on a loaded machine, potentially insufficient -- the immediately-following setup_schemas psql calls could still race. Drop the sleep and rely on pg_ctl start (optionally pass -w to be explicit).

---

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

This assertion is worthless: it tests nothing beyond what $pre_prune already established. n_hot_indexed (see hot_indexed_stats.c: counts any non-stub normal tuple with HEAP_INDEXED_UPDATED) reflects the live HOT-indexed version, which is present whether or not the opportunistic prune fired. So n_hot_indexed > 0 is trivially true here and would pass even if the prune never happened (and even with the feature reverted, as long as the stats function exists). The stated purpose is to verify "dead members collapse to LP_REDIRECT forwarders" -- but the collapse is precisely what is not asserted. Assert n_chains > 0 (the LP_REDIRECT forwarder count) to actually detect that the chain was pruned/collapsed; otherwise the whole 'survives opportunistic prune' step is a no-op. (high confidence)

💡 Suggested change

Before:

my $post_prune = $node->safe_psql('postgres',
	q{SELECT n_hot_indexed FROM pg_relation_hot_indexed_stats('hi_recov')});
cmp_ok($post_prune, '>', 0,
	'live HOT-indexed version survives opportunistic prune');

After:

my ($post_hi, $post_chains) = split /\|/, $node->safe_psql('postgres',
	q{SELECT n_hot_indexed, n_chains FROM pg_relation_hot_indexed_stats('hi_recov')});
cmp_ok($post_hi, '>', 0,
	'live HOT-indexed version survives opportunistic prune');
cmp_ok($post_chains, '>', 0,
	'dead chain members collapsed to LP_REDIRECT forwarders after prune');

---

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

This comment block is self-contradictory and factually wrong, which is a review blocker for a patch destined for -hackers. It first states a SELECT is "not enough on its own to trigger prune," then relies on exactly a SELECT count(*) seqscan (plus an UPDATE). It also claims prune "fires from an indexscan," yet the query sets enable_indexscan = off and runs a seqscan. In reality the pagemode seqscan path (heap_prepare_pagescan -> heap_page_prune_opt, heapam.c) does call heap_page_prune_opt, so the reasoning here is inaccurate on its own terms. Rewrite to describe the actual trigger (pagemode seqscan and/or heap_update free-space search invoke heap_page_prune_opt). (high confidence)

---

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

The test's value hinges on the crash actually replaying the HOT-indexed UPDATE/prune WAL records, but nothing guarantees or verifies that. With autovacuum off but no other checkpoint control, a background/timed checkpoint occurring during the 5 UPDATEs + prune would advance the redo point and leave little or nothing for recovery to replay, silently degrading this to a no-op recovery test (all assertions would still pass). Consider forcing large checkpoint spacing (e.g. append checkpoint_timeout='1h', max_wal_size large) to make it unlikely a checkpoint intervenes, and add a post-restart guard that the crash actually recovered (e.g. scan the server log for the redo/recovery messages, or assert a wal_consistency_checking mismatch was not logged). As written, a WAL-replay divergence for HOT-indexed records could go undetected if redo replayed nothing. (moderate confidence)

---

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

These convergence assertions do not exercise the new code: plain logical replication also makes the subscriber match the publisher, so both is() checks pass even if hot_indexed_on_apply = always silently demoted to a normal (non-HOT-indexed) update and left no stale leaves. This is a "passes with the feature reverted" test for its central claim ("the RI lookups found the right rows across the HOT-indexed chains").

The sibling test 039_hot_indexed_apply.pl already proves the path fired by polling n_tup_hot_indexed_upd and asserting cmp_ok($ri_hotidx, '>', 0). Add the equivalent guard here before the convergence checks, e.g. assert that pg_stat_user_tables.n_tup_hot_indexed_upd for tab_idx (and tab_full) increased. Otherwise this file adds little regression protection beyond 039. (high confidence)

---

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

Missing teardown: the subscription sub is never dropped before $subscriber->stop. The sibling tests (037_except.pl, 039_hot_indexed_apply.pl) all DROP SUBSCRIPTION before stopping so the replication slot on the publisher is released cleanly and the shutdown is quiet. Add $subscriber->safe_psql('postgres', 'DROP SUBSCRIPTION sub'); before stopping the nodes to match the established pattern. (moderate confidence)

💡 Suggested change

Before:

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

After:

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

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

---

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

These two sub-cases (off and subset_only) are vacuous: they cannot catch a regression in the stale-leaf recheck they claim to test. For tab_uk the publisher UPDATE sets payload=999, changing an indexed attribute (part of UNIQUE(payload,tag)) that is NOT covered by the PK. Per HeapUpdateHotAllowable() in heapam.c, both 'off' (!bms_equal(all_idx_attrs, pk_attrs)) and 'subset_only' (!bms_is_subset(all_idx_attrs, pk_attrs)) force HEAP_UPDATE_ALL_INDEXES, i.e. a plain non-HOT update that maintains the unique index normally. No stale leaf is ever created, so the subsequent INSERT succeeds trivially and _bt_check_unique's recheck path is never exercised. Only 'always' actually produces the stale leaf. The loop comment ("Under all three modes _bt_check_unique's recheck ... must recognize the stale leaf entry") is therefore factually wrong for off/subset_only, and these assertions would still pass with the recheck logic reverted. Either restrict this block to 'always', or add a per-mode assertion (via n_tup_hot_indexed_upd or verify_heapam) that a stale leaf was actually created before the INSERT, so the off/subset_only iterations are not silently no-ops.

---

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

On timeout poll_counters neither warns nor dies; it silently returns the last-read (possibly pre-update) counters, so a slow apply/pgstat flush on a loaded buildfarm animal yields wrong deltas that later fail cmp_ok/is with a misleading message instead of pointing at the real cause (pgstat not flushed yet). The established idiom in sibling subscription tests (e.g. 026_stats.pl) is poll_query_until(...) or die "Timed out ...". Recommend polling with poll_query_until (die on timeout) so a genuine timeout is reported as such rather than masquerading as a counter-value mismatch.

---

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

"Amit's corner" is an informal mailing-list-style attribution to a person, not a durable code comment. PostgreSQL comments should explain the invariant/rationale, not attribute it to a persona. Drop the attribution and keep the substantive explanation, e.g. start the sentence at "Under hot_indexed_on_apply = 'always' ...".

💡 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

[github-actions]

This test only asserts the absence of corruption (0 rows), but never confirms the HOT-indexed artifacts it claims to exercise were actually produced. If a future change disables the SIU path, or the page geometry causes these UPDATEs to go non-HOT (fillfactor 70 with 200 rows leaves little free space; once a page fills, subsequent updates on that page migrate off-page and are ordinary non-HOT/HOT updates, not HOT-indexed), this test still passes with 0 rows while exercising none of the LP_REDIRECT / stub / HEAP_INDEXED_UPDATED handling the comment describes. That is a silently-vacuous test: it cannot distinguish "verify_heapam handled the artifacts correctly" from "no artifacts were created". Add a positive precondition assertion (e.g. pg_stat_get_tuples_hot_indexed_updated() > 0, or pg_relation_hot_indexed_stats() showing n_hot_indexed / n_chains > 0 after the VACUUM) so the test fails loudly if the SIU path stops firing. (moderate confidence)

[github-actions]

The comment asserts VACUUM "collapses dead members to LP_REDIRECT forwarders", but VACUUM here runs against a heap whose only preceding UPDATE traffic is from earlier statements in the same session's autocommit transactions. Whether the three c2 UPDATEs on id<=50 actually build a dead intermediate chain that prune collapses depends on visibility (OldestXmin) at VACUUM time and on all three prior UPDATEs being HOT-indexed on-page. Nothing here verifies a redirect/stub was actually created, so the strong claim in the comment ("exercises chain collapse") is unverifiable from the test and may be aspirational. Either verify the redirect/stub count (pg_relation_hot_indexed_stats n_redirects/n_chains) or soften the comment to describe intent rather than asserting the outcome. (moderate confidence)

[github-actions]

The ItemPointerIsSIUMaybeStale() guard here is redundant: ItemPointerOffsetNumberStrip() is already a no-op when the marker bit isn't set (it just masks bit 14, and sentinels pass through unchanged per itemptr.h). The strip can be applied unconditionally, which reads more simply and matches the sibling code in contrib/pageinspect/btreefuncs.c that strips without a prior ItemPointerIsSIUMaybeStale() test. Low confidence this matters for correctness (it doesn't); purely a readability/consistency nit.

Suggested change

	if (ItemPointerIsSIUMaybeStale(&logtuple->t_tid))
	logtuple->t_tid.ip_posid =
	ItemPointerOffsetNumberStrip(logtuple->t_tid.ip_posid);
	logtuple->t_tid.ip_posid =
	ItemPointerOffsetNumberStrip(logtuple->t_tid.ip_posid);

[github-actions]

This test never verifies that the UPDATE actually took the HOT-indexed (SIU) path, so it can pass without exercising the behavior it claims to guard. If the update falls back to a plain HOT or non-HOT update (e.g. page layout changes, or future planner/AM changes), the bloom entry would point at the new tuple / the row would still be found, and this query returns 1 regardless -- a silent false pass. The established pattern for this feature (contrib/pageinspect/sql/hot_indexed_updates.sql) always asserts the path fired via pg_stat_get_tuples_hot_indexed_updated()/get_hi_count(). Add such an assertion here (e.g. SELECT pg_stat_force_next_flush(); then check the HOT-indexed counter advanced) before the correctness query. Confidence: high.

[github-actions]

The comment centers on a BitmapAnd, but nothing pins the plan. With enable_indexscan off and two eligible bitmap-index-scan candidates the planner should produce a BitmapAnd, but that is not guaranteed (costs can make it pick a single BitmapIndexScan with the other qual as a recheck/filter). If it does, the row is still returned and count is 1, so the specific BitmapAnd/tbm intersection path the comment describes goes untested. Add EXPLAIN (COSTS OFF) for this query to lock the BitmapAnd plan, matching the discipline in hot_indexed_updates.sql. Confidence: moderate.

[github-actions]

This Assert will crash a debug (cassert) build on exactly the corrupt-page input the following runtime clamp is designed to tolerate. Since bmnatts here comes from a stub's stashed natts in t_ctid.ip_blkid (attacker/corruption-controlled on-disk data, not a can't-happen invariant), an Assert is the wrong tool per PostgreSQL conventions (Assert is for can't-happen invariants, never data-reachable conditions). A corrupt page walked under a cassert build would PANIC rather than being tolerated. Consider dropping the Assert and keeping only the runtime clamp, or downgrading to an elog/report path. Confidence: moderate.

Suggested change

	Assert(bmnatts >= 0 && bmnatts <= relnatts);
	if (bmnatts < 0 || bmnatts > relnatts)
	bmnatts = relnatts;
	if (bmnatts < 0 || bmnatts > relnatts)
	bmnatts = relnatts;

[github-actions]

Bug: this fast path leaves the tuple bloated. A HOT-indexed source tuple has t_len == original_len + HotIndexedBitmapBytes(natts) (see heap_form_hot_indexed_tuple in heapam.c, which grows t_len by the trailing bitmap). heap_copytuple copies t_len bytes verbatim, and rewriteheap.c stores the tuple using heaptup->t_len (MAXALIGN reservation and PageAddItem). Clearing only the HEAP_INDEXED_UPDATED bit does NOT reduce t_len, so every flattened HOT-indexed tuple is persisted into the rewritten heap carrying HotIndexedBitmapBytes(natts) of dead trailing storage counted inside lp_len -- permanent per-tuple bloat that no reader will ever interpret. This is the wrong place to just flip a bit: force the reform path (heap_deform_tuple stops at natts and ignores the trailing bitmap, heap_form_tuple then produces a canonical tuple with correct length and no flag). Set needs_reform = true when the source has HEAP_INDEXED_UPDATED set instead of copying-and-clearing.

[github-actions]

Editing artifact: the parenthetical (hot-indexed) after HOT-indexed is redundant self-reference and reads as a leftover from a rename. Drop it.

Suggested change

	* If we reached the visible tuple through a HOT-indexed
	* (hot-indexed) hop, the bitmap index entry that pointed us
	* If we reached the visible tuple through a HOT-indexed
	* hop, the bitmap index entry that pointed us

[github-actions]

This function interprets raw page bytes as heap ItemId/HeapTupleHeader (LP_REDIRECT walk, HEAP_HOT_UPDATED, HEAP_INDEXED_UPDATED, HotIndexedHeaderIsStub), but it only checks relkind and never verifies the table access method is heap. A RELKIND_RELATION/RELKIND_MATVIEW relation can use a non-heap TAM, whose pages have an arbitrary layout; casting those bytes to HeapTupleHeader yields garbage or crashes. Every comparable heap-page inspector in the tree guards with relam != HEAP_TABLE_AM_OID (verify_heapam.c, pageinspect/heapfuncs.c, pg_surgery/heap_surgery.c, pgrowlocks.c, pgstatapprox.c, pgstattuple.c). Add the same guard here. (high confidence)

Suggested change

	if (rel->rd_rel->relkind != RELKIND_RELATION &&
	rel->rd_rel->relkind != RELKIND_MATVIEW &&
	rel->rd_rel->relkind != RELKIND_TOASTVALUE)
	ereport(ERROR,
	(errcode(ERRCODE_WRONG_OBJECT_TYPE),
	errmsg("\"%s\" is not a table, materialized view, or TOAST table",
	RelationGetRelationName(rel))));
	if (rel->rd_rel->relkind != RELKIND_RELATION &&
	rel->rd_rel->relkind != RELKIND_MATVIEW &&
	rel->rd_rel->relkind != RELKIND_TOASTVALUE)
	ereport(ERROR,
	(errcode(ERRCODE_WRONG_OBJECT_TYPE),
	errmsg("\"%s\" is not a table, materialized view, or TOAST table",
	RelationGetRelationName(rel))));
	if (rel->rd_rel->relam != HEAP_TABLE_AM_OID)
	ereport(ERROR,
	(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
	errmsg("only heap AM is supported")));

[github-actions]

This introduces two consecutive blank lines here (and a further double blank after the function, before the heap_prune_record_unchanged_lp_unused comment). PostgreSQL requires a single blank line between functions and git diff --check/pgindent must be clean; consecutive blank lines are whitespace churn that reviewers reject. Collapse each run to a single blank line.

Suggested change

	/*
	* heap_prune_item_preserves_hot_indexed
	+
	+/*
	+ * heap_prune_item_preserves_hot_indexed