Skip to content

Tepid: only update indexes that have changed attributes #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
gburd wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Tepid: only update indexes that have changed attributes #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fca24fd
Add tests to cover a variety of heap HOT update behaviors
gburd Mar 10, 2026
f090a3a
Identify modified indexed attributes in the executor on UPDATE
gburd Mar 10, 2026
7849e90
Add the HOT-indexed on-disk format: inline attr bitmap and stubs
gburd Jun 17, 2026
88ad4cb
Add HOT-indexed updates: selective index maintenance and reads
gburd Jun 17, 2026
ca29034
Fix a BitmapAnd/BitmapOr false negative on HOT-indexed fresh entries
gburd Jul 10, 2026
7588d90
Collapse dead HOT-indexed chains to xid-free stubs on prune/vacuum
gburd Jun 18, 2026
f4ccc7a
Teach amcheck to recognize HOT-indexed chains and collapse stubs
gburd Jun 18, 2026
b876bba
Add HOT-indexed statistics and the comprehensive test suite
gburd Jun 18, 2026
c9a05e5
Gate HOT-indexed updates on the logical-replication apply path
gburd Jun 18, 2026
8cd37c5
[DO NOT MERGE] Add a HOT/SIU benchmark harness
gburd Jun 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions contrib/amcheck/expected/check_heap.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,46 @@ SELECT * FROM verify_heapam('test_foreign_table',
endblock := NULL);
ERROR: cannot check relation "test_foreign_table"
DETAIL: This operation is not supported for foreign tables.
-- HOT-indexed (HOT/SIU) on-page artifacts:
--
-- A HOT-indexed UPDATE keeps the new tuple on the same page as a heap-only
-- tuple marked HEAP_INDEXED_UPDATED and plants index entries pointing at its
-- own TID. Pruning a chain of such updates collapses dead members to
-- LP_REDIRECT forwarders and preserves the LP of a live HOT-indexed member
-- whose index entries may not yet be swept. verify_heapam must treat all of
-- these as legitimate. This scenario exercises them and asserts that
-- verify_heapam reports zero corruption against legitimate HOT-indexed
-- activity.
CREATE TABLE hot_indexed_check (id int PRIMARY KEY, c1 int, c2 int, c3 int)
WITH (fillfactor = 70);
CREATE INDEX hot_indexed_check_c1 ON hot_indexed_check (c1);
CREATE INDEX hot_indexed_check_c2 ON hot_indexed_check (c2);
INSERT INTO hot_indexed_check
SELECT g, g, g, g FROM generate_series(1, 200) g;
-- Single-step UPDATEs: each row gets one HOT-indexed update. Each
-- successful HOT-indexed update keeps its new tuple on-page and inserts an
-- entry only into the index whose attribute changed.
UPDATE hot_indexed_check SET c1 = c1 + 1000;
-- Multi-step UPDATEs: drive several successive HOT-indexed updates against
-- the same rows so prune sees a chain of dead intermediates and collapses
-- them to LP_REDIRECT forwarders. An explicit VACUUM runs the prune path
-- and exercises chain collapse.
UPDATE hot_indexed_check SET c2 = c2 + 1 WHERE id <= 50;
UPDATE hot_indexed_check SET c2 = c2 + 1 WHERE id <= 50;
UPDATE hot_indexed_check SET c2 = c2 + 1 WHERE id <= 50;
VACUUM (INDEX_CLEANUP off) hot_indexed_check;
-- verify_heapam must not report any corruption against legitimate HOT-
-- indexed artifacts. Selecting the corrupting message makes any
-- regression unmistakable in the regress diff.
SELECT blkno, offnum, attnum, msg
FROM verify_heapam('hot_indexed_check',
startblock := NULL,
endblock := NULL);
blkno | offnum | attnum | msg
-------+--------+--------+-----
(0 rows)

DROP TABLE hot_indexed_check;
-- cleanup
DROP TABLE heaptest;
DROP TABLESPACE regress_test_stats_tblspc;
Expand Down
37 changes: 37 additions & 0 deletions contrib/amcheck/sql/check_heap.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,43 @@ SELECT * FROM verify_heapam('test_foreign_table',
startblock := NULL,
endblock := NULL);

-- HOT-indexed (HOT/SIU) on-page artifacts:
--
-- A HOT-indexed UPDATE keeps the new tuple on the same page as a heap-only
-- tuple marked HEAP_INDEXED_UPDATED and plants index entries pointing at its
-- own TID. Pruning a chain of such updates collapses dead members to
-- LP_REDIRECT forwarders and preserves the LP of a live HOT-indexed member
Comment on lines +144 to +146

@github-actions github-actions Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment-accuracy (low confidence): the comment says collapse "collapses dead members to LP_REDIRECT forwarders" and "preserves the LP of a live HOT-indexed member". The actual mechanism (hot_indexed.h L129-161, pruneheap.c heap_prune_record_stub) preserves dead entry-bearing members as xid-free LP_NORMAL stubs, not LP_REDIRECTs, and the root (not a live member) is redirected. Aligning the comment with the stub terminology used elsewhere would avoid confusion about what the test asserts.

