From 5bcd70083fab87c87f27b930be2e572155afe295 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Fri, 10 Jul 2026 15:33:34 -0400 Subject: [PATCH v60 05/10] Fix a BitmapAnd/BitmapOr false negative on HOT-indexed fresh entries A HOT-indexed (SIU) update's fresh entry in a changed index points at the new heap-only tuple, not at the chain root the way every other index entry for the same logical row does -- that positional distinction is what lets the read side judge staleness from the crossed-attribute bitmap without a value recheck. BitmapAnd/BitmapOr combine two indexes' TID sets at raw block+offset granularity in tidbitmap.c, before either side ever touches the heap. An unrelated, unchanged index's root-pointing entry for the same row will not agree with the changed index's fresh entry's offset, so an exact-mode intersection can silently drop a row that matches both predicates. Reported by Alexander Korotkov. Fixed by reserving one otherwise-unused bit (bit 14) in a stored TID's offset field, ItemPointerSIUMaybeStaleFlag. MaxOffsetNumber never needs more than 14 bits even at the largest configurable BLCKSZ, so the bit is free for any real offset; it is set only on the local TID copy handed to a HOT-indexed fresh entry's index_insert() call in ExecInsertIndexTuples, never on the slot's own tts_tid. ItemPointerGetOffsetNumber and ItemPointerCompare strip the bit by default (via the sentinel-safe ItemPointerOffsetNumberStrip, which leaves SpecTokenOffsetNumber/MovedPartitionsOffsetNumber -- both of which already have this bit set as part of their own encoding -- untouched) so every ordinary consumer keeps seeing the real offset; only ItemPointerGetOffsetNumberNoCheck exposes the raw value. The consumption is centralized in tbm_add_tuples(), the single choke point every amgetbitmap funnels exact heap TIDs through: it tests the raw flag (before the offset is stripped) and, when set, adds the whole page as lossy (tbm_add_page) instead of the single exact offset. Per tbm_intersect_page's own case analysis a lossy page survives any AND/OR against an exact-mode page and forces a recheck, so BitmapHeapScan resolves the chain and the existing heap-side crossed-attribute staleness test makes the final, correct call. Because this lives in tbm_add_tuples and not in each access method, no index AM needs to know about HOT-indexed chains: btree, hash, GIN, GiST, SP-GiST, contrib/bloom, and out-of-tree AMs are all correct with no AM-specific code, and a TID that never carries the flag takes the identical path it always did. GIN's own page-level lossy sentinel (ItemPointerIsLossyPage, an unrelated 0xffff marker used before a real heap TID is produced) is untouched; the new check only applies to genuine heap-item TIDs. The cost is precision, not correctness: any heap page carrying a live fresh entry contributes lossy (a whole-page recheck for all its tuples on that bitmap scan, not just the SIU row) until the chain collapses. It is bounded and self-healing -- prune/VACUUM collapse restores exact-mode entries. amcheck's heapallindexed verification fingerprints leaf tuples' stored TIDs and compares them against the plain heap TIDs it re-derives from the heap scan; verify_nbtree.c now strips the marker while fingerprinting so a fresh entry does not raise a spurious "lacks matching index tuple". pageinspect 1.14's bt_page_items reports the real offset in its ctid and htid columns (earlier versions surfaced the marker as an inflated offset) and adds a hot_indexed boolean column exposing the marker explicitly. Caught its own regression during development: the first cut masked bit 14 unconditionally, which corrupted SpecTokenOffsetNumber and MovedPartitionsOffsetNumber (both already have bit 14 set), silently breaking cross-partition-UPDATE conflict detection -- caught by the isolation suite (eval-plan-qual, merge-update, partition-key-update). Fixed by gating the strip on the value being below the sentinel range. Regression coverage (BitmapAnd/BitmapOr across a changed+unchanged index for every access method SIU exercises, plus a bloom case in contrib/bloom and heapallindexed on a changed index) is added alongside the rest of the HOT-indexed test suite. --- contrib/amcheck/verify_nbtree.c | 27 ++++ contrib/bloom/expected/bloom.out | 32 +++++ contrib/bloom/sql/bloom.sql | 28 ++++ contrib/pageinspect/Makefile | 2 +- contrib/pageinspect/btreefuncs.c | 46 ++++++- contrib/pageinspect/expected/btree.out | 42 +++--- contrib/pageinspect/meson.build | 1 + .../pageinspect/pageinspect--1.13--1.14.sql | 49 +++++++ contrib/pageinspect/pageinspect.control | 2 +- doc/src/sgml/pageinspect.sgml | 12 ++ src/backend/access/heap/README.HOT-INDEXED | 130 ++++++++++++++++++ src/backend/executor/execIndexing.c | 52 +++++-- src/backend/nodes/tidbitmap.c | 37 ++++- src/backend/storage/page/itemptr.c | 18 ++- src/include/storage/itemptr.h | 101 +++++++++++++- 15 files changed, 535 insertions(+), 44 deletions(-) create mode 100644 contrib/pageinspect/pageinspect--1.13--1.14.sql diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c index 3ef2d66f826..bac1e5febd9 100644 --- a/contrib/amcheck/verify_nbtree.c +++ b/contrib/amcheck/verify_nbtree.c @@ -1489,6 +1489,29 @@ bt_target_page_check(BtreeCheckState *state) if (state->heapallindexed && P_ISLEAF(topaque) && !ItemIdIsDead(itemid)) { IndexTuple norm; + ItemPointerData saved_tid; + bool siu_stripped = false; + + /* + * A HOT-indexed (SIU) fresh entry stores its heap TID with the + * ItemPointerSIUMaybeStaleFlag marker bit set in the offset field + * (see storage/itemptr.h). The marker is a bitmap-scan hint, not + * part of the tuple's heap address, and the heapallindexed callback + * (bt_tuple_present_callback) fingerprints the plain heap TID it + * gets from the heap scan. Strip the marker from this leaf tuple's + * TID for the duration of fingerprinting so the two normalized + * images match; restore it immediately afterward, since later + * checks in this loop still read itup from the (immutable) page + * image. Only plain leaf tuples can carry the marker. + */ + if (!BTreeTupleIsPosting(itup) && + ItemPointerIsSIUMaybeStale(&itup->t_tid)) + { + saved_tid = itup->t_tid; + ItemPointerSetOffsetNumber(&itup->t_tid, + ItemPointerGetOffsetNumber(&itup->t_tid)); + siu_stripped = true; + } if (BTreeTupleIsPosting(itup)) { @@ -1516,6 +1539,10 @@ bt_target_page_check(BtreeCheckState *state) if (norm != itup) pfree(norm); } + + /* Restore the SIU marker bit stripped for fingerprinting, if any. */ + if (siu_stripped) + itup->t_tid = saved_tid; } /* diff --git a/contrib/bloom/expected/bloom.out b/contrib/bloom/expected/bloom.out index edc855121e1..a5675da62b4 100644 --- a/contrib/bloom/expected/bloom.out +++ b/contrib/bloom/expected/bloom.out @@ -228,3 +228,35 @@ CREATE INDEX bloomidx2 ON tst USING bloom (i, t) WITH (length=0); ERROR: value 0 out of bounds for option "length" CREATE INDEX bloomidx2 ON tst USING bloom (i, t) WITH (col1=0); ERROR: value 0 out of bounds for option "col1" +-- +-- HOT-indexed (SIU) BitmapAnd correctness with a bloom index. +-- +-- A bloom index is bitmap-scan-only and non-summarizing, so it is eligible to +-- be the "unchanged" side of a HOT-indexed update: the update changes a +-- btree-indexed column while leaving the bloom-indexed column alone. The +-- btree fresh entry then points at the new heap-only tuple and the bloom entry +-- still points at the chain root; a BitmapAnd of the two must not drop the +-- matching row (the SIU fresh entry's TID carries an internal may-be-stale +-- marker that tbm_add_tuples degrades to a lossy page, so the intersection +-- keeps the row and the heap-side recheck resolves it). This is handled +-- centrally in tbm_add_tuples, so bloom -- like every other index AM -- needs +-- no bloom-specific code for it; this test guards that. +CREATE TABLE bloom_siu (id int PRIMARY KEY, bcol int, changed int) WITH (fillfactor = 50); +CREATE INDEX bloom_siu_b ON bloom_siu USING bloom (bcol); +CREATE INDEX bloom_siu_c ON bloom_siu (changed); +INSERT INTO bloom_siu VALUES (1, 11, 21); +UPDATE bloom_siu SET changed = 22 WHERE id = 1; -- HOT-indexed: only "changed" +SET enable_seqscan = off; +SET enable_indexscan = off; +SET enable_bitmapscan = on; +-- Must return the row (previously a false negative before the SIU bitmap fix). +SELECT count(*) AS bloom_bitmapand FROM bloom_siu WHERE bcol = 11 AND changed = 22; + bloom_bitmapand +----------------- + 1 +(1 row) + +RESET enable_seqscan; +RESET enable_indexscan; +RESET enable_bitmapscan; +DROP TABLE bloom_siu; diff --git a/contrib/bloom/sql/bloom.sql b/contrib/bloom/sql/bloom.sql index fa63b301c6e..28c9b3674ef 100644 --- a/contrib/bloom/sql/bloom.sql +++ b/contrib/bloom/sql/bloom.sql @@ -93,3 +93,31 @@ SELECT reloptions FROM pg_class WHERE oid = 'bloomidx'::regclass; \set VERBOSITY terse CREATE INDEX bloomidx2 ON tst USING bloom (i, t) WITH (length=0); CREATE INDEX bloomidx2 ON tst USING bloom (i, t) WITH (col1=0); + +-- +-- HOT-indexed (SIU) BitmapAnd correctness with a bloom index. +-- +-- A bloom index is bitmap-scan-only and non-summarizing, so it is eligible to +-- be the "unchanged" side of a HOT-indexed update: the update changes a +-- btree-indexed column while leaving the bloom-indexed column alone. The +-- btree fresh entry then points at the new heap-only tuple and the bloom entry +-- still points at the chain root; a BitmapAnd of the two must not drop the +-- matching row (the SIU fresh entry's TID carries an internal may-be-stale +-- marker that tbm_add_tuples degrades to a lossy page, so the intersection +-- keeps the row and the heap-side recheck resolves it). This is handled +-- centrally in tbm_add_tuples, so bloom -- like every other index AM -- needs +-- no bloom-specific code for it; this test guards that. +CREATE TABLE bloom_siu (id int PRIMARY KEY, bcol int, changed int) WITH (fillfactor = 50); +CREATE INDEX bloom_siu_b ON bloom_siu USING bloom (bcol); +CREATE INDEX bloom_siu_c ON bloom_siu (changed); +INSERT INTO bloom_siu VALUES (1, 11, 21); +UPDATE bloom_siu SET changed = 22 WHERE id = 1; -- HOT-indexed: only "changed" +SET enable_seqscan = off; +SET enable_indexscan = off; +SET enable_bitmapscan = on; +-- Must return the row (previously a false negative before the SIU bitmap fix). +SELECT count(*) AS bloom_bitmapand FROM bloom_siu WHERE bcol = 11 AND changed = 22; +RESET enable_seqscan; +RESET enable_indexscan; +RESET enable_bitmapscan; +DROP TABLE bloom_siu; diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile index eae989569d0..09774fd340c 100644 --- a/contrib/pageinspect/Makefile +++ b/contrib/pageinspect/Makefile @@ -13,7 +13,7 @@ OBJS = \ rawpage.o EXTENSION = pageinspect -DATA = pageinspect--1.12--1.13.sql \ +DATA = pageinspect--1.13--1.14.sql pageinspect--1.12--1.13.sql \ pageinspect--1.11--1.12.sql pageinspect--1.10--1.11.sql \ pageinspect--1.9--1.10.sql pageinspect--1.8--1.9.sql \ pageinspect--1.7--1.8.sql pageinspect--1.6--1.7.sql \ diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c index 3381e7cc2a7..f5849c9d6c4 100644 --- a/contrib/pageinspect/btreefuncs.c +++ b/contrib/pageinspect/btreefuncs.c @@ -485,8 +485,8 @@ bt_page_print_tuples(ua_page_items *uargs) bool leafpage = uargs->leafpage; bool rightmost = uargs->rightmost; bool ispivottuple; - Datum values[9]; - bool nulls[9]; + Datum values[10]; + bool nulls[10]; HeapTuple tuple; ItemId id; IndexTuple itup; @@ -497,6 +497,9 @@ bt_page_print_tuples(ua_page_items *uargs) *datacstring; char *ptr; ItemPointer htid; + bool hot_indexed; + ItemPointerData ctid_show; + ItemPointerData htid_show; id = PageGetItemId(page, offset); @@ -508,7 +511,29 @@ bt_page_print_tuples(ua_page_items *uargs) j = 0; memset(nulls, 0, sizeof(nulls)); values[j++] = Int16GetDatum(offset); - values[j++] = ItemPointerGetDatum(&itup->t_tid); + + /* + * A HOT-indexed (SIU) fresh entry stores its heap TID with the + * ItemPointerSIUMaybeStaleFlag marker bit set in the offset field (see + * storage/itemptr.h). Report the real offset in the ctid column -- the + * marker is a bitmap-scan hint, not part of the tuple's address -- and + * expose the marker itself in the separate hot_indexed column below. + * Only a plain leaf tuple's t_tid is a genuine heap TID that can carry + * the flag; a pivot/posting tuple repurposes t_tid for other data and + * must be shown verbatim. + */ + if (!BTreeTupleIsPivot(itup) && !BTreeTupleIsPosting(itup)) + { + ctid_show = itup->t_tid; + hot_indexed = ItemPointerIsSIUMaybeStale(&ctid_show); + ItemPointerSetOffsetNumber(&ctid_show, ItemPointerGetOffsetNumber(&ctid_show)); + values[j++] = ItemPointerGetDatum(&ctid_show); + } + else + { + hot_indexed = false; + values[j++] = ItemPointerGetDatum(&itup->t_tid); + } values[j++] = Int16GetDatum(IndexTupleSize(itup)); values[j++] = BoolGetDatum(IndexTupleHasNulls(itup)); values[j++] = BoolGetDatum(IndexTupleHasVarwidths(itup)); @@ -584,7 +609,12 @@ bt_page_print_tuples(ua_page_items *uargs) } if (htid) - values[j++] = ItemPointerGetDatum(htid); + { + /* Report the real offset; the SIU marker is shown via hot_indexed. */ + htid_show = *htid; + ItemPointerSetOffsetNumber(&htid_show, ItemPointerGetOffsetNumber(&htid_show)); + values[j++] = ItemPointerGetDatum(&htid_show); + } else nulls[j++] = true; @@ -606,6 +636,14 @@ bt_page_print_tuples(ua_page_items *uargs) else nulls[j++] = true; + /* + * hot_indexed: true iff this leaf entry is a HOT-indexed (SIU) fresh + * entry. Only present from pageinspect 1.14 on; older extension versions + * declare one fewer output column, so gate on the actual tuple descriptor. + */ + if (uargs->tupd->natts > j) + values[j++] = BoolGetDatum(hot_indexed); + /* Build and return the result tuple */ tuple = heap_form_tuple(uargs->tupd, values, nulls); diff --git a/contrib/pageinspect/expected/btree.out b/contrib/pageinspect/expected/btree.out index 0aa5d73322f..173734c2153 100644 --- a/contrib/pageinspect/expected/btree.out +++ b/contrib/pageinspect/expected/btree.out @@ -152,16 +152,17 @@ ERROR: invalid block number -1 SELECT * FROM bt_page_items('test1_a_idx', 0); ERROR: block 0 is a meta page SELECT * FROM bt_page_items('test1_a_idx', 1); --[ RECORD 1 ]----------------------- -itemoffset | 1 -ctid | (0,1) -itemlen | 16 -nulls | f -vars | f -data | 01 00 00 00 00 00 00 01 -dead | f -htid | (0,1) -tids | +-[ RECORD 1 ]------------------------ +itemoffset | 1 +ctid | (0,1) +itemlen | 16 +nulls | f +vars | f +data | 01 00 00 00 00 00 00 01 +dead | f +htid | (0,1) +tids | +hot_indexed | f SELECT * FROM bt_page_items('test1_a_idx', 2); ERROR: block number 2 is out of range @@ -170,16 +171,17 @@ ERROR: invalid block number SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 0)); ERROR: block is a meta page SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 1)); --[ RECORD 1 ]----------------------- -itemoffset | 1 -ctid | (0,1) -itemlen | 16 -nulls | f -vars | f -data | 01 00 00 00 00 00 00 01 -dead | f -htid | (0,1) -tids | +-[ RECORD 1 ]------------------------ +itemoffset | 1 +ctid | (0,1) +itemlen | 16 +nulls | f +vars | f +data | 01 00 00 00 00 00 00 01 +dead | f +htid | (0,1) +tids | +hot_indexed | f SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 2)); ERROR: block number 2 is out of range for relation "test1_a_idx" diff --git a/contrib/pageinspect/meson.build b/contrib/pageinspect/meson.build index c43ea400a4d..2f333635838 100644 --- a/contrib/pageinspect/meson.build +++ b/contrib/pageinspect/meson.build @@ -38,6 +38,7 @@ install_data( 'pageinspect--1.10--1.11.sql', 'pageinspect--1.11--1.12.sql', 'pageinspect--1.12--1.13.sql', + 'pageinspect--1.13--1.14.sql', 'pageinspect.control', kwargs: contrib_data_args, ) diff --git a/contrib/pageinspect/pageinspect--1.13--1.14.sql b/contrib/pageinspect/pageinspect--1.13--1.14.sql new file mode 100644 index 00000000000..6c5c824d5e2 --- /dev/null +++ b/contrib/pageinspect/pageinspect--1.13--1.14.sql @@ -0,0 +1,49 @@ +/* contrib/pageinspect/pageinspect--1.13--1.14.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "ALTER EXTENSION pageinspect UPDATE TO '1.14'" to load this file. \quit + +-- +-- bt_page_items(relname, blkno) -- add hot_indexed column +-- +-- A HOT-indexed (SIU) fresh index entry stores its heap TID with an internal +-- marker bit (ItemPointerSIUMaybeStaleFlag) set in the offset field, which +-- tells a bitmap scan the entry points mid-chain and must be resolved +-- lossily. Earlier pageinspect versions surfaced that marker as an inflated +-- ctid offset; from 1.14 the ctid/htid columns report the real offset and the +-- new hot_indexed column exposes the marker explicitly. +-- +DROP FUNCTION bt_page_items(text, int8); +CREATE FUNCTION bt_page_items(IN relname text, IN blkno int8, + OUT itemoffset smallint, + OUT ctid tid, + OUT itemlen smallint, + OUT nulls bool, + OUT vars bool, + OUT data text, + OUT dead boolean, + OUT htid tid, + OUT tids tid[], + OUT hot_indexed bool) +RETURNS SETOF record +AS 'MODULE_PATHNAME', 'bt_page_items_1_9' +LANGUAGE C STRICT PARALLEL SAFE; + +-- +-- bt_page_items(page) -- add hot_indexed column +-- +DROP FUNCTION bt_page_items(bytea); +CREATE FUNCTION bt_page_items(IN page bytea, + OUT itemoffset smallint, + OUT ctid tid, + OUT itemlen smallint, + OUT nulls bool, + OUT vars bool, + OUT data text, + OUT dead boolean, + OUT htid tid, + OUT tids tid[], + OUT hot_indexed bool) +RETURNS SETOF record +AS 'MODULE_PATHNAME', 'bt_page_items_bytea' +LANGUAGE C STRICT PARALLEL SAFE; diff --git a/contrib/pageinspect/pageinspect.control b/contrib/pageinspect/pageinspect.control index cfc87feac03..aee3f598a9e 100644 --- a/contrib/pageinspect/pageinspect.control +++ b/contrib/pageinspect/pageinspect.control @@ -1,5 +1,5 @@ # pageinspect extension comment = 'inspect the contents of database pages at a low level' -default_version = '1.13' +default_version = '1.14' module_pathname = '$libdir/pageinspect' relocatable = true diff --git a/doc/src/sgml/pageinspect.sgml b/doc/src/sgml/pageinspect.sgml index 3a113439e1d..3e9576419c3 100644 --- a/doc/src/sgml/pageinspect.sgml +++ b/doc/src/sgml/pageinspect.sgml @@ -458,6 +458,18 @@ test=# SELECT itemoffset, ctid, itemlen, nulls, vars, data, dead, htid, tids[0:2 away, which is represented as a NULL htid value. + + hot_indexed is true when the leaf tuple is a + fresh index entry planted by a HOT-indexed (selective index update) + UPDATE, which points at a mid-chain heap-only tuple + rather than at the chain root. Such an entry's stored heap TID carries + an internal marker bit in its offset that a bitmap scan uses to fall + back to page-level (lossy) matching; ctid and + htid report the real heap offset with that + marker removed, and hot_indexed exposes the + marker itself. It is false for ordinary entries and for the pivot and + posting-list tuples whose ctid is repurposed. + Note that the first item on any non-rightmost page (any page with a non-zero value in the btpo_next field) is the diff --git a/src/backend/access/heap/README.HOT-INDEXED b/src/backend/access/heap/README.HOT-INDEXED index 5d4a2c7d66c..b562831039d 100644 --- a/src/backend/access/heap/README.HOT-INDEXED +++ b/src/backend/access/heap/README.HOT-INDEXED @@ -206,6 +206,136 @@ under the opclass even if not bitwise-identical, e.g. numeric 1.0 vs 1.00) is still detected. (Appendix A motivates this recheck in detail.) +Bitmap scans: the mid-chain TID and BitmapAnd/BitmapOr +----------------------------------------------------- + +The crossed-attribute test above runs on the way to the heap. A bitmap scan +has an earlier hazard it cannot reach: BitmapAnd/BitmapOr combine two indexes' +TID sets in tidbitmap.c at raw block+offset granularity, before either side +touches the heap. A HOT-indexed fresh entry points at the mid-chain new tuple +while an unchanged index's entry for the same row points at the chain root, so +for "WHERE changed_col = x AND unchanged_col = y" the two bitmap index scans +contribute different TIDs for the one live row, the exact-mode intersection +finds nothing in common, and the row is dropped -- a false negative (bitmap +scans tolerate false positives, not false negatives). + +A fresh entry's stored heap TID therefore carries ItemPointerSIUMaybeStaleFlag, +bit 14 of the offset field (see storage/itemptr.h). MaxOffsetNumber needs at +most 14 bits even at the largest configurable BLCKSZ, so the bit is free for +any real offset; the two reserved offset sentinels (SpecTokenOffsetNumber, +MovedPartitionsOffsetNumber) already set it as part of their own encoding, so +ItemPointerGetOffsetNumber's strip is range-gated to leave them intact. The +flag is set only on the local TID copy handed to a fresh entry's index_insert +(ExecInsertIndexTuples), never on the heap tuple's own t_ctid and never on a +classic-HOT or plain entry. ItemPointerGetOffsetNumber and ItemPointerCompare +strip it by default, so the plain index-scan heap lookup, the unique check, +tuplesort, and the tid opclass all see the real offset; only +ItemPointerGetOffsetNumberNoCheck exposes the raw value. + +tbm_add_tuples -- the single choke point every amgetbitmap funnels exact heap +TIDs through -- checks the flag and, when set, adds the whole page as lossy +(tbm_add_page) instead of the single exact offset. A lossy page survives any +AND/OR against an exact-mode page (tbm_intersect_page's own case analysis) and +forces a recheck, so BitmapHeapScan resolves the chain and the crossed- +attribute test above makes the final call. Because this lives in +tbm_add_tuples, no index access method needs any HOT-indexed-specific code: +btree, hash, GIN, GiST, SP-GiST, contrib/bloom, and out-of-tree AMs are all +correct, and a TID that never carries the flag takes the identical old path. +GIN's unrelated page-level lossy sentinel (ItemPointerIsLossyPage) is not +affected. + +The cost is precision, not correctness: a heap page carrying a live fresh +entry contributes lossy (a whole-page recheck on that bitmap scan) until the +chain collapses; it is bounded and self-healing via prune/VACUUM. pageinspect +1.14's bt_page_items reports the real offset (earlier versions showed the +marker as an inflated offset) and adds a hot_indexed boolean column. + + +Prune and chain collapse +------------------------- + +Because a HOT-indexed update plants an index entry pointing at a mid-chain +heap-only tuple's own TID, classic HOT's assumption that mid-chain line +pointers have no external references no longer holds. Pruning therefore must +not reclaim such a line pointer while a not-yet-swept index entry can still +arrive at it. + +heap_prune_chain collapses a run of dead chain members to a single +LP_REDIRECT that forwards to the first live tuple, and preserves the line +pointer of a live HOT-indexed member (heap_prune_item_preserves_hot_indexed) +so a reader arriving via a stale entry still finds a walkable hop. More than +one LP_REDIRECT may forward to the same live tuple. The redirect lifecycle +reuses the existing prune WAL records; there is no new on-disk format. + + +Vacuum reclamation +------------------ + +VACUUM's index cleanup sweeps the stale index entries. The collapse back to +classic HOT is driven by prune, not by VACUUM's second pass: once a chain is +fully dead, a later prune (heap_prune_chain / heap_prune_chain_find_live) +reclaims its members and re-points the root redirect straight at first_live. +Re-pointing a redirect preserves reachability (every walker still reaches +first_live), so it is safe under the exclusive lock prune already holds. + +VACUUM's second pass (lazy_vacuum_heap_page) does not itself re-point +redirects or reclaim stubs; it performs the usual LP_DEAD -> LP_UNUSED +conversion and leaves the HOT-indexed collapse to prune. + +A page that still carries a preserved HOT-indexed member or a collapse-survivor +stub is deliberately left non-all-visible, so that an index-only scan +heap-fetches through the chain and the crossed-attribute bitmap can filter +stale entries (enforced in heap_prune_record_redirect, the stub recorders, and +heap_page_would_be_all_visible). + + +amcheck and statistics +---------------------- + +verify_heapam treats the HOT-indexed artifacts as legitimate: a live +HEAP_INDEXED_UPDATED heap-only tuple whose line pointer is preserved, and +multiple LP_REDIRECTs forwarding to one live tuple. + +Statistics: pg_stat_all_tables.n_tup_hot_indexed_upd counts HOT-indexed +updates; pg_stat_all_indexes.n_tup_hot_indexed_upd_matched / _skipped count +per-index recheck outcomes; and pg_relation_hot_indexed_stats() reports +per-relation HOT-indexed chain counts. + + +Logical replication +------------------- + +A HOT-indexed update of a replica-identity attribute on a subscriber leaves a +stale index leaf; the apply worker's replica-identity lookups tolerate that +only when the indexed attributes are covered by the replica identity. The +per-subscription hot_indexed_on_apply option (pg_subscription.subhotindexedonapply, +off / subset_only / always; subset_only is the default) controls this: +HeapUpdateHotAllowable consults GetHotIndexedApplyMode on the apply path and +falls back to non-HOT when the indexed attributes are not exactly the primary +key (off) or not a subset of it (subset_only). + + +Recovery +-------- + +HOT-indexed updates and the prune/collapse use the existing heap UPDATE and +prune/freeze WAL records, so crash recovery replays them with no new record +types. src/test/recovery/t/054_hot_indexed_recovery.pl builds a chain, +crashes without a checkpoint (forcing WAL redo), and verifies the chain walk, +verify_heapam, and vacuum reclamation after restart, with +wal_consistency_checking = 'all' comparing each replayed page to its FPI. + + +Adversarial tests +----------------- + +src/test/isolation/specs/hot_indexed_adversarial.spec exercises the cases the +invariant must satisfy under concurrency: key cycling (X->Y->X), aborted +HOT-indexed updates, concurrent unique inserts against a freed/taken key, +snapshot-safe reclaim of stale leaves, and reader consistency across a +concurrent prune/collapse. + + Appendices ---------- diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index a23144592ea..e5fad59fdcc 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -461,15 +461,49 @@ ExecInsertIndexTuples(ResultRelInfo *resultRelInfo, indexUnchanged = (flags & EIIT_IS_UPDATE) ? indexInfo->ii_IndexUnchanged : false; - satisfiesConstraint = - index_insert(indexRelation, /* index relation */ - values, /* array of index Datums */ - isnull, /* null flags */ - tupleid, /* tid of heap tuple */ - heapRelation, /* heap relation */ - checkUnique, /* type of uniqueness check to do */ - indexUnchanged, /* UPDATE without logical change? */ - indexInfo); /* index AM may need this */ + /* + * A fresh entry planted here under a HOT-indexed update points at the + * new heap-only tuple itself (tupleid), not at the chain's root the + * way every other index entry does -- that positional distinction is + * what lets the read side judge staleness from the crossed-attribute + * bitmap without a value recheck (see hot_indexed.h). A bitmap scan + * combines two indexes' TID sets at raw block+offset granularity + * before either side touches the heap, so an unrelated, unchanged + * index's root-pointing entry for this same row will not agree with + * this entry's offset, and BitmapAnd/BitmapOr can silently drop a + * matching row. Flag the copy of tupleid handed to this index's + * insert (never the slot's own tts_tid, which other indexes in this + * same loop -- and the caller -- still need unflagged) so every + * amgetbitmap implementation can recognize the hazard from the TID + * alone and fall back to a page-level bitmap contribution instead of + * an exact one; see ItemPointerSIUMaybeStaleFlag in itemptr.h. + */ + if ((flags & EIIT_IS_HOT_INDEXED) && !indexInfo->ii_Summarizing) + { + ItemPointerData siu_tid = *tupleid; + + ItemPointerSetSIUMaybeStale(&siu_tid); + + satisfiesConstraint = + index_insert(indexRelation, /* index relation */ + values, /* array of index Datums */ + isnull, /* null flags */ + &siu_tid, /* tid of heap tuple, SIU-flagged */ + heapRelation, /* heap relation */ + checkUnique, /* type of uniqueness check to do */ + indexUnchanged, /* UPDATE without logical change? */ + indexInfo); /* index AM may need this */ + } + else + satisfiesConstraint = + index_insert(indexRelation, /* index relation */ + values, /* array of index Datums */ + isnull, /* null flags */ + tupleid, /* tid of heap tuple */ + heapRelation, /* heap relation */ + checkUnique, /* type of uniqueness check to do */ + indexUnchanged, /* UPDATE without logical change? */ + indexInfo); /* index AM may need this */ /* * If the index has an associated exclusion constraint, check that. diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index f1f925cb13b..f3faf51e4b9 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -362,6 +362,25 @@ tbm_free_shared_area(dsa_area *dsa, dsa_pointer dp) * * If recheck is true, then the recheck flag will be set in the * TBMIterateResult when any of these tuples are reported out. + * + * A TID may carry the SIU "may-be-stale" marker (ItemPointerSIUMaybeStaleFlag, + * see storage/itemptr.h): a HOT-indexed (selective index update) fresh entry + * points at a mid-chain heap-only tuple rather than at the chain root the way + * every other index entry for that row does. Because BitmapAnd/BitmapOr + * intersect/union TID sets here at block+offset granularity before the heap + * is ever consulted, an unrelated unchanged index's root-pointing entry for + * the same row would not agree with such an entry's offset, and an exact-mode + * intersection could silently drop a matching row. Whenever we see the flag + * we therefore add the whole page as lossy instead of the exact offset: a + * lossy page survives any AND/OR against an exact-mode page (see + * tbm_intersect_page) and forces a recheck, so BitmapHeapScan resolves the + * chain and the table AM's heap-side staleness test makes the final call. + * + * Handling this here -- at the single choke point every amgetbitmap funnels + * exact heap TIDs through -- means no index access method needs to know about + * HOT-indexed chains: core AMs, contrib AMs (bloom), and out-of-tree AMs are + * all correct automatically, and a TID that never carries the flag takes the + * identical path it always did. */ void tbm_add_tuples(TIDBitmap *tbm, const ItemPointerData *tids, int ntids, @@ -374,10 +393,26 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointerData *tids, int ntids, for (int i = 0; i < ntids; i++) { BlockNumber blk = ItemPointerGetBlockNumber(tids + i); - OffsetNumber off = ItemPointerGetOffsetNumber(tids + i); + OffsetNumber off; int wordnum, bitnum; + /* + * A HOT-indexed fresh entry's TID must not be trusted at exact + * offset granularity here (see the function header). Test the raw + * TID -- before ItemPointerGetOffsetNumber strips the marker -- and + * if set, degrade the whole page to lossy and move on. + */ + if (unlikely(ItemPointerIsSIUMaybeStale(tids + i))) + { + tbm_add_page(tbm, blk); + /* tbm_add_page may have lossified pages; force a fresh lookup */ + currblk = InvalidBlockNumber; + continue; + } + + off = ItemPointerGetOffsetNumber(tids + i); + /* safety check to ensure we don't overrun bit array bounds */ if (off < 1 || off > TBM_MAX_TUPLES_PER_PAGE) elog(ERROR, "tuple offset out of range: %u", off); diff --git a/src/backend/storage/page/itemptr.c b/src/backend/storage/page/itemptr.c index 546874ebc5f..3c57502b919 100644 --- a/src/backend/storage/page/itemptr.c +++ b/src/backend/storage/page/itemptr.c @@ -52,20 +52,28 @@ ItemPointerCompare(const ItemPointerData *arg1, const ItemPointerData *arg2) { /* * Use ItemPointerGet{Offset,Block}NumberNoCheck to avoid asserting - * ip_posid != 0, which may not be true for a user-supplied TID. + * ip_posid != 0, which may not be true for a user-supplied TID. Strip + * the SIU may-be-stale bit (see ItemPointerSIUMaybeStaleFlag in + * itemptr.h) so two TIDs naming the same (block, offset) always compare + * equal regardless of which one, if either, happens to carry it -- the + * bit is a hint to a handful of bitmap-scan call sites, not part of a + * TID's identity for ordering/equality purposes. ItemPointerOffsetNumberStrip + * leaves the reserved sentinel values (SpecTokenOffsetNumber, + * MovedPartitionsOffsetNumber) untouched, since those already have the + * same bit set as part of their own encoding. */ BlockNumber b1 = ItemPointerGetBlockNumberNoCheck(arg1); BlockNumber b2 = ItemPointerGetBlockNumberNoCheck(arg2); + OffsetNumber o1 = ItemPointerOffsetNumberStrip(ItemPointerGetOffsetNumberNoCheck(arg1)); + OffsetNumber o2 = ItemPointerOffsetNumberStrip(ItemPointerGetOffsetNumberNoCheck(arg2)); if (b1 < b2) return -1; else if (b1 > b2) return 1; - else if (ItemPointerGetOffsetNumberNoCheck(arg1) < - ItemPointerGetOffsetNumberNoCheck(arg2)) + else if (o1 < o2) return -1; - else if (ItemPointerGetOffsetNumberNoCheck(arg1) > - ItemPointerGetOffsetNumberNoCheck(arg2)) + else if (o1 > o2) return 1; else return 0; diff --git a/src/include/storage/itemptr.h b/src/include/storage/itemptr.h index 8b3392dbefe..7b746998962 100644 --- a/src/include/storage/itemptr.h +++ b/src/include/storage/itemptr.h @@ -69,6 +69,65 @@ typedef ItemPointerData *ItemPointer; #define MovedPartitionsOffsetNumber 0xfffd #define MovedPartitionsBlockNumber InvalidBlockNumber +/* + * SIU (selective index update / HOT-indexed) "may-be-stale" marker bit. + * + * Set ONLY on a TID stored inside an INDEX TUPLE by a HOT-indexed update's + * fresh entry -- never on a heap tuple's own t_ctid, and never on a plain + * ItemPointer value passed around the executor/table AM (those still mean + * exactly what they always have). Such an entry points at a non-root, + * heap-only chain member rather than at the chain's root the way every + * other index entry does, so a bitmap scan must not trust a raw offset- + * level match against it: BitmapAnd/BitmapOr intersect two indexes' TID + * sets at block+offset granularity, and this entry's offset will not agree + * with an unrelated, unchanged index's entry for the same logical row (see + * the HOT-indexed design notes for the full false-negative scenario). A + * caller that finds this bit set on a heap-item TID must fall back to a + * page-level (lossy) bitmap contribution instead of an exact one, forcing + * the existing heap-side crossed-attribute recheck to make the final call. + * + * Bit 14 is free at every supported BLCKSZ: MaxOffsetNumber needs at most + * 14 bits (BLCKSZ=32KB, the largest configurable block size) to represent + * every legal offset, leaving bits 14-15 unused by the real offset value; + * bit 15 is left alone because it is set in both of the sentinels above + * (SpecTokenOffsetNumber, MovedPartitionsOffsetNumber), and staying clear + * of that neighborhood avoids ever having to reason about the interaction. + * nbtree separately overloads the top 4 bits of a *pivot or posting* tuple's + * own t_tid (BT_STATUS_OFFSET_MASK, gated by INDEX_ALT_TID_MASK in t_info) + * to mean something else entirely -- this bit is never read or set on that + * field; it only ever applies to a genuine heap-row TID (including each + * individual entry of a posting list's heap-TID array, which are ordinary + * TIDs despite living inside a posting tuple). + * + * IMPORTANT: never mask this bit off unconditionally. Both sentinels above + * (SpecTokenOffsetNumber = 0xfffe, MovedPartitionsOffsetNumber = 0xfffd) + * already have bit 14 set as part of their own value; blindly clearing it + * would corrupt them into an unrecognized offset (this was caught by the + * isolation suite: partition-key-update / MERGE tests silently stopped + * detecting a concurrently-moved tuple). A flagged real offset is always + * well below the sentinel range even at the largest configurable BLCKSZ + * (worst case 0x6000 at BLCKSZ=32KB, vs. sentinels at 0xfffd/0xfffe), so + * ItemPointerOffsetNumberStrip only clears the bit when the raw value is + * below that range; a sentinel passes through unchanged. + */ +#define ItemPointerSIUMaybeStaleFlag ((OffsetNumber) (1 << 14)) + +#define ItemPointerOffsetNumberMask ((OffsetNumber) ~ItemPointerSIUMaybeStaleFlag) + +/* + * ItemPointerOffsetNumberStrip + * Strip the SIU may-be-stale bit from a raw ip_posid value, UNLESS that + * value is one of the reserved sentinels (SpecTokenOffsetNumber, + * MovedPartitionsOffsetNumber), which must pass through untouched. + */ +static inline OffsetNumber +ItemPointerOffsetNumberStrip(OffsetNumber raw) +{ + if (raw >= MovedPartitionsOffsetNumber) /* covers both sentinels */ + return raw; + return (OffsetNumber) (raw & ItemPointerOffsetNumberMask); +} + /* ---------------- * support functions @@ -108,7 +167,11 @@ ItemPointerGetBlockNumber(const ItemPointerData *pointer) /* * ItemPointerGetOffsetNumberNoCheck - * Returns the offset number of a disk item pointer. + * Returns the offset number of a disk item pointer, INCLUDING the SIU + * may-be-stale bit if set. Only appropriate for code that explicitly + * wants the raw stored value (e.g. bitmap-scan code deciding whether to + * trust an exact match); anything that will use the result to locate a + * line pointer on a page must use ItemPointerGetOffsetNumber instead. */ static inline OffsetNumber ItemPointerGetOffsetNumberNoCheck(const ItemPointerData *pointer) @@ -118,13 +181,45 @@ ItemPointerGetOffsetNumberNoCheck(const ItemPointerData *pointer) /* * ItemPointerGetOffsetNumber - * As above, but verifies that the item pointer looks valid. + * As above, but verifies that the item pointer looks valid, AND strips + * the SIU may-be-stale bit (see ItemPointerSIUMaybeStaleFlag) so every + * ordinary consumer -- anything that turns this into a page lookup, a + * comparison against another TID, or a value handed back to a caller + * that predates this bit's existence -- keeps seeing exactly the real + * offset it always has. Only the handful of call sites that need to + * notice the flag (currently: each amgetbitmap implementation) use + * ItemPointerGetOffsetNumberNoCheck / ItemPointerIsSIUMaybeStale instead. */ static inline OffsetNumber ItemPointerGetOffsetNumber(const ItemPointerData *pointer) { Assert(ItemPointerIsValid(pointer)); - return ItemPointerGetOffsetNumberNoCheck(pointer); + return ItemPointerOffsetNumberStrip(ItemPointerGetOffsetNumberNoCheck(pointer)); +} + +/* + * ItemPointerIsSIUMaybeStale + * True iff this heap-row TID (as stored in an index tuple) was + * flagged by a HOT-indexed fresh-entry insert as pointing at a + * non-root chain member; see ItemPointerSIUMaybeStaleFlag. + */ +static inline bool +ItemPointerIsSIUMaybeStale(const ItemPointerData *pointer) +{ + return (pointer->ip_posid & ItemPointerSIUMaybeStaleFlag) != 0; +} + +/* + * ItemPointerSetSIUMaybeStale + * Set the SIU may-be-stale bit on a heap-row TID that will be stored + * in an index tuple. Must only be called on a genuine heap TID, not + * on a repurposed nbtree pivot/posting t_tid. + */ +static inline void +ItemPointerSetSIUMaybeStale(ItemPointerData *pointer) +{ + Assert(pointer); + pointer->ip_posid |= ItemPointerSIUMaybeStaleFlag; } /* -- 2.50.1