From 7fdcb90abfc5caff810962702e3a404eb8b6173f Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Fri, 10 Jul 2026 15:33:34 -0400 Subject: [PATCH v63 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 | 28 +++++ contrib/bloom/expected/bloom.out | 32 +++++ contrib/bloom/sql/bloom.sql | 28 +++++ contrib/pageinspect/Makefile | 2 +- contrib/pageinspect/btreefuncs.c | 53 ++++++++- 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/executor/execIndexing.c | 52 +++++++-- src/backend/nodes/tidbitmap.c | 37 +++++- src/backend/storage/page/itemptr.c | 18 ++- src/include/storage/itemptr.h | 109 +++++++++++++++++- 14 files changed, 421 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..c1bfdcc133c 100644 --- a/contrib/amcheck/verify_nbtree.c +++ b/contrib/amcheck/verify_nbtree.c @@ -1489,6 +1489,30 @@ 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; + /* Strip on the raw offset (no validity assert); restore below. */ + itup->t_tid.ip_posid = + ItemPointerOffsetNumberStrip(itup->t_tid.ip_posid); + siu_stripped = true; + } if (BTreeTupleIsPosting(itup)) { @@ -1516,6 +1540,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..2eeee3e0e19 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,35 @@ 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); + /* + * Strip the SIU marker directly on the raw offset. Do not route + * through ItemPointerGetOffsetNumber(), which asserts a valid (nonzero) + * offset: this function also inspects arbitrary raw page images, where + * a corrupt ip_posid == 0 must be shown, not trip an assertion. + */ + ctid_show.ip_posid = ItemPointerOffsetNumberStrip(ctid_show.ip_posid); + 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 +615,13 @@ 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; + /* Strip on the raw offset; see the ctid path for why not the getter. */ + htid_show.ip_posid = ItemPointerOffsetNumberStrip(htid_show.ip_posid); + values[j++] = ItemPointerGetDatum(&htid_show); + } else nulls[j++] = true; @@ -606,6 +643,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/executor/execIndexing.c b/src/backend/executor/execIndexing.c index ebc4422accc..467b4903ad2 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..cf6948125e3 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,53 @@ 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); + /* + * The marker must only ever land on a genuine, in-range heap offset: a + * value in the sentinel range (SpecTokenOffsetNumber / MovedPartitions- + * OffsetNumber) already has bit 14 set and would be corrupted, and + * ItemPointerOffsetNumberStrip deliberately leaves such values untouched, + * so a later strip would not clear the flag. Catch misuse in debug builds. + */ + Assert(ItemPointerGetOffsetNumberNoCheck(pointer) < MovedPartitionsOffsetNumber); + pointer->ip_posid |= ItemPointerSIUMaybeStaleFlag; } /* -- 2.50.1