Tepid: only update indexes that have changed attributes

[gburd]

No description provided.

[github-actions]

🔍 OCR found 81 issue(s).

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

---

📄 src/test/recovery/t/055_hot_indexed_recovery.pl

Duplicate test number: t/055_cascade_reconnect.pl already exists (and is listed just above this one in src/test/recovery/meson.build). TAP test files use unique numeric prefixes; two 055_* files is confusing and breaks the convention that the number orders the suite. The next free number in this directory is 061 (056-060 are taken). Rename this file to 061_hot_indexed_recovery.pl and update meson.build accordingly. Also note the Makefile change pulls in amcheck/pageinspect/pg_buffercache for the whole recovery suite; that is fine, but confirm every consumer skips cleanly if contrib is unavailable.

---

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

[moderate confidence] Behavior broadening for summarizing indexes on classic-HOT catalog updates. Previously a pure HOT catalog update that changed no summarized column reported TU_None and skipped all index inserts; TU_Summarizing was returned only when a summarized attribute actually changed. Now update_all_indexes = !HeapTupleIsHeapOnly(tup) is false for every HOT update, so index_unchanged = true and summarizing indexes always receive an aminsert attempt even when no summarized column changed. For catalogs this is practically harmless (no summarizing indexes exist on them), but the contract shift is worth confirming is intended, since it drops the ability to skip summarizing-index maintenance when nothing summarized changed.

---

📄 src/include/access/tableam.h (L1463-L1464)

Gratuitous whitespace churn: this inserts extra blank lines (resulting in three consecutive blanks before the "non-modifying operations" banner) with no functional purpose. Remove to keep the diff minimal.

---

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

Unrelated whitespace change: this deletes a pre-existing blank line between the IndexInfo typedef and the amproperty comment that the feature does not require. Minimal-diff discipline: avoid touching untouched lines.

---

📄 src/include/access/tableam.h (L1044-L1045)

NULL-deref footgun: RelationGetLocatorDesc() dereferences rel->rd_tableam->relation_locator without checking rel->rd_tableam != NULL. For relkinds where !RELKIND_HAS_TABLE_AM() (views, partitioned tables, foreign tables, composite types) rd_tableam is NULL (see relcache.c RelationInitTableAccessMethod gating), so this crashes. The sibling helper RelationUpdatesInPlace() added in this same patch deliberately guards rel->rd_tableam != NULL first, making the asymmetry a trap for future callers. No current in-tree caller reaches this with a NULL AM, but the inline is a public API; either add the same guard (returning the default descriptor / asserting), or document the non-NULL rd_tableam precondition explicitly. (moderate confidence)

---

📄 src/include/access/amlocator.h (L73-L77)

The documented negative-width convention has no consumer. The only reader of width is index_check_locator() in catalog/index.c, which does if (locdesc->width > 0 && locdesc->width <= sizeof(ItemPointerData)) return; and otherwise routes to the amcanvarlocator check. The magnitude of a negative width is never read anywhere (no buffer sizing, no bound check uses it). The variable-width AM in the test module (wide_locator) even sets a positive .width = 16, not a negative value. As written, this is speculative API surface (YAGNI): a variable-width AM can simply set width <= 0 and the sign/magnitude semantics documented here are inert. Either wire a consumer that actually uses the magnitude bound, or simplify the contract to "positive = exact fixed width; non-positive = variable-width, handled via amcanvarlocator" and drop the "magnitude bounds the width" claim. Confidence: high.

---

📄 src/backend/access/heap/heapam_indexscan.c (L20-L22)

Duplicate #include "access/hot_indexed.h" (also included two lines above at the top of the access/* block). Besides being redundant, the second copy is out of alphabetical order (it sits after access/relscan.h). Remove the duplicate; the first include already covers it.

💡 Suggested change

Before:

#include "access/relscan.h"
#include "access/hot_indexed.h"
#include "access/sysattr.h"

After:

#include "access/relscan.h"
#include "access/sysattr.h"

---

📄 src/backend/access/heap/heapam_handler.c (L22-L23)

