Re: Tepid: selective index updates for heap relations - Mailing list pgsql-hackers

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