-- whose index entries may not yet be swept. verify_heapam must treat all of
Comment on lines +145 to +147

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment misdescribes the artifact this test is meant to protect. Per pruneheap.c (heap_prune_chain collapse path), a dead HOT-indexed key member that still has a live index entry is preserved as an xid-free stub (an LP_NORMAL forwarding item carrying the segment's modified-attrs bitmap), not as an "LP_REDIRECT forwarder"; only the chain root is turned into an LP_REDIRECT. Stub handling (HotIndexedHeaderIsStub) is precisely the new logic added to verify_heapam.c that this test is supposed to cover, so conflating stubs with LP_REDIRECT undersells and misstates the scenario. Suggest rewording to describe stubs (and separately the root redirect).

Confidence: high.

Suggested change
-- own TID. Pruning a chain of such updates collapses dead members to
-- LP_REDIRECT forwarders and preserves the LP of a live HOT-indexed member
-- whose index entries may not yet be swept. verify_heapam must treat all of
-- own TID. Pruning a chain of such updates preserves each dead HOT-indexed
-- key member (whose index entries may not yet be swept) as an xid-free stub
-- forwarding to the next survivor, and redirects the chain root (LP_REDIRECT)
-- to the first survivor. verify_heapam must treat all of

-- these as legitimate. This scenario exercises them and asserts that
-- verify_heapam reports zero corruption against legitimate HOT-indexed
-- activity.
CREATE TABLE hot_indexed_check (id int PRIMARY KEY, c1 int, c2 int, c3 int)
WITH (fillfactor = 70);
CREATE INDEX hot_indexed_check_c1 ON hot_indexed_check (c1);
CREATE INDEX hot_indexed_check_c2 ON hot_indexed_check (c2);
INSERT INTO hot_indexed_check
SELECT g, g, g, g FROM generate_series(1, 200) g;
-- Single-step UPDATEs: each row gets one HOT-indexed update. Each
-- successful HOT-indexed update keeps its new tuple on-page and inserts an
-- entry only into the index whose attribute changed.
UPDATE hot_indexed_check SET c1 = c1 + 1000;
-- Multi-step UPDATEs: drive several successive HOT-indexed updates against
-- the same rows so prune sees a chain of dead intermediates and collapses
-- them to LP_REDIRECT forwarders. An explicit VACUUM runs the prune path
-- and exercises chain collapse.
Comment on lines +163 to +164

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment overstates coverage as a guarantee. The feature's own tests document that VACUUM collapse to LP_REDIRECT and stub creation are gated on the global xmin horizon, which a concurrent snapshot in the running regression cluster can pin back indefinitely (see contrib/pageinspect/sql/hot_indexed_updates.sql test 12: "whether the chain has collapsed to an LP_REDIRECT is not deterministic here"). So this VACUUM does not reliably "collapse them to LP_REDIRECT forwarders" or "exercise chain collapse" on any given run; the new verify_heapam stub / multi-predecessor / redirect paths this test is meant to guard may not be reached. The test still correctly passes (verify_heapam reports 0 rows regardless of layout), but the comment asserts horizon-dependent behavior as fact. Reword to reflect that collapse is best-effort here, and note that deterministic collapse coverage lives in the isolation spec. Confidence: high.

UPDATE hot_indexed_check SET c2 = c2 + 1 WHERE id <= 50;
UPDATE hot_indexed_check SET c2 = c2 + 1 WHERE id <= 50;
UPDATE hot_indexed_check SET c2 = c2 + 1 WHERE id <= 50;
Comment on lines +165 to +167

@github-actions github-actions Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test-coverage gap (moderate confidence): all three multi-step UPDATEs modify the same indexed column (c2). During chain collapse (pruneheap.c ~L1971-2000), each dead member's modified-attrs bitmap is {c2}, and laterattrs (union of later hops) also contains {c2}, so HotIndexedBitmapIsSubset() is true for every dead member -- they are all reclaimed via heap_prune_record_dead_or_unused (LP_DEAD). heap_prune_record_stub() is never reached, so the collapse-survivor stub (the LP_NORMAL non-tuple that is the principal new artifact verify_heapam had to be taught to tolerate: see verify_heapam.c L623-779) is never produced. The comment claims to exercise the preserved live-member/collapse path, but this scenario does not create a stub. To actually cover it, drive updates against different indexed columns across hops (e.g. update c1, then c2, then c1) so a dead member changed an attribute not superseded by a later hop, forcing a stub.

VACUUM (INDEX_CLEANUP off) hot_indexed_check;
Comment on lines +167 to +168

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This positive test can silently become a no-op and give false confidence. Whether the intended artifacts (LP_REDIRECT forwarders, collapse-survivor stubs, HEAP_INDEXED_UPDATED members) actually get produced depends on prune/collapse running, which in turn depends on tuple visibility and page fill. With 200 tiny 4-int rows the initial data spans only ~1-2 pages, and a concurrent snapshot (e.g. autovacuum, or the VACUUM's own horizon) can leave the dead intermediates unprunable so no chain collapse occurs. Because the test only asserts (0 rows), it passes identically whether or not the new code paths were exercised — so it cannot detect a regression that stops producing these on-page structures. Consider asserting the artifacts are actually present (e.g. via pageinspect heap_page_items to show LP_REDIRECT / stub items) before running verify_heapam, so the coverage is verifiable rather than assumed. (moderate confidence)

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment asserts VACUUM "collapses dead members to LP_REDIRECT forwarders", but VACUUM here runs against a heap whose only preceding UPDATE traffic is from earlier statements in the same session's autocommit transactions. Whether the three c2 UPDATEs on id<=50 actually build a dead intermediate chain that prune collapses depends on visibility (OldestXmin) at VACUUM time and on all three prior UPDATEs being HOT-indexed on-page. Nothing here verifies a redirect/stub was actually created, so the strong claim in the comment ("exercises chain collapse") is unverifiable from the test and may be aspirational. Either verify the redirect/stub count (pg_relation_hot_indexed_stats n_redirects/n_chains) or soften the comment to describe intent rather than asserting the outcome. (moderate confidence)

-- verify_heapam must not report any corruption against legitimate HOT-
-- indexed artifacts. Selecting the corrupting message makes any
-- regression unmistakable in the regress diff.
SELECT blkno, offnum, attnum, msg
FROM verify_heapam('hot_indexed_check',
startblock := NULL,
endblock := NULL);
Comment on lines +172 to +175

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test cannot detect whether the HOT-indexed artifacts it describes (HEAP_INDEXED_UPDATED tuples, LP_REDIRECT forwarders, collapse stubs) were actually produced. The only assertion is verify_heapam returning zero rows, which will also pass if the UPDATEs silently fell back to non-HOT updates (e.g., tuples spilling to a new page, or the feature being disabled). It therefore risks becoming a false-negative test that passes without exercising the intended prune/collapse paths. Consider adding a positive check (e.g., via pageinspect's heap_page_items to confirm the presence of LP_REDIRECT/stub items and HEAP_INDEXED_UPDATED tuples) so a regression that stops generating these artifacts is caught rather than silently masked.

Comment on lines +172 to +175

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a purely negative assertion (expected output is (0 rows)). If VACUUM does not actually collapse the chains into LP_REDIRECT forwarders / collapse-survivor stubs — e.g. because OldestXmin has not advanced past the intermediate tuples, or a concurrent snapshot holds them back — verify_heapam never reaches the new stub-forward-link and multi-predecessor-redirect branches in verify_heapam.c, yet the test still prints (0 rows) and passes. The test therefore cannot detect whether it exercised the code it claims to cover, so it can silently rot into providing no coverage. Consider asserting that the intended on-page artifacts actually exist (e.g. via pageinspect heap_page_items showing LP_REDIRECT / stub items after the VACUUM) so a regression that stops producing those artifacts is caught. Confidence: moderate.

Comment on lines +172 to +175

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds only a positive smoke test. The corresponding verify_heapam.c change introduces several new corruption-detection branches that get zero coverage here: the stub forward-link range check ("HOT-indexed stub forward link ... out of range or self-referential"), the duplicate-redirect check ("redirect line pointer points to offset ... but offset ... also points there"), and the stub-forwards-to-non-heap-only check. A change that guarantees verify_heapam does not false-positive on legitimate HOT-indexed layout does not prove it still detects real corruption in these paths. amcheck convention (see the rest of this suite / verify_nbtree tests) is to also exercise the detection paths against deliberately-corrupted pages. Please add negative coverage for the new report_corruption branches. (moderate confidence)

Comment on lines +172 to +175

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test only asserts "zero corruption rows", so it passes trivially even if none of the HOT-indexed artifacts the comment describes (HEAP_INDEXED_UPDATED on-page tuples, LP_REDIRECT forwarders, collapse-survivor stubs) are actually produced — e.g. if the update ever falls back to a non-HOT path or the SIU path is disabled. As written it cannot detect a regression where the feature silently stops taking the HOT-indexed path; it would still report 0 rows. Since the comment claims the scenario "exercises" and "asserts" these artifacts, consider adding a positive assertion that they exist (e.g. via pageinspect heap_page_items checking for LP_REDIRECT/stub items, or pg_relation_hot_indexed_stats) before running verify_heapam, so the test fails loudly if the artifacts are absent. Confidence: moderate.

Comment on lines +172 to +175

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test only asserts the absence of corruption (0 rows), but never confirms the HOT-indexed artifacts it claims to exercise were actually produced. If a future change disables the SIU path, or the page geometry causes these UPDATEs to go non-HOT (fillfactor 70 with 200 rows leaves little free space; once a page fills, subsequent updates on that page migrate off-page and are ordinary non-HOT/HOT updates, not HOT-indexed), this test still passes with 0 rows while exercising none of the LP_REDIRECT / stub / HEAP_INDEXED_UPDATED handling the comment describes. That is a silently-vacuous test: it cannot distinguish "verify_heapam handled the artifacts correctly" from "no artifacts were created". Add a positive precondition assertion (e.g. pg_stat_get_tuples_hot_indexed_updated() > 0, or pg_relation_hot_indexed_stats() showing n_hot_indexed / n_chains > 0 after the VACUUM) so the test fails loudly if the SIU path stops firing. (moderate confidence)

DROP TABLE hot_indexed_check;

-- cleanup
DROP TABLE heaptest;
DROP TABLESPACE regress_test_stats_tblspc;
Expand Down
102 changes: 92 additions & 10 deletions contrib/amcheck/verify_heapam.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "access/detoast.h"
#include "access/genam.h"
#include "access/heaptoast.h"
#include "access/hot_indexed.h"
#include "access/multixact.h"
#include "access/relation.h"
#include "access/table.h"
Expand Down Expand Up @@ -522,9 +523,12 @@ verify_heapam(PG_FUNCTION_ARGS)
*/
if (ItemIdIsRedirected(ctx.itemid))
{
OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid);
OffsetNumber rdoffnum;
ItemId rditem;

/* Resolve the redirect's target offset. */
rdoffnum = ItemIdGetRedirect(ctx.itemid);
Comment on lines +526 to +530

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minimal-diff nit: splitting OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid); into a bare declaration plus a deferred assignment and a new comment is gratuitous churn unrelated to the HOT-indexed feature. There is no goto/label between the declaration and the former initialization that would require deferring it. Per PostgreSQL minimal-diff discipline this hunk inflates the patch on untouched logic; revert it to the original one-line initializer. Confidence: high.

Suggested change
OffsetNumber rdoffnum;
ItemId rditem;
/* Resolve the redirect's target offset. */
rdoffnum = ItemIdGetRedirect(ctx.itemid);
OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid);
ItemId rditem;

Comment on lines +526 to +530

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minimal-diff hygiene: splitting OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid); into a bare declaration plus a separate assignment with a new "Resolve the redirect's target offset." comment is cosmetic churn on logic unrelated to the HOT/SIU feature. Nothing between the declaration and assignment requires the split. pgsql-hackers reviewers routinely reject unrelated reformatting of untouched lines; revert this to the original one-line initializer. (high confidence)