Include is out of alphabetical order: access/amlocator.h should precede access/genam.h (a < g). The project keeps the access/* group alphabetized; reorder to avoid a style nit on the list.

💡 Suggested change

Before:

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

After:

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

---

📄 src/include/access/tableam.h (L23-L23)

Unnecessary #include. Nothing in tableam.h uses a symbol from sysattr.h; the FirstLowInvalidHeapAttributeNumber convention is referenced only in comments. Moreover sysattr.h is already pulled in transitively via executor/tuptable.h (which this header includes). Drop this added include to keep the diff minimal and avoid adding dependencies to a very widely-included core header.

---

📄 src/include/access/tableam.h (L537-L538)

Not pgindent-clean: the continuation lines of this new callback are indented with plain tabs and do not align under the opening parenthesis, unlike the sibling fetch_tid callback a few lines below (whose continuations align under 'Relation rel'). pgindent will re-wrap this; run pgindent before posting.

---

📄 src/include/access/tableam.h (L543-L547)

Double blank line here (two blanks between the callback and the banner). A single blank line is enough; drop one to avoid gratuitous whitespace churn.

💡 Suggested change

Before:

								TupleTableSlot *slot);


	/* ------------------------------------------------------------------------
	 * Callbacks for non-modifying operations on individual tuples

After:

								TupleTableSlot *slot);

	/* ------------------------------------------------------------------------
	 * Callbacks for non-modifying operations on individual tuples

---

📄 src/backend/replication/logical/worker.c (L6198-L6203)

Contradictory wording about caching. This function's leading comment (and the header comment in logicalworker.h) describes the result as the "cached" HOT-indexed apply mode, while the inner comment states it derives the value "directly from MySubscription rather than caching". Both are talking about the same value: MySubscription->hotindexedonapply is itself the cached copy of the catalog column, and "rather than caching" only means no second module-level cache variable is added. As written, the two comments read as a direct contradiction. Reword one of them to disambiguate (e.g. keep "cached" for the struct field, and change the inner comment to "read directly from MySubscription so there is no second cache variable to keep in sync").

---

📄 src/backend/utils/cache/relcache.c (L5434-L5435)

Memory leak on the cache-miss path. TextDatumGetCString(datum) and the stringToNode() output tree are palloc'd in the caller's current memory context and never freed. This diverges from the established pattern in this very file (RelationGetDummyIndexExpressions does exprsString = TextDatumGetCString(...); rawExprs = stringToNode(exprsString); pfree(exprsString);). On the first call per relcache lifetime for an expression/partial index this leaks the CString plus the full parsed node tree into whatever context the caller is running in. For executor callers (ExecInsertIndexTuples) that context is reset per-query, so it is bounded, but if the first call lands in a long-lived context the garbage accumulates. Capture the CString in a local and pfree it, and consider freeing the parsed tree too, as the dummy-expressions helper does.

💡 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);

---

📄 src/backend/utils/cache/relcache.c (L5442-L5443)

Same leak as above for the predicate branch: the CString from TextDatumGetCString and the stringToNode output are not freed on the cache-miss path.

💡 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);

---

📄 src/backend/utils/cache/relcache.c (L5744-L5745)

Comment drift: there is no all_summarizing check (nor any identifier by that name) in HeapUpdateHotAllowable. That function implements the equivalent logic inline as bms_is_subset(modified_idx_attrs, RelationGetIndexAttrBitmapNoCopy(relation, INDEX_ATTR_BITMAP_SUMMARIZED)). Referencing a named check that does not exist will mislead readers; describe the actual subset test instead.

💡 Suggested change

Before:

	 * block summary refreshed.  HeapUpdateHotAllowable's all_summarizing
	 * check still keeps such an update on the classic-HOT path (it stays

After:

	 * block summary refreshed.  HeapUpdateHotAllowable's summarizing-only
	 * subset check still keeps such an update on the classic-HOT path (it stays

---

📄 src/backend/commands/trigger.c (L4440-L4448)

The images metadata array keeps growing for every saved image even after spilling to the temp file: once image_file != NULL, only the tuple bytes go to disk, but img = &afterTriggers.images[afterTriggers.nimages] and the nimages >= maximages doubling here still run per image, so maximages/nimages are unbounded. For a large bulk UPDATE this defeats the purpose of the spill: the array alone grows at sizeof(AfterTriggerImage) (~24 bytes) per row and is held in memory until transaction end.

Two concrete consequences:

1. A multi-billion-row UPDATE can collide the returned index with the AFTER_TRIGGER_NO_IMAGE == PG_UINT32_MAX sentinel (silent corruption), and consumes tens of GB of RAM for metadata that the spill was supposed to avoid.
2. maximages * sizeof(AfterTriggerImage) is a uint32 * size_t product; on a 32-bit build size_t is 32 bits, so an unbounded maximages can overflow the size argument to repalloc_huge/MemoryContextAllocHuge, under-allocating the array.

At minimum, guard against the sentinel/overflow boundary (error out, or document and bound the count). Ideally the spill-to-file path should not retain a per-image in-memory record so that memory stays bounded by work_mem.

---

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

This #include "utils/datum.h" is unused: no datum.h symbol (datumIsEqual/datumCopy/datum_image_eq/datumGetSize/etc.) appears anywhere in execTuples.c. This is a minimal-diff violation; drop the include.

---

📄 src/backend/executor/nodeModifyTable.c (L2318-L2320)

Memory leak on concurrent-update retry. modified_attrs is allocated at line 2444 (ExecUpdateModifiedIdxAttrs) before table_tuple_update, so it is always set when the result is not TM_Ok. On TM_Updated the caller does goto redo_act, which re-enters ExecUpdateAct and resets modified_attrs = NULL here, orphaning the prior Bitmapset. It is palloc'd via RelationGetIndexAttrBitmap in the per-query context (ExecUpdateEpilogue's own comment notes this and frees it only on the TM_Ok path), so each retry leaks one Bitmapset for the statement's lifetime. The MERGE lmerge_matched retry path has the same issue. Free the previous allocation before overwriting (e.g. bms_free here, or free on all non-TM_Ok exits).

💡 Suggested change

Before:

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

After:

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

---

📄 src/backend/utils/cache/relcache.c (L5347-L5351)

This header comment is inaccurate on two points and will mislead future readers:

1. It states the bitmap is built from RelationGetIndexExpressions/RelationGetIndexPredicate, but the implementation deliberately does NOT call those (see the inline comment below: it parses the raw stored trees via stringToNode precisely to avoid eval_const_expressions).

2. It states a copy is cached "in rd_indexedattr". The cache field is rd_indattr (set at line 5455). rd_indexedattr is an unrelated field on the heap relation used by RelationGetIndexAttrBitmap.

Please fix both so the comment describes what the code actually does.

---

📄 src/backend/utils/cache/relcache.c (L5826-L5829)

"never on the bench hot path" reads as informal jargon (benchmark). In a committed header comment this is confusing; state the actual rationale, e.g. "The first-use path is rare and not on any hot path".

---

📄 src/backend/executor/nodeModifyTable.c (L228-L229)

Doc inaccuracy: the parameters are old_tts/new_tts (TupleTableSlots), but this line refers to oldtup/newtup, which do not exist in this function. Use the actual parameter names so the comment matches the signature.

💡 Suggested change

Before:

 * Return a Bitmapset that contains the set of modified (changed) indexed
 * attributes between oldtup and newtup.

After:

 * Return a Bitmapset that contains the set of modified (changed) indexed
 * attributes between old_tts and new_tts.

---

📄 src/backend/executor/execIndexing.c (L1182-L1186)

Behavior regression for expression indexes vs. the removed index_unchanged_by_update(): the old code walked the expression tree (index_expression_changed_walker) and kept ii_IndexUnchanged=true when none of the expression's referenced columns changed. This new code unconditionally sets keychanged=true for any expression key, so an expression index never gets the indexUnchanged hint even when its inputs are unchanged. That silently disables bottom-up deletion / dedup for all expression indexes on non-HOT (row_moved) updates. If the conservatism is intentional, the comment should state why the walker can't be reused; otherwise consider retaining the expression-tree check to preserve the hint.

---

📄 src/backend/executor/nodeIndexonlyscan.c (L135-L137)

The parenthetical cites functions that don't exist in this tree and misstates where the flag is managed (medium / high confidence). There is no index_getnext_tid here (the TID advance is tableam_index_getnext_tid, and it does not reset xs_entry_needs_recheck per entry), and the flag is not set in any index_fetch_heap. The actual per-entry reset-to-false and the set happen in the heap table-AM getnext path (heapam_indexscan.c: reset at the scan->xs_entry_needs_recheck = false; before the fetch, set via heapam_index_entry_needs_recheck()). The behavioral conclusion (no staleness check needed on the VM-all-visible path, since the fetch is skipped and the flag stays false) is correct, but the explanation names wrong/nonexistent functions. Fix the reference so the comment describes the code as it actually exists.

💡 Suggested change

Before:

		 * all-visible entry never crossed a HOT/SIU hop.  (index_getnext_tid
		 * also resets xs_entry_needs_recheck per entry, and only the heap fetch
		 * in index_fetch_heap ever sets it, so it cannot be set here anyway.)

After:

		 * all-visible entry never crossed a HOT/SIU hop.  (The heap table AM's
		 * index getnext path resets xs_entry_needs_recheck to false before each
		 * fetch and only sets it from the fetched tuple, so it cannot be set on
		 * the all-visible path that skips the fetch.)

---

📄 src/include/access/relscan.h (L124-L124)

Unrelated whitespace churn: this inserts a second consecutive blank line before struct IndexScanInstrumentation;, which is not needed by the two new fields this patch adds. Per minimal-diff discipline, drop this hunk so the patch touches only lines required for the change. (low confidence it's intentional; git diff --check/pgindent will not catch an extra blank line.)

💡 Suggested change

Before:

+
 struct IndexScanInstrumentation;

After:

 struct IndexScanInstrumentation;

---

📄 src/include/access/relscan.h (L202-L202)

This comment names a function table_index_entry_needs_recheck() that does not exist anywhere in the tree. The verdict carried by xs_entry_needs_recheck is actually produced by the heap-specific static heapam_index_entry_needs_recheck() (via the xs_getnext_slot/table_index_getnext_slot scan path), and the analogous constraint-enforcement path is table_index_fetch_tuple_check(). Referencing a non-existent table-AM entry point is misleading; cite the real function(s) or drop the specific name. (moderate confidence)

💡 Suggested change

Before:

	 * produced by the table AM via table_index_entry_needs_recheck(); the

After:

	 * produced by the table AM (for heap, via the xs_getnext_slot callback
	 * reached by table_index_getnext_slot()); the

---

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

Do not bump CATALOG_VERSION_NO in a patch. The catversion bump is the committer's job at push time. Including it in the PR causes needless merge conflicts (every rebase re-conflicts on this line) and is the single most common author mistake in catalog patches. Drop this hunk; the committer will bump catversion when the catalog changes land. (high confidence)

---

📄 src/backend/executor/nodeIndexscan.c (L297-L298)

The comment misattributes where xs_entry_needs_recheck is set. The flag is reset/set only by the heap table AM in src/backend/access/heap/heapam_indexscan.c (based on the per-hop crossed-attrs bitmaps from heap_hot_search_buffer); genam.c only initializes it to false, and src/backend/access/index/indexam.c never touches it. Saying "indexam.c sets it" is wrong and will mislead readers grepping for the source. (This is the same class of comment-accuracy defect flagged in nodeIndexonlyscan.c, but a distinct line.) Reword to point at the heap AM, matching the accurate IndexNext comment above ("decided by the heap AM ... see heap_hot_search_buffer").

---

📄 src/backend/nodes/tidbitmap.c (L43-L45)

tidbitmap.c now uses Buffer, BufferIsValid, InvalidBuffer, and ReleaseBuffer, but relies on these being pulled in transitively via access/tableam.h -> storage/read_stream.h -> storage/bufmgr.h. Per project convention, include the header for symbols you use directly; add #include "storage/bufmgr.h" so the dependency is explicit and does not break if the transitive include chain changes.

---

📄 src/bin/pg_upgrade/file.c (L365-L365)

A read error is misreported as a partial page. If read() returns -1 (e.g. EIO, or EINTR on a signal), the while condition != BLCKSZ exits the loop, the final destination page may be flushed from stale data, and then bytesRead != 0 (i.e. -1 != 0) triggers the "partial page found" pg_fatal -- masking the real I/O error and giving a misleading diagnostic. The sibling copyFile() in this same file explicitly checks nbytes < 0 and reports "could not read file". Add the same handling here.

Example:

while ((bytesRead = read(src_fd, src_buf.data, BLCKSZ)) == BLCKSZ)
{
	...
}
if (bytesRead < 0)
	pg_fatal("error while copying relation \"%s.%s\": could not read file \"%s\": %m",
			 nspname, relname, fromfile);

Confidence: high.

---

📄 src/bin/pg_upgrade/file.c (L384-L386)

For a multi-segment VM fork (segno > 0), transfer_relfile() calls rewriteVisibilityMap() once per segment, and new_vmblk restarts at 0 here each call. The backend checksums VM pages with the fork-absolute block number (pg_checksum_page() XORs blkno in to detect transposed pages; md.c passes the absolute blocknum before segment math). So segment >0 pages would be written with checksums computed against the wrong (segment-relative) block number, producing invalid checksums when data_checksum_version != 0. In practice a 1GB VM segment covers an impossibly large heap, so this is a latent/unreachable corner (and mirrors the historical 9.6 rewrite), but it is worth a comment noting the first-segment-only assumption. Confidence: low.

---

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

This whole directory is developer-only benchmark tooling, not reproducible tests. Confirmed it is not wired into any Makefile/meson schedule, and the absolute paths it hardcodes (/scratch/siu-bench, $HOME/ws/postgres/tepid, /scratch/pg) appear nowhere else in the tree, so this is not an existing convention. Per PostgreSQL patch hygiene, shipping un-wired benchmark scaffolding that bakes in machine-specific paths, personal workspace references, build-layout assumptions (usr/local/pgsql, lib64/lib), and fork branch names (tepid, origin/master) is scope creep that -hackers will reject; it also leaks author environment details into the patch. Either drop these scripts from the patch or move them out of src/test/ and strip the environment-specific defaults. [high confidence]

---

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

siu_count() swallows errors with 2>/dev/null and coerces any non-numeric result to 0. I confirmed the pgstat column n_tup_hot_indexed_upd does exist (system_views.sql:733) and that n_tup_hot_upd includes the HOT-indexed subset (pgstat_relation.c:466-473, "counted in addition to tuples_hot"), so the arithmetic classic_hot=hot-siu is well-defined only when this query succeeds. On the tepid build a genuinely broken counter query is indistinguishable from "feature absent on master": siu silently becomes 0, so every HOT-indexed update is misattributed as classic HOT (classic_hot = hot - 0 = hot). That defeats the harness's own goal and silently masks regressions. Fail loudly on the tepid arm instead of coercing to 0. [high confidence]

---

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

find "$datadir" -mindepth 1 -delete uses GNU-find-only syntax (-mindepth/-delete); it is not portable to BSD/macOS base find, contradicting the portability the siu scripts imply. Same construct appears in soak.sh. Use rm -rf "$datadir" as split_ab.sh already does. [moderate confidence]

---

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

sed -i.bak and grep -oP (PCRE) are GNU-only. On BSD/macOS, sed -i takes a mandatory separate argument and lacks GNU -i.bak semantics, and base grep has no -P. The split_off build and the tps/lat parsing (lines 130-131) will silently fail on non-Linux, producing a meaningless A/B delta. Either document a hard Linux-only requirement or use portable sed/awk. [moderate confidence]

---

📄 src/test/benchmarks/siu/scripts/split_ab.sh (L73-L75)

Footgun: under set -euo pipefail, if meson compile (or setup/install) fails during the split_off build, the script aborts before the .bak restore below, and the EXIT trap only runs git checkout --quiet $orig (line 54) which will NOT discard the uncommitted sed edit. The working tree is left with .bitmap_and_inexact = NULL injected into heapam_handler.c and a stray .bak file. Restore the file from the trap (e.g. add the mv of the .bak back, or git checkout -- src/backend/access/heap/heapam_handler.c, to the EXIT trap) so a mid-build failure cannot leave the tree corrupted. [high confidence]

---

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

n_tup_hot_indexed_upd_skipped is confirmed to exist, but as a per-index column of pg_stat_all_indexes (system_views.sql:874), which this query handles correctly via WHERE indexrelname='${tbl}_b'. However, like run.sh's siu_count, this silently returns 0 via coalesce(...,0) on a master build where the column is absent, making a real query failure indistinguishable from "feature absent". Given this script is tepid-only (it exercises the HOT-indexed skip path), prefer failing loudly if the column is missing rather than reporting a misleading 0 skip count. [moderate confidence]

---

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

This core src/bin/pg_upgrade TAP test takes a hard dependency on the contrib amcheck extension (CREATE EXTENSION amcheck + verify_heapam/bt_index_check). Unlike src/bin/pg_amcheck (which depends on amcheck by construction), pg_upgrade has no such dependency, and contrib is not guaranteed to be built/installed when these tests run (e.g. a src/bin-only make check/meson run). If amcheck is absent, CREATE EXTENSION fails and the whole test errors out rather than skipping. Gate the test on amcheck availability (skip_all when not installed), or move the amcheck-dependent coverage to a test under a module that already depends on amcheck. Confidence: moderate.

---

📄 src/bin/pg_upgrade/pg_upgrade.h (L447-L450)

Prototype misplaced: rewriteVisibilityMap() is defined in file.c, but it is declared here under the /* relfilenumber.c */ section. pg_upgrade.h groups prototypes by their defining source file; move this declaration under the /* file.c */ section (alongside cloneFile/copyFile/etc.) so the grouping stays accurate. Confidence: high.

