Re: Tepid: selective index updates for heap relations

Hello,

Attached is v63 which has a few fixes and is rebased (0b3e646fd7f) as well.

What changed vs v60?

Eight of the ten patches changed in content since v60; only 0001 (heap
HOT-update tests) and 0007 (amcheck) are unchanged apart from rebase-induced
hunk offsets. The headline is the CLUSTER data-loss fix (0004, with its
regression test in 0008); the rest are correctness, robustness, and
consistency fixes found in review, plus a test relocation.

The headline: CLUSTER data loss on HOT-indexed (SIU) tables

CLUSTER ... USING <secondary index> silently dropped live rows. On a table
with an SIU chain where the clustering column changed (or ABA-cycled) in an
earlier hop and a different indexed column changed in the last hop, with the
chain left uncollapsed (no VACUUM), CLUSTER returned 14 of 20 rows; six
live rows gone from the heap (seqscan-verified). Row count depending on
which index you cluster by is an unambiguous correctness bug.

Root cause:

CLUSTER-by-index copies the heap via an index scan under SnapshotAny,
trusting the clustering index to reach every live tuple directly. On an
uncollapsed SIU chain, a live tuple whose last hop changed a different
column has no clustering-index entry pointing at it, so it's unreachable
through that index -> never copied -> lost.

This affects any clusterable index AM, not just btree. Both btree and GiST
(the only two amclusterable core AMs) lose the same rows, and a btree-only
fix would have left GiST (and any out-of-tree clusterable AM) exposed.

The fix (patch 0004):

In the heap AM (heapam_relation_copy_for_cluster, heapam_handler.c). The
copy routes through the relation_copy_for_cluster tableam callback, so the
heap AM -- the layer that actually creates index-unreachable live tuples --
refuses the lossy index-scan copy path itself when it holds SIU chains (heap
has >1 index):

- btree clustering index -> switch to seqscan+sort (cluster order
preserved);
- any other clusterable AM (GiST, or out-of-tree) -> plain seqscan copy
(order dropped, no row lost).

Both direct-scan paths visit every heap tuple, the same way VACUUM FULL
already does. No index AM (core or out-of-tree) needs any change; the heap
guarantees its own invariant regardless of which index drove the CLUSTER.
(An earlier iteration carried a btree-only guard in repack.c; that is
reverted to pristine and the guard now lives in the heap AM.)

The regression test (patch 0008, hot_indexed_updates.sql): reproduces the
actual losing shape (a-ABA early, b in the last hop, fillfactor=40,
autovacuum_enabled=false, no VACUUM before CLUSTER), asserting all 20 rows
survive and remain findable, plus bt_index_check(heapallindexed => true). A
GiST case (CLUSTER ... USING <gist index>) proves the fix is AM-agnostic --
a guard a btree-only fix would miss. Both cases lose 6 rows without the fix
and pass with it.

Changes since v60

0002: identify modified indexed attributes in the executor

- Comment-only; the code is unchanged from v60. Documents why
ExecCompareSlotAttrs (used by ExecUpdateModifiedIdxAttrs on the executor
path) compares slots with datum_image_eq while the non-executor path
(simple_heap_update -> HeapUpdateModifiedIdxAttrs -> heap_attr_equals)
uses datumIsEqual. The difference is intentional: at the executor stage
the slot values are logical and cannot be TOASTed (TOAST is a heapam
storage detail applied later), the executor must stay agnostic of how a
table AM stores variable-length data, and datumIsEqual would force a
needless slot->Datum transform on a hot path.
- Also notes on heap_attr_equals and HeapUpdateModifiedIdxAttrs that they
are not public API -- their only consumer is simple_heap_update(), and
they are targeted for eventual removal once catalog-tuple updates track
their own changed columns.

0003: on-disk format

- Comment corrections only. (1) The inline attribute bitmap is located using
the tuple's own write-time attribute count, not the relation's current
natts -- after ADD COLUMN a chain can hold tuples whose bitmaps were sized
for a smaller natts, and sizing from the relation's current natts would
read past the tuple's data. (2) A new README.HOT-INDEXED section spells out
the datum_image_eq-vs-datumIsEqual rationale described under 0002.

0004: selective index maintenance and reads, beyond the CLUSTER fix above

- Ordered/distance index scans (GiST/SP-GiST, ORDER BY <->) now drop stale
HOT-indexed entries, which the non-ordered scan path already did; the
ordered path (IndexNextWithReorder) was the one remaining gap.
- The HOT/SIU chain walk in heap_hot_search_buffer is bounded by a per-page
hop guard and now calls CHECK_FOR_INTERRUPTS. A corrupt page whose forward
links form a cycle among valid in-range offsets would otherwise spin
forever under a buffer share-lock.
- The unique-insert self-check tolerates the legitimate repeat self-arrival
(the canonical direct entry plus stale chain-walk entries resolving to the
same TID, in either arrival order) while still raising on a genuine
duplicate TID.
- In _bt_check_unique, the right-sibling buffer is kept pinned while scanning
equal tuples on the HOT-indexed skip paths: page, opaque, and curitup may
point into it, so releasing it early would dereference a freed buffer.
- The palloc'd per-row modified-attrs bitmap is freed after the index
inserts consume it (a bulk UPDATE previously accumulated one Bitmapset per
updated row for the statement's lifetime), and the "update all indexes"
sentinel is accepted on input for an index whose expression references a
whole-row Var.

0005: BitmapAnd/BitmapOr false negative

- Assert that the stale marker (ItemPointerSetSIUMaybeStale) is only ever set
on a genuine in-range offset, not a sentinel offset.

0006: collapse dead chains to xid-free stubs

- Bound the stub-forward walk in heap_prune_chain with a per-page hop guard,
so a corrupt stub->stub cycle cannot spin forever (mirroring the guard
heap_prune_chain_find_live already has).
- Drop the got_cleanup_lock plumbing through lazy_vacuum_heap_page; rely on
heap_page_would_be_all_visible()'s own SIU-redirect and stub guards to
refuse marking a page all-visible while an unreclaimed HOT-indexed member
is still present.

0008: statistics + comprehensive test suite

- The SIU regression tests that depend on pageinspect/amcheck/btree_gist
moved from src/test/regress into contrib/pageinspect (core "make check"
does not install contrib, so those CREATE EXTENSION calls would fail under
a standard build; contrib/pageinspect is the idiomatic home and pulls the
other two in via EXTRA_INSTALL). The suite gained the CLUSTER and
GiST-CLUSTER cases described above, plus a KNN case for the ordered-scan
fix in 0004.

0009: gate HOT-indexed updates on the logical-replication apply path

- Derive the apply mode directly from MySubscription instead of caching it
in a worker.c static, removing a second copy that had to be kept in sync
across subscription reloads.

0010: benchmark harness

- DO NOT MERGE. Script-only robustness/comment fixes (results-dir creation,
early validation of the resolved base revision, skipping failed pgbench
iterations rather than recording NA rows, and a workload that actually
changes the indexed value); no change to what it benchmarks.

best.

-greg