Suggested change
OffsetNumber rdoffnum;
ItemId rditem;
/* Resolve the redirect's target offset. */
rdoffnum = ItemIdGetRedirect(ctx.itemid);
OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid);
ItemId rditem;

Comment on lines +526 to +530

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary diff churn: OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid); was split into a bare declaration plus a separate assignment with an added comment, with no behavioral change and no use of rdoffnum in between. Minimal-diff discipline on pgsql-hackers discourages reformatting untouched logic; revert to the original single-line initializer to keep the patch focused. [high confidence]

Comment on lines +526 to +530

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minimal-diff violation: splitting OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid); into a bare declaration plus a separate assignment with a new comment is cosmetic churn unrelated to the SIU feature. This block's logic is untouched by the change. On pgsql-hackers, reformatting otherwise-untouched code invites pushback; keep the initialization inline and drop the added comment. (low confidence this is functionally required -- it isn't.)

Suggested change
OffsetNumber rdoffnum;
ItemId rditem;
/* Resolve the redirect's target offset. */
rdoffnum = ItemIdGetRedirect(ctx.itemid);
OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid);
ItemId rditem;

Comment on lines +526 to +530

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary churn: this hunk splits the existing OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid); into a bare declaration plus a later assignment and adds a comment, all in a block that is entirely unrelated to the HOT/SIU stub feature. This is functionally a no-op and touches code the change does not need. It violates the minimal-diff discipline (a top pgsql-hackers rejection reason) and should be reverted so the redirect block reads as it did before. Confidence: high.

Suggested change
OffsetNumber rdoffnum;
ItemId rditem;
/* Resolve the redirect's target offset. */
rdoffnum = ItemIdGetRedirect(ctx.itemid);
OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid);
ItemId rditem;

Comment on lines +526 to +530

@github-actions github-actions Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cosmetic churn unrelated to the SIU feature: rdoffnum was previously initialized at its declaration in a single line. Splitting it into a bare declaration plus a separate assignment with a restating comment ("Resolve the redirect's target offset.") touches otherwise-untouched code and adds a what-comment. Per minimal-diff discipline this is an avoidable review nit; revert to OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid);. (high confidence)