---

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

The two core SIU workloads reach run_one via the default *) branch, which runs pgbench WITHOUT -D scale=$SCALE. Both hot_indexed_update.sql and hot_indexed_mixed.sql reference :scale (\set aid random(1, :scale * 100000)), so pgbench aborts every client with an undefined-variable error. tps then comes back empty and is coerced to NA, i.e. the two primary workloads this harness exists to measure produce no data. The wide_* and read_indexscan branches correctly pass -D; the default branch must too. [high confidence]

💡 Suggested change

Before:

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

After:

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

---

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

wide_0 is meant to be the "no indexed column changed" baseline, but it emits SET id=id, and id is the table's PRIMARY KEY (CREATE TABLE wide_table(id int PRIMARY KEY ...)), i.e. an indexed column. The comment "touch a non-indexed column (id % 1 so it's a no-op)" is wrong on two counts: id is indexed, and the clause is id=id, not id % 1. Every c1..cN column is also indexed, so this schema has no non-indexed column to touch at all. The baseline arm therefore does not measure what it claims. [moderate confidence]

---

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

Trailing whitespace after the closing quote; git diff --check (a PostgreSQL hard gate) will flag this. [high confidence]

💡 Suggested change

Before:

  local seedopt="--random-seed=$seed" 

After:

  local seedopt="--random-seed=$seed"

---

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

This test uses old and new clusters built from the same binaries, so old_cluster.controldata.cat_ver is always >= VISIBILITYMAP_WIDTH_CHANGE_CAT_VER and the new rewriteVisibilityMap() path in relfilenumber.c is never taken (the _vm fork is copied/linked verbatim). The 2->4 bit VM rewrite -- the main C logic added by this patch -- therefore has no test coverage here, including its per-block packing arithmetic, checksum recompute, and multi-segment handling. Consider adding coverage that actually drives the rewrite branch (e.g. a fixture with a pre-widening _vm fork), or at least document why the rewrite path is left untested.

---

📄 src/test/modules/test_locator_tableam/test_locator_tableam.c (L145-L147)

The offset bound check only tests the upper bound. heap's own update-chain walker (heap_get_latest_tid) guards both ends: offnum < FirstOffsetNumber || offnum > PageGetMaxOffsetNumber(page). Here, if a followed t_ctid ever carries offset 0 (InvalidOffsetNumber), PageGetItemId(page, 0) dereferences pd_linp[-1] -- an out-of-bounds read. The initial TID is valid, but cur = htup->t_ctid is followed without validating its offset, so a malformed/corrupted ctid chain reaches this. Match the canonical idiom and check the lower bound too.

💡 Suggested change