Suggested change
OffsetNumber rdoffnum;
ItemId rditem;
/* Resolve the redirect's target offset. */
rdoffnum = ItemIdGetRedirect(ctx.itemid);
OffsetNumber rdoffnum = ItemIdGetRedirect(ctx.itemid);
ItemId rditem;


if (rdoffnum < FirstOffsetNumber)
{
report_corruption(&ctx,
Expand Down Expand Up @@ -615,18 +619,47 @@ verify_heapam(PG_FUNCTION_ARGS)
ctx.tuphdr = (HeapTupleHeader) PageGetItem(ctx.page, ctx.itemid);
ctx.natts = HeapTupleHeaderGetNatts(ctx.tuphdr);

/* Ok, ready to check this next tuple */
check_tuple(&ctx,
&xmin_commit_status_ok[ctx.offnum],
&xmin_commit_status[ctx.offnum]);
/*
* A HOT-selectively-updated collapse-survivor stub is an
* LP_NORMAL item that is not a real tuple: HEAP_INDEXED_UPDATED
* with natts == 0, permanently invisible (HEAP_XMIN_INVALID),
* carrying a forward link and a modified-attrs bitmap. The
* per-tuple checks assume a real tuple and would misreport it, so
* skip them; the update-chain pass below still records its
* forward edge and treats it like a redirect (a forwarding node).
*/
if (!HotIndexedHeaderIsStub(ctx.tuphdr))
check_tuple(&ctx,
&xmin_commit_status_ok[ctx.offnum],
&xmin_commit_status[ctx.offnum]);

/*
* If the CTID field of this tuple seems to point to another tuple
* on the same page, record that tuple as the successor of this
* one.
* one. A collapse-survivor stub stores its forward link in the
* t_ctid offset only (the block half is repurposed to hold the
* stub's write-time natts), so resolve its successor via the stub
* accessor; the forward target is always on the same page.
*/
nextblkno = ItemPointerGetBlockNumber(&(ctx.tuphdr)->t_ctid);
nextoffnum = ItemPointerGetOffsetNumber(&(ctx.tuphdr)->t_ctid);
if (HotIndexedHeaderIsStub(ctx.tuphdr))
{
nextblkno = ctx.blkno;
nextoffnum = HotIndexedStubGetForward(ctx.tuphdr);
if (nextoffnum == ctx.offnum ||
nextoffnum < FirstOffsetNumber || nextoffnum > maxoff)
{
Comment on lines +648 to +650

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This range/self-reference check is redundant with the unchanged guard immediately below at line 663 (if (nextblkno == ctx.blkno && nextoffnum != ctx.offnum && nextoffnum >= FirstOffsetNumber && nextoffnum <= maxoff)), which re-validates the same bounds before assigning successor[ctx.offnum]. Keeping both is duplicated validation that can drift over time. Since a stub's forward target is by definition on the same page, you could rely on the existing guard and drop the duplicate bounds test here (retaining only the corruption report you want for a stub that forwards out of range). [moderate confidence]

report_corruption(&ctx,
psprintf("HOT-indexed stub forward link to item at offset %d is out of range or self-referential (valid range %d..%d)",

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corruption messages here begin with an uppercase word ("HOT-indexed"), which diverges from the PostgreSQL message style guide and from every other report_corruption() call in this file (all begin lowercase, e.g. "redirected line pointer points to...", "tuple points to new version..."). While "HOT" is an established acronym, the leading phrase "HOT-indexed stub" reads as a sentence start; prefer a lowercase lead-in for consistency, e.g. "stub forward link (HOT-indexed) ..." or reword to start lowercase. (Confidence: moderate — style only, not a functional defect.)

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corruption messages here begin with an uppercase word ("HOT-indexed"), which diverges from the PostgreSQL message style guide and from every other report_corruption() call in this file (all begin lowercase, e.g. "redirected line pointer points to...", "tuple points to new version..."). While "HOT" is an established acronym, the leading phrase "HOT-indexed stub" reads as a sentence start; prefer a lowercase lead-in for consistency, e.g. "stub forward link (HOT-indexed) ..." or reword to start lowercase. (Confidence: moderate — style only, not a functional defect.)

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corruption message convention violation: primary messages in PostgreSQL (and every other message in this file, e.g. "redirect line pointer points to offset...", "redirected line pointer points to a dead item...") start with a lowercase letter. Both new stub messages start with uppercase "HOT-indexed". Lowercase the leading word ("HOT" is fine mid-sentence as an acronym) to match the surrounding style, e.g. "HOT-indexed stub..." -> a lowercased phrasing like "stub forward link ... is out of range". This will draw objections upstream otherwise.

Comment on lines +651 to +652

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message says "out of range or self-referential" for all three OR'd conditions, but only nextoffnum == ctx.offnum is self-referential; the other two are out-of-range. When e.g. nextoffnum > maxoff the "self-referential" wording is misleading. Since nextoffnum is printed with the valid range, the reader can disambiguate, but a precise message would state the actual condition. Minor. Confidence: high.

Comment on lines +651 to +652

@github-actions github-actions Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The corruption message text is not wrapped in _(). Existing report_corruption() call sites in this file (e.g. the surrounding "redirected line pointer points to..." messages) also pass bare psprintf() strings, so this is consistent with the file's convention -- no change needed. Noting only for completeness; the message style (lowercase start, no trailing period) matches. (low confidence, likely non-issue)

nextoffnum,
FirstOffsetNumber, maxoff));
Comment on lines +651 to +654

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The single error message conflates two distinct failure modes (self-reference vs. out-of-range) and its "(valid range %d..%d)" suffix is misleading in the self-referential case, where nextoffnum can be within range yet still rejected. Splitting into two messages (one for nextoffnum == ctx.offnum, one for the range violation) would give clearer diagnostics, matching the style of the redirect checks above that report each condition separately. Confidence: low (diagnostic clarity only).

continue;
}
}
else
{
nextblkno = ItemPointerGetBlockNumber(&(ctx.tuphdr)->t_ctid);
nextoffnum = ItemPointerGetOffsetNumber(&(ctx.tuphdr)->t_ctid);
}
if (nextblkno == ctx.blkno && nextoffnum != ctx.offnum &&
nextoffnum >= FirstOffsetNumber && nextoffnum <= maxoff)
successor[ctx.offnum] = nextoffnum;
Expand Down Expand Up @@ -675,7 +708,7 @@ verify_heapam(PG_FUNCTION_ARGS)
*/
Assert(ItemIdIsNormal(next_lp));

/* Can only redirect to a HOT tuple. */
/* A redirect targets the first surviving chain member. */
next_htup = (HeapTupleHeader) PageGetItem(ctx.page, next_lp);
if (!HeapTupleHeaderIsHeapOnly(next_htup))
{
Expand All @@ -687,6 +720,32 @@ verify_heapam(PG_FUNCTION_ARGS)
/* HOT chains should not intersect. */
if (predecessor[nextoffnum] != InvalidOffsetNumber)
{
ItemId prev_lp = PageGetItemId(ctx.page,
predecessor[nextoffnum]);

/*
* In the HOT/SIU model several redirects legitimately
* forward to the same live tuple: when a chain collapses,
* the root and each entry-bearing dead member become a
* redirect to first_live so every stale btree entry still
* resolves there (the read path then rechecks the leaf
* key). Multiple predecessors are therefore expected -- but
* only in that specific shape: the target is a heap-only
* HOT-indexed survivor (verified heap-only above), and every
* predecessor forwarding to it is itself a redirect (this
* branch) or a collapse stub. Exempt only that shape; if the
* other predecessor is an ordinary tuple, or the target is
* not HOT-indexed, this is a genuine chain intersection and
* must still be reported -- do not weaken corruption
* detection to the mere presence of HEAP_INDEXED_UPDATED.
*/
if ((next_htup->t_infomask2 & HEAP_INDEXED_UPDATED) != 0 &&
(ItemIdIsRedirected(prev_lp) ||
(ItemIdIsNormal(prev_lp) &&
HotIndexedHeaderIsStub((HeapTupleHeader)
PageGetItem(ctx.page, prev_lp)))))
continue;
Comment on lines +742 to +747

@github-actions github-actions Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asymmetric exemption: the redirect-fan-in branch here exempts a legitimate SIU collapse shape (target has HEAP_INDEXED_UPDATED and the other predecessor is a redirect or stub), but the ordinary-tuple intersection branch below ("tuple points to new version at offset %d, but offset %d also points there", ~line 805) has no equivalent exemption. Because the stub branch records predecessor[nextoffnum] = ctx.offnum when it runs before an ordinary-tuple edge to the same target, a stub processed first can set predecessor[L], and a later ordinary xmax/xmin edge to L would then be reported as a false chain intersection. If a stub and an ordinary update-chain edge can legitimately both resolve to the same live tuple L, this produces a spurious corruption report depending purely on offset iteration order. Confirm whether that state is reachable; if so, the ordinary-tuple branch needs the same stub/redirect exemption. (moderate confidence)


report_corruption(&ctx,
psprintf("redirect line pointer points to offset %d, but offset %d also points there",
nextoffnum, predecessor[nextoffnum]));
Expand All @@ -701,6 +760,30 @@ verify_heapam(PG_FUNCTION_ARGS)
continue;
}

/*
* A collapse-survivor stub forwards like a redirect: it is not a
* real tuple, so don't apply the tuple-to-tuple update-chain
* checks, but do record the predecessor edge to its target so the
* live tuple it ultimately forwards to is not mistaken for a
* chain root. Its target must be heap-only (another stub or the
* live heap-only tuple).
*/
curr_htup = (HeapTupleHeader) PageGetItem(ctx.page, curr_lp);
if (HotIndexedHeaderIsStub(curr_htup))
{
if (ItemIdIsNormal(next_lp))
{
next_htup = (HeapTupleHeader) PageGetItem(ctx.page, next_lp);
if (!HeapTupleHeaderIsHeapOnly(next_htup))
Comment on lines +774 to +777

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a stub's forward target next_lp is not LP_NORMAL, this branch silently does nothing and continues without recording the predecessor edge and without reporting anything. A stub forwarding to a redirect or dead line pointer is not a shape the writer produces (pruneheap sets the forward offset to an LP_NORMAL heap-only key tuple/stub), so an ItemIdIsRedirected(next_lp)/otherwise-invalid target here is itself a corruption that amcheck now fails to report. Consider reporting corruption in the else case (e.g. "HOT-indexed stub forwards to a non-normal line pointer at offset %d") to avoid a detection gap. Confidence: moderate.

Comment on lines +774 to +777

@github-actions github-actions Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a stub's forward target next_lp is not LP_NORMAL (a redirect, dead, or unused item), this branch silently continues without recording a predecessor edge or reporting anything. The producer (heap_prune_record_stub in pruneheap.c) always forwards a stub to next_survivor, which is a heap-only LP_NORMAL tuple or another stub, never a redirect -- so a stub forwarding to a redirect is itself an unexpected/corrupt shape. amcheck silently accepting it means this corruption case is not detected. Consider reporting corruption for a stub forwarding to a non-normal item, mirroring the non-heap-only check. (moderate confidence)

report_corruption(&ctx,
psprintf("HOT-indexed stub forwards to a non-heap-only tuple at offset %d",

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same message-convention issue: this primary message starts with uppercase "HOT-indexed" while all other report_corruption() messages in this file begin lowercase. Reword to start lowercase for consistency.

nextoffnum));
else if (predecessor[nextoffnum] == InvalidOffsetNumber)
predecessor[nextoffnum] = ctx.offnum;
}
continue;
}

/*
* If the next line pointer is a redirect, or if it's a tuple but
* the XMAX of this tuple doesn't match the XMIN of the next
Expand All @@ -709,7 +792,6 @@ verify_heapam(PG_FUNCTION_ARGS)
*/
if (ItemIdIsRedirected(next_lp))
continue;
curr_htup = (HeapTupleHeader) PageGetItem(ctx.page, curr_lp);
curr_xmax = HeapTupleHeaderGetUpdateXid(curr_htup);
next_htup = (HeapTupleHeader) PageGetItem(ctx.page, next_lp);
next_xmin = HeapTupleHeaderGetXmin(next_htup);
Expand Down
41 changes: 41 additions & 0 deletions contrib/amcheck/verify_nbtree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1489,6 +1489,33 @@ 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. Posting-list tuples can carry the marker too -- nbtree
* deduplication copies a flagged heap TID verbatim into a posting
* list's heap-TID array -- so those are stripped per element below,
* on the palloc'd plain tuple bt_posting_plain_tuple() produces.
*/
if (!BTreeTupleIsPosting(itup) &&
ItemPointerIsSIUMaybeStale(&itup->t_tid))
Comment on lines +1504 to +1511

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug (high confidence): posting-list heap TIDs can carry the SIU may-be-stale flag, but this branch strips it only for plain (non-posting) leaf tuples, so posting lists are left unstripped.

Deduplication merges flagged TIDs into posting lists verbatim: _bt_form_posting() does memcpy(BTreeTupleGetPosting(...), htids, ...) / ItemPointerCopy() without masking, and the ordering Assert(_bt_posting_valid()) uses ItemPointerCompare(), which strips the flag — so a flagged TID passes every dedup check and is stored with the flag set. Here the posting branch feeds each element straight through bt_posting_plain_tuple() -> bt_normalize_tuple() -> bloom_add_element() with the flag still in t_tid, while bt_tuple_present_callback() probes with the plain (unflagged) heap TID from the scan. The fingerprints won't match and heapallindexed will raise a spurious "lacks matching index tuple" corruption error.

The comment's claim "Only plain leaf tuples can carry the marker" directly contradicts itemptr.h ("including each individual entry of a posting list's heap-TID array"). The flag must be stripped from each posting element's TID before fingerprinting — e.g. inside the per-element loop after bt_posting_plain_tuple(), apply ItemPointerSetOffsetNumber(&logtuple->t_tid, ItemPointerGetOffsetNumber(&logtuple->t_tid)) (logtuple is a fresh palloc'd copy, so no save/restore is needed there).

{
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;
Comment on lines +1513 to +1517

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The strip relies on a non-obvious side effect: ItemPointerGetOffsetNumber() internally calls ItemPointerOffsetNumberStrip() (itemptr.h) which clears the SIU bit, then it is written back via the setter. This works, but reads as an identity round-trip and obscures intent — a maintainer who does not know the getter strips the bit will assume this line is a no-op and may "simplify" it away, silently reintroducing the false-positive heapallindexed reports. Call the strip helper directly to make the intent explicit.

Suggested change
saved_tid = itup->t_tid;
ItemPointerSetOffsetNumber(&itup->t_tid,
ItemPointerGetOffsetNumber(&itup->t_tid));
siu_stripped = true;
saved_tid = itup->t_tid;
ItemPointerSetOffsetNumber(&itup->t_tid,
ItemPointerOffsetNumberStrip(ItemPointerGetOffsetNumberNoCheck(&itup->t_tid)));
siu_stripped = true;

Comment on lines +1513 to +1517

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This strip idiom is obscure and fragile. ItemPointerSetOffsetNumber(&itup->t_tid, ItemPointerGetOffsetNumber(&itup->t_tid)) reads like a no-op; it only clears the SIU flag because ItemPointerGetOffsetNumber() internally calls ItemPointerOffsetNumberStrip() while ItemPointerSetOffsetNumber() stores verbatim. If ItemPointerGetOffsetNumber() ever stops stripping, this silently breaks with no compile error. Since ItemPointerSetSIUMaybeStale() exists as the setter, consider adding/using a symmetric clear helper (e.g. ItemPointerClearSIUMaybeStale() or applying ItemPointerOffsetNumberStrip() explicitly) so the intent is self-documenting.

Comment on lines +1513 to +1517

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stripping idiom is obscure and error-prone: at a glance ItemPointerSetOffsetNumber(x, ItemPointerGetOffsetNumber(x)) reads like a no-op. It only works because ItemPointerGetOffsetNumber() happens to call ItemPointerOffsetNumberStrip() internally. Prefer the documented, intent-revealing helper: ItemPointerSetOffsetNumber(&itup->t_tid, ItemPointerOffsetNumberStrip(ItemPointerGetOffsetNumberNoCheck(&itup->t_tid)));. This also makes the sentinel-preserving contract explicit at the call site.

Suggested change
saved_tid = itup->t_tid;
ItemPointerSetOffsetNumber(&itup->t_tid,
ItemPointerGetOffsetNumber(&itup->t_tid));
siu_stripped = true;
saved_tid = itup->t_tid;
ItemPointerSetOffsetNumber(&itup->t_tid,
ItemPointerOffsetNumberStrip(ItemPointerGetOffsetNumberNoCheck(&itup->t_tid)));
siu_stripped = true;

}
Comment on lines +1510 to +1518

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix only strips the SIU marker from plain leaf tuples (!BTreeTupleIsPosting(itup)), but the marker also lands in posting-list tuples. ItemPointerSetSIUMaybeStale() (execIndexing.c) sets the flag on a heap TID handed to index_insert(); that TID becomes an ordinary leaf tuple's t_tid, which nbtree deduplication (_bt_dedup_start_pending/_bt_dedup_save_htid in nbtdedup.c) copies verbatim into a posting list's heap-TID array without stripping. itemptr.h explicitly documents (lines 98-100) that each posting-list heap-TID array entry is an ordinary TID that can carry this flag. When such a posting tuple is fingerprinted below via bt_posting_plain_tuple(), the per-element TID still carries bit 14 and will not match the plain heap TID from bt_tuple_present_callback(), producing exactly the false-positive bt_index_check failure this change is trying to prevent. The strip must be applied to the plain tuples produced by bt_posting_plain_tuple() as well, not only to non-posting leaf tuples. The comment "Only plain leaf tuples can carry the marker" contradicts itemptr.h and is incorrect. (moderate confidence: assumes dedup can merge SIU-flagged entries, which nothing in the visible code prevents.)

Comment on lines +1510 to +1518

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix only strips the SIU marker from plain leaf tuples (!BTreeTupleIsPosting(itup)), but the marker also lands in posting-list tuples. ItemPointerSetSIUMaybeStale() (execIndexing.c) sets the flag on a heap TID handed to index_insert(); that TID becomes an ordinary leaf tuple's t_tid, which nbtree deduplication (_bt_dedup_start_pending/_bt_dedup_save_htid in nbtdedup.c) copies verbatim into a posting list's heap-TID array without stripping. itemptr.h explicitly documents (lines 98-100) that each posting-list heap-TID array entry is an ordinary TID that can carry this flag. When such a posting tuple is fingerprinted below via bt_posting_plain_tuple(), the per-element TID still carries bit 14 and will not match the plain heap TID from bt_tuple_present_callback(), producing exactly the false-positive bt_index_check failure this change is trying to prevent. The strip must be applied to the plain tuples produced by bt_posting_plain_tuple() as well, not only to non-posting leaf tuples. The comment "Only plain leaf tuples can carry the marker" contradicts itemptr.h and is incorrect. (moderate confidence: assumes dedup can merge SIU-flagged entries, which nothing in the visible code prevents.)

Comment on lines +1510 to +1518

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SIU marker is only stripped for plain leaf tuples (!BTreeTupleIsPosting(itup)), but the comment's claim that "Only plain leaf tuples can carry the marker" is wrong. Per storage/itemptr.h (lines 98-100), the marker "applies to a genuine heap-row TID (including each individual entry of a posting list's heap-TID array...)". Deduplication copies the raw t_tid verbatim into the posting list (nbtdedup.c _bt_dedup_save_htid, memcpy(state->htids + ..., htids, ...)), so a HOT-indexed fresh entry that gets deduplicated retains its marker bit inside a posting tuple.

In the posting branch below, bt_posting_plain_tuple() -> _bt_form_posting() copies that marked TID verbatim, then bt_normalize_tuple/bloom_add_element fingerprints the full IndexTuple bytes including t_tid. The heap-scan side (bt_tuple_present_callback) fingerprints the plain (stripped) heap TID it gets from the scan, so the fingerprints will not match — producing false-positive "heap tuple ... lacks matching index tuple" corruption reports whenever a HOT-indexed entry lands in a deduplicated posting list.

The marker must also be stripped for each posting-list element (e.g. strip the marker on each logtuple->t_tid before bt_normalize_tuple, or extract stripped TIDs). [high confidence]

Comment on lines +1510 to +1518

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug (high confidence): the SIU marker is not stripped for posting-list entries, causing false-positive heapallindexed corruption reports.

The guard !BTreeTupleIsPosting(itup) means posting-list tuples never get their marker stripped. But per storage/itemptr.h (the comment at lines 95-100: "including each individual entry of a posting list's heap-TID array"), a posting entry can carry ItemPointerSIUMaybeStaleFlag: a HOT-indexed fresh entry is inserted with the flag set (execIndexing.c ItemPointerSetSIUMaybeStale), and nbtree deduplication later memcpy's that flagged TID verbatim into a posting list's heap-TID array (_bt_form_posting).

In the posting branch below, bt_posting_plain_tuple() -> _bt_form_posting(..., 1) copies the raw flagged TID (ItemPointerCopy at nbtdedup.c:906) into logtuple->t_tid, so the marker bit lands in the bloom-filter fingerprint. The probe side (bt_tuple_present_callback) always fingerprints an unmarked heap TID from the heap scan, so the images will not match and verification wrongly reports "heap tuple ... lacks matching index tuple".

The strip must be applied per posting element (on each logtuple->t_tid before bt_normalize_tuple), not gated on !BTreeTupleIsPosting.

Comment on lines +1510 to +1518

@github-actions github-actions Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SIU-marker strip is open-coded in two places here (this inline block and the per-element block in the posting loop below), each pairing ItemPointerIsSIUMaybeStale() with ItemPointerOffsetNumberStrip() on the raw ip_posid. bt_normalize_tuple() is the routine whose entire purpose is to normalize away index-vs-heap representation differences that would otherwise break heapallindexed; the SIU marker is exactly such a difference. Consider folding the strip into bt_normalize_tuple() (operating on a copy when it would otherwise return itup unchanged) so both callers get it for free and the save/restore dance disappears. As written the logic is duplicated and the correctness of the whole scheme depends on the inline strip and the restore staying in sync with the posting-loop strip. (maintainability/DRY; moderate confidence)


if (BTreeTupleIsPosting(itup))
{
Expand All @@ -1498,6 +1525,16 @@ bt_target_page_check(BtreeCheckState *state)
IndexTuple logtuple;

logtuple = bt_posting_plain_tuple(itup, i);
/*
* A posting element's heap TID may carry the SIU
* may-be-stale marker (see above); strip it on this
* palloc'd copy so its fingerprint matches the plain heap
* TID from the heap scan. logtuple is a fresh tuple, so
* no restore is needed.
*/
if (ItemPointerIsSIUMaybeStale(&logtuple->t_tid))
logtuple->t_tid.ip_posid =
ItemPointerOffsetNumberStrip(logtuple->t_tid.ip_posid);
Comment on lines +1535 to +1537

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ItemPointerIsSIUMaybeStale() guard here is redundant: ItemPointerOffsetNumberStrip() is already a no-op when the marker bit isn't set (it just masks bit 14, and sentinels pass through unchanged per itemptr.h). The strip can be applied unconditionally, which reads more simply and matches the sibling code in contrib/pageinspect/btreefuncs.c that strips without a prior ItemPointerIsSIUMaybeStale() test. Low confidence this matters for correctness (it doesn't); purely a readability/consistency nit.

Suggested change
if (ItemPointerIsSIUMaybeStale(&logtuple->t_tid))
logtuple->t_tid.ip_posid =
ItemPointerOffsetNumberStrip(logtuple->t_tid.ip_posid);
logtuple->t_tid.ip_posid =
ItemPointerOffsetNumberStrip(logtuple->t_tid.ip_posid);

norm = bt_normalize_tuple(state, logtuple);
bloom_add_element(state->filter, (unsigned char *) norm,
IndexTupleSize(norm));
Expand All @@ -1516,6 +1553,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;
}

/*
Expand Down
32 changes: 32 additions & 0 deletions contrib/bloom/expected/bloom.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
28 changes: 28 additions & 0 deletions contrib/bloom/sql/bloom.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +100 to +102

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This 14-line comment over-narrates implementation history ("previously a false negative before the SIU bitmap fix", "handled centrally in tbm_add_tuples ... needs no bloom-specific code"). PostgreSQL comment discipline: describe what the test does now and why, concisely. Trim to a couple of lines stating the scenario (HOT-indexed update leaves the bloom-indexed column unchanged; a BitmapAnd must still return the row). (low confidence)

-- 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"

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test assumes the UPDATE takes the HOT-indexed (SIU) path, but nothing verifies it actually did. HeapUpdateHotAllowable classifies the update, yet heap_update() still falls back to a non-HOT update if the new tuple does not fit on the page. fillfactor = 50 on a nearly empty page makes SIU likely but not guaranteed across all BLCKSZ builds, and a fallback would leave the row reachable by a plain single-index scan — again masking a regression. Consider asserting the update was HOT-indexed (e.g. via the pageinspect/pgstat HOT-indexed counters added elsewhere in this change) so a silent fallback fails the test loudly.

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test never verifies that the UPDATE actually took the HOT-indexed (SIU) path, so it can pass without exercising the behavior it claims to guard. If the update falls back to a plain HOT or non-HOT update (e.g. page layout changes, or future planner/AM changes), the bloom entry would point at the new tuple / the row would still be found, and this query returns 1 regardless -- a silent false pass. The established pattern for this feature (contrib/pageinspect/sql/hot_indexed_updates.sql) always asserts the path fired via pg_stat_get_tuples_hot_indexed_updated()/get_hi_count(). Add such an assertion here (e.g. SELECT pg_stat_force_next_flush(); then check the HOT-indexed counter advanced) before the correctness query. Confidence: high.

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;
Comment on lines +118 to +119

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test does not verify that a BitmapAnd node is actually chosen, so it may not exercise the intersection code path it claims to guard. With a single-row, single-page table, the planner can satisfy this query with a single BitmapIndexScan on one column while applying the other condition as a recheck/filter, never producing a BitmapAnd. In that case the SIU lossy-degradation path in tbm_add_tuples (which only matters when two bitmaps are intersected) is never hit, yet the test still returns 1 and passes — giving false coverage for the very fix it protects. The other tests in this file (lines 24-26, 68-70) confirm plan shape with EXPLAIN (COSTS OFF); do the same here and assert a BitmapAnd node appears. Consider also inserting enough rows that a BitmapAnd is genuinely favored. Confidence: moderate.

Comment on lines +118 to +119

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test does not verify that a BitmapAnd node is actually chosen, so it may not exercise the intersection code path it claims to guard. With a single-row, single-page table, the planner can satisfy this query with a single BitmapIndexScan on one column while applying the other condition as a recheck/filter, never producing a BitmapAnd. In that case the SIU lossy-degradation path in tbm_add_tuples (which only matters when two bitmaps are intersected) is never hit, yet the test still returns 1 and passes — giving false coverage for the very fix it protects. The other tests in this file (lines 24-26, 68-70) confirm plan shape with EXPLAIN (COSTS OFF); do the same here and assert a BitmapAnd node appears. Consider also inserting enough rows that a BitmapAnd is genuinely favored. Confidence: moderate.

@github-actions github-actions Bot Jul 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test does not verify that a BitmapAnd plan is actually chosen, so it may silently stop guarding the fix it targets. The scenario relies on tbm_add_tuples degrading the SIU fresh entry's TID to a lossy page during a two-index intersection. But bloom_siu has only a single row, so with enable_bitmapscan=on/enable_indexscan=off the planner is very likely to pick a single bitmap index scan (e.g. on bloom_siu_c for changed = 22) and apply bcol = 11 as a heap recheck — never forming a BitmapAnd node. In that case the guarded intersection path is never exercised, yet the query still returns the correct row via the single-scan recheck, so the test passes with or without the SIU bitmap fix. Add an EXPLAIN (COSTS OFF) assertion (as the existing tests at lines 24-26 and 68-70 do) to pin the BitmapAnd shape; consider inserting enough non-matching rows so a BitmapAnd is genuinely the cheaper plan rather than relying on a 1-row table.

Suggested change
SELECT count(*) AS bloom_bitmapand FROM bloom_siu WHERE bcol = 11 AND changed = 22;
EXPLAIN (COSTS OFF) SELECT count(*) FROM bloom_siu WHERE bcol = 11 AND changed = 22;
SELECT count(*) AS bloom_bitmapand FROM bloom_siu WHERE bcol = 11 AND changed = 22;

Comment on lines +118 to +119

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test claims to guard BitmapAnd/SIU intersection correctness, but it never verifies that a BitmapAnd plan is actually chosen, and with a single-row table the planner will almost certainly not pick one. Without ANALYZE and with only one tuple, the optimizer is free to choose a single Bitmap Index Scan (applying the other qualifier as a heap recheck/filter) or another shape entirely; in that case tbm_add_tuples' lossy-page intersection path is never exercised and the test passes trivially, guarding nothing. Every other bitmap test in this file pins the plan with EXPLAIN (COSTS OFF); this one should do the same so a regression to a non-BitmapAnd plan is caught. Consider adding an EXPLAIN (COSTS OFF) that shows BitmapAnd over both index scans (and enough rows/ANALYZE to make that plan reliably cheapest). Confidence: high.

Comment on lines +115 to +119

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test claims to guard the SIU BitmapAnd intersection path, but it never verifies a BitmapAnd plan is actually chosen. With a single-row table the planner is very likely to pick a single BitmapIndexScan (e.g. on bloom_siu_c) with a heap recheck rather than a BitmapAnd of both indexes; the btree changed entry already points at the new HOT tuple, so a single-index scan trivially returns the row. This makes the test a probable false pass that does not exercise the lossy-page degradation / heap-recheck code the comment describes. Add an EXPLAIN (COSTS OFF) before the SELECT to pin the BitmapAnd shape (and force the intersection), otherwise the test does not guard the fix. (high confidence)

Suggested change
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;
SET enable_seqscan = off;
SET enable_indexscan = off;
SET enable_bitmapscan = on;
-- Confirm the plan is actually a BitmapAnd of both indexes so the intersection
-- path (not a single-index scan) is what is being tested.
EXPLAIN (COSTS OFF)
SELECT count(*) FROM bloom_siu WHERE bcol = 11 AND changed = 22;
-- 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;

@github-actions github-actions Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment centers on a BitmapAnd, but nothing pins the plan. With enable_indexscan off and two eligible bitmap-index-scan candidates the planner should produce a BitmapAnd, but that is not guaranteed (costs can make it pick a single BitmapIndexScan with the other qual as a recheck/filter). If it does, the row is still returned and count is 1, so the specific BitmapAnd/tbm intersection path the comment describes goes untested. Add EXPLAIN (COSTS OFF) for this query to lock the BitmapAnd plan, matching the discipline in hot_indexed_updates.sql. Confidence: moderate.

Comment on lines +118 to +119

@github-actions github-actions Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is meant to guard the SIU BitmapAnd intersection path in tbm_add_tuples, but nothing here verifies that a BitmapAnd plan is actually chosen. On a single-row table the planner will very likely pick a single BitmapIndexScan (on bloom_siu_b or bloom_siu_c) plus a heap recheck of the other qual, which never exercises the two-bitmap intersection this test claims to cover. The query would still return 1 even if the SIU-lossy-page fix regressed, so this is a silent false-positive guard. Every sibling test in this feature (e.g. contrib/pageinspect/sql/hot_indexed_updates.sql) pins the plan shape with EXPLAIN (COSTS OFF) first; do the same here and assert the plan is a BitmapAnd of the two bitmap index scans. Confidence: high.

Suggested change
-- 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;
-- Confirm the plan is actually a BitmapAnd of both index bitmaps; otherwise
-- this test does not exercise the SIU intersection path it is meant to guard.
EXPLAIN (COSTS OFF)
SELECT count(*) FROM bloom_siu WHERE bcol = 11 AND changed = 22;
-- 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;
Loading
Loading