Before:

		if (ItemPointerGetOffsetNumber(&cur) <= PageGetMaxOffsetNumber(page))
		{
			lp = PageGetItemId(page, ItemPointerGetOffsetNumber(&cur));

After:

		if (ItemPointerGetOffsetNumber(&cur) >= FirstOffsetNumber &&
			ItemPointerGetOffsetNumber(&cur) <= PageGetMaxOffsetNumber(page))
		{
			lp = PageGetItemId(page, ItemPointerGetOffsetNumber(&cur));

---

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

This execl -> execlp change is unrelated to the stated purpose of the patch (HOT indexed updates / table AM locator) and should be dropped to keep the diff minimal. shellprog is SHELLPROG, defined as $(SHELL) (GNUmakefile) or /bin/sh (meson) -- both absolute paths -- so the PATH lookup that execlp adds can never trigger and the behavior is identical. Bundling unrelated changes (this, plus the .clangd/flake.nix/shell.nix/pg-aliases.sh/.gdbinit dev-environment files elsewhere in the change) is a top rejection reason on -hackers. Confidence: high.

💡 Suggested change

Before:

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

After:

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

---

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

This assertion does not actually verify that the prune/collapse happened. pre_prune already asserted n_hot_indexed > 0, and n_hot_indexed counts the surviving live HOT-indexed version (see hot_indexed_stats.c), which is unaffected by whether dead members collapsed to LP_REDIRECT. So this post-prune check is identical in form to the pre-prune check and passes whether or not opportunistic prune fired. Since the SELECT+UPDATE path only best-effort triggers heap_page_prune_opt (gated by PageIsFull / free-space heuristics), the collapse the comment claims is never verified: the test can pass with the chain uncollapsed, defeating the point of exercising collapsed-chain redo. Assert the collapse directly, e.g. that n_chains (LP_REDIRECT count) increased, or inspect heap_page_items for LP_REDIRECT, rather than re-checking n_hot_indexed > 0.

---

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

This comment is design narration / aspirational reasoning ("that's not enough on its own", "The reliable way to drive opportunistic prune is...") rather than a concise statement of what the test does. Per project comment discipline, describe the current action (what the SELECT+UPDATE achieves) and keep it short. Note also the claim that this reliably forces the prune is not borne out: opportunistic prune is best-effort (see heap_page_prune_opt), which ties into the tautological assertion below.

---

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

The WAL assertion is tightly coupled to the exact rmgr-desc text and to specific line-pointer offsets. The desc format is confirmed (nstubs: N ... stubs:[... a->b ...] from heapdesc.c + redirect_elem_desc's %u->%u), but the literal 2->4 hardcodes a stub re-pointing from offset 2 to 4, which depends on exact on-page placement produced by the preceding UPDATE/VACUUM sequence and could differ across block sizes or minor layout changes. Consider matching the structural shape (stubs:\[[^]]*->) rather than a specific 2->4 pair, or add a page-layout assertion that pins offsets 2 and 4 so a layout change fails loudly instead of silently not matching.

---

📄 src/test/recovery/t/060_hot_indexed_vm.pl (L101-L101)

Comment/code mismatch about which update carries the applied VM full-page image. The SQL updates fresh first, then existing, so in WAL order $updates[0] is fresh's HOT_UPDATE, not existing's. But the comment above says "The existing table's row lock clears all-frozen first and can carry the applied image" and the branch keys the fork vm blk 0 FPW(?! for) (real FPI) check on $update eq $updates[0]. existing is the one that was VACUUM (FREEZE)'d (so it has an all-frozen bit to clear), whereas fresh was never frozen. Either the comment is wrong about which record is $updates[0], or the branch is checking the wrong record and only passes incidentally (e.g. because fresh's VM page is being initialized under fpw=on). Please reconcile the comment with the actual WAL ordering and the intended record.

---

📄 src/test/recovery/t/060_hot_indexed_vm.pl (L148-L151)

The expected VM byte values (4, 52, 51) and page offsets (24, 25) are hardcoded magic numbers derived by hand from BITS_PER_HEAPBLOCK=4, VISIBILITYMAP_LOCATOR_SPLIT=0x04 and a 24-byte page header. The derivation is correct against the current headers (confirmed), but nothing guards it: if BITS_PER_HEAPBLOCK or the SPLIT bit value changes, these assertions break silently with opaque numeric diffs. The comment documents the arithmetic, which helps, but consider deriving the offset from SizeOfPageHeaderData or at least referencing the symbolic constants so a future bit-layout change is traceable.

---

📄 src/test/modules/test_locator_tableam/test_locator_tableam.c (L24-L26)

Comment-accuracy drift. This file defines four table AMs (heap_noretain, heap_wide, heap_bigoffset, heap_orrecheck), and all four install as_heap_index_build_range_scan, yet this sentence says "both AMs present heap's routine". The opening "Both AMs store rows exactly as heap does" (line 6) is stale for the same reason. Per the PostgreSQL comment-accuracy discipline, update these to reflect that all four AMs are involved (e.g. "each AM"/"all four AMs"). Also, line 13 is wrapped well past the ~78-column width used by the rest of the block.

💡 Suggested change

Before:

 * heap_getnext() refuses any relation whose AM routine is not heap's, and
 * heap's index build scan uses it, so both AMs present heap's routine for the
 * duration of that scan.  CREATE INDEX CONCURRENTLY is not supported.

After:

 * heap_getnext() refuses any relation whose AM routine is not heap's, and
 * heap's index build scan uses it, so each AM presents heap's routine for the
 * duration of that scan.  CREATE INDEX CONCURRENTLY is not supported.

---

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

Factual error in the comment: verify_heapam's skip parameter defaults to 'none', not 'all-frozen' (see amcheck--1.2--1.3.sql: skip text default 'none'). The code correctly passes skip := 'none', but the comment misstates the default and is misleading. Suggest dropping the parenthetical or correcting it.

💡 Suggested change

Before:

# verify_heapam reports no errors on the relation (skip_option =
#    'all-frozen' is the default; we want to scan everything).

After:

# verify_heapam reports no errors on the relation.  Pass skip :=
# 'none' explicitly so every page is scanned.

---

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

This file begins with a blank line before the copyright header. Every other recovery TAP file (and 060_hot_indexed_vm.pl in this same change) starts the copyright comment on line 1. Remove the leading blank line for consistency with the project convention.

💡 Suggested change

Before:

# Copyright (c) 2026, PostgreSQL Global Development Group

After:

# Copyright (c) 2026, PostgreSQL Global Development Group

---

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

poll_counters silently returns stale counters when the 10 s wall-clock deadline is hit (the loop just lasts and returns whatever was last read). On a slow/loaded buildfarm member where the apply worker's asynchronous pgstat flush exceeds 10 s, the positive assertions below (cmp_ok($ss_pk_hot, '>', 0), cmp_ok($al_extra_hotidx, '>', 0), cmp_ok($ri_hotidx, '>', 0)) will fail with a misleading message, and a genuine regression cannot be distinguished from a flush delay. wait_for_catchup already runs before the final poll, so apply has completed and only the stats flush is outstanding; the poll should die on timeout so a flush delay surfaces as a clear, actionable timeout rather than a spurious value assertion. (medium confidence)

💡 Suggested change

Before:

		last if ($upd + 0) >= $upd_target || time() >= $deadline;
		usleep(100_000);
	}
	my ($upd, $hot, $hot_idx) = split /\|/, $row;
	return ($upd + 0, $hot + 0, $hot_idx + 0);

After:

		last if ($upd + 0) >= $upd_target;
		die "timed out waiting for n_tup_upd >= $upd_target on $table"
		  if time() >= $deadline;
		usleep(100_000);
	}
	my ($upd, $hot, $hot_idx) = split /\|/, $row;
	return ($upd + 0, $hot + 0, $hot_idx + 0);

---

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

This test never asserts that the apply worker actually took the HOT-indexed path. If HOT-indexed eligibility silently regressed to plain non-HOT apply (e.g. the gating in HeapUpdateHotAllowable broke, or hot_indexed_on_apply was ignored), the convergence checks (is($sub_full, $pub_full ...), is($sub_idx, $pub_idx ...)) and the verify_heapam check would all still pass vacuously -- i.e. the test would pass with the feature reverted, which defeats its purpose. 039 guards this with cmp_ok($ri_hotidx, '>', 0) on n_tup_hot_indexed_upd; add an equivalent positive check here (poll n_tup_hot_indexed_upd on tab_full and tab_idx and assert it advanced) so the test genuinely exercises the new path. (medium confidence)

---

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

This INSERT-after-UPDATE block does not verify which apply path fired, so it passes vacuously if HOT-indexed eligibility regresses. More importantly, for off and subset_only the apply worker demotes to a plain (non-HOT) index-maintaining update, so the payload leaf (0, 'mode_$mode') is rewritten to (999, ...) and no longer exists. The INSERT with payload=0 then has no conflicting entry at all and trivially succeeds without ever reaching the _bt_check_unique stale-leaf recheck the comment at lines 304-308 describes. Only the always iteration exercises that recheck, and even then nothing asserts the HOT-indexed path was taken. As written, this whole block still passes with the feature reverted for all three modes. Add a counter check (e.g. poll_counters n_tup_hot_indexed_upd on tab_uk under always) so the test would catch a regression, and scope the comment to the mode that actually leaves a stale leaf.

---

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

This loop uses copy_data = true, so each iteration's new subscription runs an initial table sync that COPYs the entire publisher tab_uk. But rows inserted/replicated in a prior iteration are never removed from the subscriber (DROP SUBSCRIPTION does not truncate the table), and the publisher retains all previously-inserted rows too. So iteration 2's COPY tries to insert id=2 (seeded in iteration 1) again, hitting the id PRIMARY KEY -> duplicate-key violation in the tablesync worker. That tablesync will error and retry in a loop while wait_for_catchup (which only tracks the main apply LSN, not tablesync completion) still returns, leaving the subscriber in an inconsistent/nondeterministic state and polluting logs. Use copy_data = false here (the rows are seeded on both sides as in the apply-path section) or clean up the table between iterations, or use wait_for_subscription_sync to actually surface the failure.

[github-actions]

This file is personal developer tooling and must not be part of a pgsql-hackers patch for the HOT-indexed-updates feature. It also hardcodes dozens of source line-number breakpoints that are already stale/misleading: e.g. the comment at heapam.c:4019 claims "pure HOT (no indexed columns changed)", but line 4019 is actually inside an xl_heap_lock WAL block (xlrec.offnum = ...); heapam.c:4033 is annotated as "size+space check before creating augmented tuple" but is really if (cleared_all_frozen) PageSetLSN(...). Any edit to these files invalidates every numeric breakpoint and the behavioral comments drift from reality. This belongs in a local worktree/gitignore, not the committed tree.

[github-actions]

This shebang change is unrelated to the HOT-indexed-updates feature and should be dropped from the patch (minimal-diff discipline). pgindent is a committer/infrastructure tool and #!/usr/bin/perl is the established convention; switching to #!/usr/bin/env perl changes portability assumptions by making perl resolution PATH-dependent, which can pick up an unexpected interpreter. Keep this hunk out of the feature patch.

Suggested change

	#!/usr/bin/env perl
	#!/usr/bin/perl

[github-actions]

This whole dev-shell infrastructure is personal tooling that should not be committed as part of a pgsql-hackers patch (minimal-diff / one-logical-change discipline). It also rebuilds glibc for the shell via overrideAttrs applying a locally-maintained glibc-no-fortify-warning.patch; the override's correctness depends entirely on that patch applying cleanly against the pinned glibc, and if it ever fails to apply it breaks nix develop evaluation/build for every variant (default, clang, musl). A heavyweight glibc rebuild to silence a -Wcpp warning is a non-reproducible-looking footgun. Keep this file as untracked local tooling.

[github-actions]

This flake and the associated dev-shell files (shell.nix, .clangd, .gdbinit, pg-aliases.sh) are personal developer-environment scaffolding and should not be bundled into the HOT-indexed-updates feature patch. Committing environment tooling violates the minimal-diff discipline and bundles unrelated changes; it belongs in a local worktree or an untracked gitignore (the .local-gitignore referenced by shell.nix suggests these are intended to be untracked anyway).

[github-actions]

This helper is personal developer tooling that should not be committed with the feature patch. Beyond scope, several aliases are environment-mutating and risk affecting unrelated processes/system state: pg-stop uses pkill -f "postgres.*-D.*$PG_DATA_DIR" which can match more than the intended server, and pg-enable-cores/pg-disable-cores write the system-wide kernel.core_pattern via sudo. It also depends on many non-standard tools (trash, compdb, codespell, rr, bpftrace, perf). Keep this as untracked local tooling rather than part of the tree.

[github-actions]

environment.localBinInPath is a NixOS-module option, not a valid per-system flake output. flake-utils.lib.eachDefaultSystem merges whatever this attrset returns into outputs.<attr>.<system>, so this produces a bogus environment.localBinInPath.<system> = true output that no flake consumer recognizes; it has no effect and signals the attribute was pasted from a NixOS config by mistake. Drop it. (This is separate from the already-noted problem that the whole dev-shell/flake scaffolding is out of scope for the HOT-indexed-updates patch.)

[github-actions]

This test does not reliably exercise the bitmap_and_inexact path it claims to guard, and would very likely pass with the feature reverted (a "worthless" regression test).

The VISIBILITYMAP_LOCATOR_SPLIT/bitmap_and_inexact fix only matters when the planner produces a BitmapAnd of two separate index scans (the exact-mode TID intersection is where the false negative occurs). With a single-row table and no statistics, the planner has every reason to pick a single-index Bitmap Index Scan plus a heap recheck filter on the other column, which never enters BitmapAnd. In that plan the row is returned regardless of whether the fix exists, so count(*) = 1 proves nothing.

Compare the existing bloom cases in this file (2000 rows, EXPLAIN-pinned) and the nested case in contrib/pageinspect/sql/hot_indexed_updates.sql, which deliberately uses a 2000-row table + VACUUM ANALYZE + EXPLAIN (COSTS OFF) precisely to force and verify a BitmapAnd. This test should do the same: populate enough rows that a BitmapAnd of bloom_hot_indexed_b and bloom_hot_indexed_c is actually chosen, and add an EXPLAIN (COSTS OFF) to pin the BitmapAnd plan shape so the test cannot silently degrade to a single bitmap scan.

[github-actions]

This block pins the exact nested BitmapAnd/BitmapOr plan shapes via EXPLAIN (COSTS OFF) into the committed expected output. Unlike the single-index EXPLAINs elsewhere in this file (which are forced by enable_* GUCs onto one unambiguous plan), here three indexes compete and the AND/OR nesting order and the choice of which index sits under which node are driven by the planner's cost estimates. Those estimates depend on the VACUUM ANALYZE statistics gathered on a 2000-row synthetic distribution and are liable to shift across platforms, planner-cost changes, and future releases — a classic source of intermittent buildfarm diffs. The semantic guarantee this test exists to prove is the result value {3,1853} (which the two array_agg queries already pin and is deterministic). Consider dropping the two EXPLAIN statements and relying on the result assertions, or if the plan shape must be guaranteed, force it deterministically rather than leaning on cost estimates.

[github-actions]

The heap_stayed_compact assertion depends on opportunistic HOT prune+collapse actually reclaiming the superseded versions during the 200-UPDATE loop. Reclaim is gated on the superseded versions falling below the global xmin horizon, which a concurrent snapshot held by another backend in the shared regression cluster can pin back. The adjacent hi_reclaim / hi_vm blocks explicitly document this horizon dependence and deliberately avoid asserting exact physical layout for exactly this reason, yet this block asserts a hard size bound without the same caveat. In the common single-session repeated-UPDATE case the superseded versions are dead to the current transaction and prune reliably, so this will usually hold; but it is a potential source of a flaky buildfarm failure. At minimum, align the reasoning here with the horizon caveat the neighbouring blocks already carry, or confirm the pruning is session-local and horizon-independent for this pattern.

[github-actions]

hi_aba is defined here (k int, v int; unique index on k) and then again ~150 lines later with a different schema (id int PRIMARY KEY, k int, v int; non-unique index on k). Likewise hi_part is reused for two unrelated schemas. The earlier instance is dropped before the later CREATE, so this is not a hard error, but reusing one name for two semantically distinct tables within a single script hurts readability and makes a failure harder to localize. Prefer distinct names (e.g. hi_aba_unique vs hi_aba_cycle).

[github-actions]

(hot - 0) > 0 is just hot > 0; the - 0 is dead arithmetic. Simplify to avoid puzzling future readers.

Suggested change

	SELECT (hot - 0) > 0 AS classic_hot_fired,
	SELECT hot > 0 AS classic_hot_fired,

[github-actions]

The helper parses tids by casting through text to point: (target_ctid::text::point)[0]::int. This relies on tid out and point in agreeing on the (block,offset) textual form and is a roundabout idiom not used anywhere else in the tree. It happens to be locale-safe (plain integers, '.'/',' punctuation), so it is not a correctness bug, but it is fragile relative to extracting the block/offset directly. Since these helpers only ever need the block number of a known ctid, consider a simpler extraction to reduce the cast chain's maintenance surface.

[github-actions]

This hunk is pure comment churn: the test's output is unchanged by the HOT-indexed-update feature (the expected .out diff only touches this comment). Replacing the plain --trigger section marker with prose about feature internals (heap_modify_tuple(), "the executor's SET-clause tracking") is a minimal-diff violation and the phrase "SET-clause tracking" is not an established term anywhere else in the tree, so it does not aid a reader of this test. Either drop this hunk entirely or restore the original --trigger marker. (moderate confidence)

Suggested change

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

[github-actions]

This GIN block contradicts this file's stated scope. The header (lines 5-11) says hot_updates.sql covers only HOT decisions that "apply identically on a pre-hot-indexed server" and that "hot-indexed-specific behaviour ... is covered in hot_indexed_updates.sql." Reordering a GIN-covered array column is detected as a modified indexed attribute (modified-column detection is image-based), so this update takes the HOT-indexed path; the expected output records hot=1, which depends on hot-indexed being enabled. On a pre-hot-indexed server this would be non-HOT (hot=0). Move this block to hot_indexed_updates.sql to keep this file's invariant (results independent of hot-indexed) intact.

[github-actions]

Dead code: get_hot_count is defined here (and dropped at the end) but never called anywhere in this file. All call sites use get_hi_count, which already returns hot alongside hot_idx. Remove this unused helper (and its DROP in the Cleanup section) per the minimal-diff/YAGNI discipline.

[github-actions]

This cleanup comment lists only pageinspect and amcheck, but btree_gist is also created with IF NOT EXISTS (near the CLUSTER-by-GiST test) and is likewise left installed. Update the comment to mention btree_gist so it matches what the script actually does.

Suggested change

	-- pageinspect and amcheck were both created above with IF NOT EXISTS and may
	-- have pre-existed this test; leave them, matching amcheck's treatment,
	-- rather than risk dropping an extension this test did not create.
	-- pageinspect, amcheck and btree_gist were all created above with IF NOT
	-- EXISTS and may have pre-existed this test; leave them, matching amcheck's
	-- treatment, rather than risk dropping an extension this test did not create.

[github-actions]

This chain walk undercounts chain length for any collapsed chain that contains stubs. A collapse-survivor stub is written in pruneheap.c with t_infomask2 = HEAP_ONLY_TUPLE | HEAP_INDEXED_UPDATED (note: NOT HEAP_HOT_UPDATED), and its chain successor is reached via HotIndexedStubGetForward() (the offset half of t_ctid), not via a HEAP_HOT_UPDATED t_ctid link. Here, when the walk reaches a stub it does len++ and then breaks at !(thdr->t_infomask2 & HEAP_HOT_UPDATED), so it stops at the first stub and never follows the stub's forward link. The authoritative walkers (heapam_indexscan.c, pruneheap.c) continue past stubs. Result: avg_chain_len/max_chain_len are undercounted for collapsed chains. The walk must handle a stub as a chain member and follow HotIndexedStubGetForward() instead of breaking. Also note the block-half check ItemPointerGetBlockNumber(&thdr->t_ctid) != blk is only valid for a real HOT-updated tuple; a stub's block half holds its stashed natts (HotIndexedStubSetBitmapNatts), not a block number. (high confidence)

[github-actions]

The header comment states n_hot_indexed is "live HOT-indexed versions", but this counter performs no visibility check: it counts every non-stub LP_NORMAL item with HEAP_INDEXED_UPDATED set, regardless of whether the tuple is visible/live or dead-but-not-yet-pruned. Per the comment-accuracy rule, either drop the word "live" (it counts on-disk HOT-indexed tuple versions) or add an actual visibility test. The current comment overpromises. (high confidence)

[github-actions]

Widening BITS_PER_HEAPBLOCK to 4 silently breaks visibilitymap_count(). That function (visibilitymap.c:533-535) calls pg_popcount_masked() with the pre-existing masks VISIBLE_MASK8 (0x55) and FROZEN_MASK8 (0xaa), which are hardcoded for 2-bit-per-block packing (visible bits at positions 0/2/4/6, frozen at 1/3/5/7). With 4 bits per block (2 blocks/byte) the real visible bits sit at positions 0 and 4 and frozen at 1 and 5, so the correct masks are 0x11 and 0x22. Under the old masks the counts become wrong: 0x55 does not even select the frozen-field's visible bit cleanly and mixes in the unused upper bits of each 4-bit field. This corrupts the all_visible/all_frozen totals that feed pg_class.relallvisible and the planner's index-only-scan costing. The masks must be updated alongside this width change (they are not in this diff but are directly invalidated by it). (high confidence)

Suggested change

	#define BITS_PER_HEAPBLOCK 4
	#define BITS_PER_HEAPBLOCK 4
	/*
	* NB: VISIBLE_MASK8/FROZEN_MASK8 in visibilitymap.c must track BITS_PER_HEAPBLOCK;
	* with 4 bits/block they must be 0x11/0x22, not the 2-bit 0x55/0xaa.
	*/

[github-actions]

These three new modifiers deviate from the existing visibilitymap_set/visibilitymap_clear wrong-buffer safeguard. Those functions use a runtime elog(ERROR, "wrong (VM) buffer passed to ...") so that passing a VM buffer for the wrong map block never silently writes (and WAL-logs) a modification to the wrong heap block in a production (assert-disabled) build. The split setter/clearer gate the same invariant with Assert(BufferGetBlockNumber(vmBuf) == mapBlock), which is compiled out in production — turning a hard error into silent VM/on-disk corruption. Since these functions dirty and WAL-log an on-disk page just like visibilitymap_set, they should use the same elog(ERROR) guard rather than an Assert. (moderate confidence)

[github-actions]

The header comment states "Caller holds the VM buffer exclusive-locked in a critical section," but unlike visibilitymap_set/visibilitymap_clear (which Assert(BufferIsLockedByMeInMode(vmBuf, BUFFER_LOCK_EXCLUSIVE))), neither visibilitymap_set_locator_split nor visibilitymap_clear_locator_split asserts the exclusive content lock is actually held. Add the same assertion so the documented locking contract is checked. (moderate confidence)

[github-actions]

Divergence from the established visibilitymap_get_status pattern: there, after ReleaseBuffer(*vmbuf) the code sets *vmbuf = InvalidBuffer before calling vm_readbuf(), so that if vm_readbuf() throws (it can ereport/palloc), the caller's cached buffer handle is not left pointing at an already-released buffer (which would be double-released on resource-owner cleanup). Here *vmbuf is only overwritten by vm_readbuf()'s return value, leaving a dangling released-buffer handle in *vmbuf across the vm_readbuf() call. Set *vmbuf = InvalidBuffer immediately after ReleaseBuffer to match the proven-safe idiom. (moderate confidence)

Suggested change

	if (BufferIsValid(*vmbuf))
	ReleaseBuffer(*vmbuf);
	*vmbuf = vm_readbuf(rel, mapBlock, false);
	if (BufferIsValid(*vmbuf))
	{
	ReleaseBuffer(*vmbuf);
	*vmbuf = InvalidBuffer;
	}
	*vmbuf = vm_readbuf(rel, mapBlock, false);

[github-actions]

The original one-line comment /* Number of bits for one heap page */ is now redundant with the new multi-line block immediately below it (which re-describes the same macro). Remove the stale one-liner (or fold its wording into the new block) to avoid duplicated/dangling comment text above a single #define. (low confidence)

Suggested change

	/* Number of bits for one heap page */
	#define BITS_PER_HEAPBLOCK 2
	/*
	* Bits per heap block in the visibility map. Widened from 2 to 4 to add
	/*
	* Bits per heap block in the visibility map. Widened from 2 to 4 to add

[github-actions]

[moderate confidence] This pass takes ConditionalLockBufferForCleanup and falls back to an exclusive lock, but the comment itself states this pass only turns LP_DEAD into LP_UNUSED and does NOT reclaim stubs or re-point redirects - i.e. it never does cleanup-lock-requiring work. The got_cleanup_lock result is only used to decide between cleanup vs. exclusive lock and is otherwise discarded. Preferring a cleanup lock here is therefore speculative overhead with no consumer; a plain exclusive lock (as before) suffices. If a later patch needs the cleanup lock, add it then. Also note the removed Assert(nunused > 0) just below: the caller only yields blocks with dead items, so that invariant still holds and the assertion should be kept (it matches the "guaranteed to have had dead line pointers" comment).

[github-actions]

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

1 could not be posted

src/backend/access/heap/vacuumlazy.c L2836: 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