amcheck: add index-all-keys-match verification for B-Tree

Hello

+ /* Only verify tuples pointing to visible heap rows */
+ if (!heap_entry_is_visible(state, tid))
+ return;
...
+ slot = table_slot_create(state->heaprel, NULL);
+ found = table_tuple_fetch_row_version(state->heaprel, tid,
+   state->snapshot, slot);
+ if (!found)
+ {

This seems like a duplication, heap_entry_is_visible does the same
thing and returns found.

This also means that the if (!found) block should be unreachable?

Wouldn't it be simpler to remove the is_visible check completely and
use an if(found) later?

+ indexinfo = state->indexinfo;
+ estate = CreateExecutorState();
+ GetPerTupleExprContext(estate)->ecxt_scantuple = slot;
+ FormIndexDatum(indexinfo, slot, estate, values, isnull);
+ FreeExecutorState(estate);

This doesn't need the same cleanup code as heapam.c:1754 and :1997?
Seems like state remains, so we have dangling pointers there and could
crash later.

/* These may have been pointing to the now-gone estate */
indexInfo->ii_ExpressionsState = NIL;
indexInfo->ii_PredicateState = NULL;

Hi Zsolt!

Many thanks for the review!

On 26 Feb 2026, at 04:18, Zsolt Parragi <zsolt.parragi@percona.com> wrote:

Hello

+ /* Only verify tuples pointing to visible heap rows */
+ if (!heap_entry_is_visible(state, tid))
+ return;
...
+ slot = table_slot_create(state->heaprel, NULL);
+ found = table_tuple_fetch_row_version(state->heaprel, tid,
+   state->snapshot, slot);
+ if (!found)
+ {

This seems like a duplication, heap_entry_is_visible does the same
thing and returns found.

This also means that the if (!found) block should be unreachable?

Wouldn't it be simpler to remove the is_visible check completely and
use an if(found) later?

Yup, done so.

Also thinking about it more, I decided to add more corruption checks here:
non-existent tuples are also kind of corruption that must be reported.
This issue addressed in patch 2 so you can verify your notes were addressed.

I have a gut feeling that we might do without snapshot at all...

+ indexinfo = state->indexinfo;
+ estate = CreateExecutorState();
+ GetPerTupleExprContext(estate)->ecxt_scantuple = slot;
+ FormIndexDatum(indexinfo, slot, estate, values, isnull);
+ FreeExecutorState(estate);

This doesn't need the same cleanup code as heapam.c:1754 and :1997?
Seems like state remains, so we have dangling pointers there and could
crash later.

/* These may have been pointing to the now-gone estate */
indexInfo->ii_ExpressionsState = NIL;
indexInfo->ii_PredicateState = NULL;

Yup, fixed and added tests exercising ii_ExpressionsState.

Thanks!

Best regards, Andrey Borodin.

On 26 Feb 2026, at 10:28, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

I have a gut feeling that we might do without snapshot at all...

I decided that it's a good idea to verify only visible tuples.
Mismatch of dead tuples might be bad sign, but it's not a corruption.
So we need a snapshot.

PFA patch with documentation.

Thanks!

Best regards, Andrey Borodin.

I don't see any other logic problems in the code, I only have a few
minor comments/questions:

+ * Bloom filter says (key, tid) not in heap.  Follow TID to verify; this
+ * amortizes random heap lookups when the filter has false negatives, or

This comment could be a bit confusing, as bloom filters typically have
false positives, not negatives.
Maybe it would be better to phrase this somehow differently?

Another thing is that now amcheck can create two bloom filters,
allocated at the same time, both up to maintenance_work_mem.
Isn't that a detail that should be at least mentioned somewhere?

And in the documentation, shouldn't this new check mention something
similar to heapallindexed, which describes the check possibly missing
corruption because of the bloom filter?

+ (errcode(ERRCODE_INDEX_CORRUPTED),
+ errmsg("index tuple in index \"%s\" does not match heap tuple",
+ RelationGetRelationName(state->rel)),

Shouldn't this also print out the table name?

On 3 Mar 2026, at 04:19, Zsolt Parragi <zsolt.parragi@percona.com> wrote:

I don't see any other logic problems in the code, I only have a few
minor comments/questions:

+ * Bloom filter says (key, tid) not in heap.  Follow TID to verify; this
+ * amortizes random heap lookups when the filter has false negatives, or

This comment could be a bit confusing, as bloom filters typically have
false positives, not negatives.
Maybe it would be better to phrase this somehow differently?

Another thing is that now amcheck can create two bloom filters,
allocated at the same time, both up to maintenance_work_mem.
Isn't that a detail that should be at least mentioned somewhere?

And in the documentation, shouldn't this new check mention something
similar to heapallindexed, which describes the check possibly missing
corruption because of the bloom filter?

+ (errcode(ERRCODE_INDEX_CORRUPTED),
+ errmsg("index tuple in index \"%s\" does not match heap tuple",
+ RelationGetRelationName(state->rel)),

Shouldn't this also print out the table name?

Thanks for the review! I've addressed all these issues in patch steps 4 and 5. Other steps are intact. PFA.

Best regards, Andrey Borodin.

Hi Andrey
I've been reviewing the patch and ran some concurrent tests against it.
I found an issue with false positive corruption reports under concurrent
VACUUM.

+ slot = table_slot_create(state->heaprel, NULL);
+ found = table_tuple_fetch_row_version(state->heaprel, tid,
+                         SnapshotAny, slot);
+ if (!found)
+ {
+         ExecDropSingleTupleTableSlot(slot);
+         ereport(ERROR,

bt_verify_index_tuple_points_to_heap uses SnapshotAny with
table_tuple_fetch_row_version to distinguish "tuple doesn't exist" from
"tuple exists but is dead". However, bt_index_check only holds
AccessShareLock which compatible with VACUUM's ShareUpdateExclusiveLock.
A concurrent VACUUM Phase 1 can prune heap pages(ItemIdSetDead) between
Bloom filter probe and the heap lookup. Causing
table_tuple_fetch_row_version to return false for a legal
dead-but-now-pruned tuple and a false positive corruption report.

The table_tuple_satisfies_snapshot check does not help, it only runs
when the fetch succeeds(LP_NORMAL). Once VACUUM sets LP_DEAD,
heapam_fetch sees !ItemIdIsNormal(lp) and returns false before any
snapshot checks.

The reproduce should be: DELETE all rows form a big table, then launch
VACUUM and bt_index_check concurrently. A POC test script attached.

On Thu, Mar 5, 2026 at 7:00 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Thanks for the review! I've addressed all these issues in patch steps 4 and 5. Other steps are intact. PFA.

Hi Andrey and Wenboo,

1. I tried the v4 to see how it would cope with heap missing segments[0]/messages/by-id/013D63E2-5D75-492E-85FF-1D5CC0148C82@gmail.com
(with -Dsegsize_blocks=6), so given:

create table ptest(id bigint, tenant_id bigint);
insert into ptest select g, mod(g,10) from generate_series(1, 3000) g;
alter table ptest add primary key (id);
checkpoint;

postgres=# select count(*) from ptest;
count
-------
3000

-- simulate loss of middle segment:
$ mv base/5/24578.1 base/5/24578.1.ORG
$ ls -1 base/5/24578*
base/5/24578
base/5/24578.1.ORG
base/5/24578.2
base/5/24578_fsm
base/5/24578_vm

-- force _mfd_openseg() via new backend to show silent data loss:
postgres=# select count(*) from ptest;
count
-------
1110

-- try new amcheck
postgres=# select bt_index_check(index=>'ptest_pkey',
heapallindexed=>true, checkunique=>true, indexallkeysmatch=>false);
bt_index_check
----------------

-- try the new indexallkeysmatch
-- might need restart before it is able to properly bailout
postgres=# select bt_index_check(index=>'ptest_pkey',
heapallindexed=>true, checkunique=>true, indexallkeysmatch=>true);
ERROR: could not open file "base/5/24578.1" (target block 6): No such
file or directory

this error was not quite expected for me (altough it is much better than nothing
as on master today), because patch v4-0002 assumes in
bt_verify_index_tuple_points_to_heap() that we should get false and error in
more human-firendly way ("index tuple points to non-existent heap"). The error
itself is coming out of the following stacktrace:

_mdfd_getseg (forknum=MAIN_FORKNUM, blkno=6)
<- mdstartreadv (..) <- smgrstartreadv (blocknum=blocknum@entry=6 (!))
<- StartReadBuffersImpl (blockNum=6 (!)) <- StartReadBuffer()
<- ReadBuffer_common() <- ReadBufferExtended (blockNum=6 (!))
<- heap_fetch() <- heapam_fetch_row_version()
<- table_tuple_fetch_row_version()
<- bt_verify_index_tuple_points_to_heap(targetblock=5, offset=14)
<- bt_target_page_check() <- bt_check_level_from_leftmost()
<- bt_check_every_level() <- bt_index_check_callback()
<- amcheck_lock_relation_and_check() <- bt_index_check()
and it's because itup in bt_target_page_check() has bi_lo = 6, ip_posid = 1

So with the attached patch it becomes more human understandable, but it is
somehow orthogonal check? Still maybe we can combine this under this $thread
ambrella and make it as an option of indexallkeysmatch too? Attached
behaves likes this:

postgres=# select bt_index_check(index=>'ptest_pkey');
ERROR: index line pointer in index "ptest_pkey" points to missing
page in table "ptest"
DETAIL: Index tid=(5,14) points to heap tid=(6,1) but heap has only 6 blocks.
HINT: this can be caused by lost relation segment (missing or removed file).

(note it does not need to indexallkeysmatch, but it's related)

Also, the attached patch right now blows up pg_amcheck test (not
contrib/amcheck), where we ask to ignore certain corrupted schemas/tables to
test exclusion logic, but we still verify btrees and with attached this btree
verification ends up discovering short heap segment :D I honestly do not know
what to do in this situation (we ask to ignore one table T, but still launch
bt_index_check() on it's index and discover illegal sitation about state of T).

Also I'm not sure if the rechecking of heapnblocks is good way. It's just some
idea.

2. For some reason email from Wenbo didn't get to my emailbox, but I can
see his email here:
/messages/by-id/resend/ba92ac77-24f1-44ad-abf0-11e64e0a7831@gmail.com
(and resending is is somewhat not effective)

I've been reviewing the patch and ran some concurrent tests against it.
I found an issue with false positive corruption reports under concurrent
VACUUM.

+ slot = table_slot_create(state->heaprel, NULL);
+ found = table_tuple_fetch_row_version(state->heaprel, tid,
+                         SnapshotAny, slot);
+ if (!found)
+ {
+         ExecDropSingleTupleTableSlot(slot);
+         ereport(ERROR,

bt_verify_index_tuple_points_to_heap uses SnapshotAny with
table_tuple_fetch_row_version to distinguish "tuple doesn't exist" from
"tuple exists but is dead". However, bt_index_check only holds
AccessShareLock which compatible with VACUUM's ShareUpdateExclusiveLock.
A concurrent VACUUM Phase 1 can prune heap pages(ItemIdSetDead) between
Bloom filter probe and the heap lookup. Causing
table_tuple_fetch_row_version to return false for a legal
dead-but-now-pruned tuple and a false positive corruption report.

The table_tuple_satisfies_snapshot check does not help, it only runs
when the fetch succeeds(LP_NORMAL). Once VACUUM sets LP_DEAD,
heapam_fetch sees !ItemIdIsNormal(lp) and returns false before any
snapshot checks.

The reproduce should be: DELETE all rows form a big table, then launch
VACUUM and bt_index_check concurrently. A POC test script attached.

I think this is spot-on. I haven't run repro, but if we take ASL and VACUUM
is allowed to run (under ShareUpdateExclusiveLock) then amcheck performs the
heap bloom building (but ignores dead tuples, while btree still can
point to those),
but VACUUM could later remove the heap tuple. It seems to be impossible (under
ASL) to differentiate orphaned index tuple from concurrently vacuumed
dead tuple?

Maybe we should grab ShareUpdateExclusiveLock in bt_index_check for
indexallkeysmatch==true? With some new documentation warning about
this? (to block
just VACUUM, but not DML)

Alternative is to do it via bt_index_parent_check()/ShareLock, but it's kind of
hardly acceptable in many sitations.

3. This is more a question than finding: assuming extremly big tables, wouldn't
we benefit from some form of caching for EState in
bt_verify_index_tuple_points_to_heap()? Or we do not care about such stuff
in amcheck? (we seem to re-create Estate with each call to FormIndexDatum())

-J.

[0]: /messages/by-id/013D63E2-5D75-492E-85FF-1D5CC0148C82@gmail.com

Hi Andrey,

On Wed, Apr 8, 2026 at 3:17 PM Jakub Wartak
<jakub.wartak@enterprisedb.com> wrote:

On Thu, Mar 5, 2026 at 7:00 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Thanks for the review! I've addressed all these issues in patch steps 4 and 5. Other steps are intact. PFA.

Hi Andrey and Wenboo,

1. I tried the v4 to see how it would cope with heap missing segments[0]
(with -Dsegsize_blocks=6), so given:

[..]

postgres=# select bt_index_check(index=>'ptest_pkey',
heapallindexed=>true, checkunique=>true, indexallkeysmatch=>true);
ERROR: could not open file "base/5/24578.1" (target block 6): No such
file or directory

this error was not quite expected for me (altough it is much better than nothing
as on master today), because patch v4-0002 assumes in
bt_verify_index_tuple_points_to_heap() that we should get false and error in
more human-firendly way ("index tuple points to non-existent heap"). The error
itself is coming out of the following stacktrace:

[..]

So with the attached patch it becomes more human understandable, but it is
somehow orthogonal check? Still maybe we can combine this under this $thread
ambrella and make it as an option of indexallkeysmatch too? Attached
behaves likes this:

postgres=# select bt_index_check(index=>'ptest_pkey');
ERROR: index line pointer in index "ptest_pkey" points to missing
page in table "ptest"
DETAIL: Index tid=(5,14) points to heap tid=(6,1) but heap has only 6 blocks.
HINT: this can be caused by lost relation segment (missing or removed file).

(note it does not need to indexallkeysmatch, but it's related)

[..]

2. For some reason email from Wenbo didn't get to my emailbox, but I can
see his email here:
/messages/by-id/resend/ba92ac77-24f1-44ad-abf0-11e64e0a7831@gmail.com
(and resending is is somewhat not effective)

I've been reviewing the patch and ran some concurrent tests against it.
I found an issue with false positive corruption reports under concurrent
VACUUM.

+ slot = table_slot_create(state->heaprel, NULL);
+ found = table_tuple_fetch_row_version(state->heaprel, tid,
+                         SnapshotAny, slot);
+ if (!found)
+ {
+         ExecDropSingleTupleTableSlot(slot);
+         ereport(ERROR,

bt_verify_index_tuple_points_to_heap uses SnapshotAny with
table_tuple_fetch_row_version to distinguish "tuple doesn't exist" from
"tuple exists but is dead". However, bt_index_check only holds
AccessShareLock which compatible with VACUUM's ShareUpdateExclusiveLock.
A concurrent VACUUM Phase 1 can prune heap pages(ItemIdSetDead) between
Bloom filter probe and the heap lookup. Causing
table_tuple_fetch_row_version to return false for a legal
dead-but-now-pruned tuple and a false positive corruption report.

The table_tuple_satisfies_snapshot check does not help, it only runs
when the fetch succeeds(LP_NORMAL). Once VACUUM sets LP_DEAD,
heapam_fetch sees !ItemIdIsNormal(lp) and returns false before any
snapshot checks.

The reproduce should be: DELETE all rows form a big table, then launch
VACUUM and bt_index_check concurrently. A POC test script attached.

I think this is spot-on. I haven't run repro, but if we take ASL and VACUUM
is allowed to run (under ShareUpdateExclusiveLock) then amcheck performs the
heap bloom building (but ignores dead tuples, while btree still can
point to those) (..)

[..]

[0] - /messages/by-id/013D63E2-5D75-492E-85FF-1D5CC0148C82@gmail.com

Here's my attempt at the fixing the LP_DEAD tuples being reported as false
positives by the v4 patchset as stated earlier. The simplest way for the
v4 to hit those was to:
1) get a fresh initdb cluster
2) run it on all pg_catalog tables/indexes, and to me it (v4*) always reliably
spotted some LP_DEAD ones there, e.g.:

ERROR: index tuple in index "pg_proc_proname_args_nsp_index" points
to non-existent heap tuple in table "pg_proc"
DETAIL: Index tid=(5,21) points to heap tid=(18,32) that no longer exists.

postgres=# SELECT lp, lp_off, lp_flags, lp_len, t_xmin, t_xmax,
t_ctid, t_infomask, t_infomask2 FROM
heap_page_items(get_raw_page('pg_catalog.pg_proc', 18)) WHERE lp =
32;
lp | lp_off | lp_flags | lp_len | t_xmin | t_xmax | t_ctid |
t_infomask | t_infomask2
----+--------+----------+--------+--------+--------+--------+------------+-------------
32 | 0 | 3 | 0 | | | | |

where lp_flags=3 ==> LP_DEAD

When we do bt_index_check() still with AccessShareLock we may expect:
- concurrent VACUUM producing LP_DEADs
- SELECT doing opportunistic heap pruning (micro-vacuuming) and that is
allowed to run even with ShareUpdate (bt_index_parent_check)
- LP_DEAD being already there before we even start (aborted VACUUM?)

I've tried to enhance v4 based on above assumptions, so v5 is attached.

Note: AFAIU opportunistic pruning can produce LP_UNUSED only on HOT-chain
intermediate tuples, and those should be never referenced by a btree index.
I think it's safe to still to stick to AccessShareLock (earlier I've
mentioned ShareUpdateExclusiveLock, but it looks it is doable without that)
as we are not affected by truncation and we have xmin horizon so stuff should
not be removed from underneath us.

Note that the pg_amcheck/003_check (not contrib/amcheck) still blows-up:
not ok 46 - pg_amcheck over schema s2 with corrupt tables excluded
reports no corruption status (got 2 vs expected 0)
not ok 47 - pg_amcheck over schema s2 with corrupt tables excluded
reports no corruption stdout /(?^:^$)/

it's due to SELECT "amcheck_schema".bt_index_check(index := c.oid,
heapallindexed := false )
ERROR: could not open file "base/16384/16508": No such file or directory

being called on index, while the *heap* relation has been intentionally
removed by plan_to_remove_relation_file() from that test. So even if the
tables are exluded by the pg_amcheck --exclude-table , amcheck with
v5-0005 blows up on RelationGetNumberOfBlocks() as the file is gone.
IMHO, we should remove that tes there, but I have not done so far this
far as I simply do not know if that's acceptable.

Moving that check into heapallindexed option is also a possibilty, but
that would
hide cheap check under big and costly option (reading whole heap).

Also, some rough measurements: One can expect indexallkeysmatched to multiply
bt_index_check runtime by roughly ~5x compared to a bare check (all flags off)
run, or - as expected - ~2x when compared to heapallindexed (that makes sense,
we are building another full bloom filter). Sample performance baseline of
corruption checking seems to be like that:
- ~5s for COPY pgbench_accounts to /dev/null (wont discover missing stuff)
- ~2s for COPY (SELECT aid FROM pgbench_accounts) to /dev/null (IOT,
wont discover
missing stuff)
amcheck:
- ~1.5s for bt_index_check('pgbench_accounts_pkey' (with v5b-0005 might discover
loss of heap relation segments)
- ~2s for bt_index_check() with just checkunique=on
- ~7.1s for bt_index_check() with just heapallindexed=on
- ~8.1s for bt_index_check() with just indexallkeysmatched=on (this new feature,
discovers dangling/missing/corrupted entries as Andrey explain in
original post)
- ~14s for full verification (everything on)

-J.

Hi Jakub,

Many thanks for your reviews, patches and performance measurements!
Sorry for the long pause. Attached is v6, a reworked patchset that
incorporates your work and Wenbo's findings. It consists of 4 steps:

0001 - core indexallkeysmatch verification
0002 - detection of dangling index entries (based on your "take 2")
0003 - detection of lost heap segments (your patch)
0004 - documentation

Below is how your notes were addressed.

Here's my attempt at the fixing the LP_DEAD tuples being reported as false
positives by the v4 patchset as stated earlier.

I took a slightly different route in 0001. The root cause of Wenbo's
false positive was using SnapshotAny to make an existence check that
a regular index scan never makes. In v6 the heap lookup mirrors an
index scan exactly: table_index_fetch_tuple() with the same MVCC
snapshot used for fingerprinting. It follows LP_REDIRECT and HOT
chains (which table_tuple_fetch_row_version() does not - a redirected
HOT chain root would previously return "not found" and never get its
keys verified), and a "not found" result is simply skipped, just like
an index scan would do. This kills the LP_DEAD race by construction:
no page state inspection is needed on the common path.

Your line pointer inspection can be found as 0002, a follow-up check
when the MVCC fetch finds nothing. There I had to restrict it
somewhat:

* beyond-EOF TIDs are reported at any lock level - relation extension
only grows the heap and truncation requires AccessExclusiveLock,
which our AccessShareLock blocks;

* out-of-range offsets and LP_UNUSED slots are reported only under
ShareLock, i.e. bt_index_parent_check(). Under AccessShareLock
there is a narrow race: we verify a copy of the index page, so
concurrent VACUUM phase 2 may have already removed the index entry
we are holding and set the heap slot LP_UNUSED (or truncated the
line pointer array). We cannot distinguish that from corruption, so
bt_index_check() stays silent. Under ShareLock VACUUM is blocked,
and opportunistic pruning only sets LP_UNUSED for HOT chain member
slots, which are never referenced from indexes - so there the
report is reliable.

So the answer to your question about taking ShareUpdateExclusiveLock
in bt_index_check() is: with the MVCC approach it is not needed. The
key-match verification itself works fine under AccessShareLock; only
the LP_UNUSED detection needs the stronger lock, and users who want
it can run bt_index_parent_check().

Note that the pg_amcheck/003_check (not contrib/amcheck) still blows-up:
[...] IMHO, we should remove that tes there, but I have not done so far this
far as I simply do not know if that's acceptable.

I think I found a way that does not require touching the test. In
0003 the beyond-EOF checks are now disabled in one narrow case: when
the first segment of the heap's main fork is missing entirely
(smgrexists() returns false). That case is different from the silent
data loss your patch targets: with segment 0 gone, every heap access
already fails loudly with "could not open file", so there is nothing
silent to detect. The silent case - a missing or truncated
higher-numbered segment, where mdnblocks() happily returns a short
size without any error - is still detected exactly as in your patch.

It is worth spelling out that today PostgreSQL simply has no way to
notice that a relation file is missing or shorter than it should be:
the relation size is defined by whatever files are present, there is
no record of the expected size to compare against. This patchset
creates the first such way, by cross-checking heap size against
index TIDs.

The reason for the skip is that bt_index_check() without heap-related
options historically never touches the heap at all, and pg_amcheck
relies on that: in the 003_check test the user explicitly excludes
the corrupt tables with --exclude-table but still checks their
indexes. Failing the pure index-structure check because the
(deliberately excluded) heap is unreadable would be a behavior
regression. With this change the whole pg_amcheck test suite passes.

As for your question whether to make this an option of
indexallkeysmatch: I kept the check unconditional instead. It is a
simple comparison of a leaf tuple's TID against a cached block count,
so it costs nearly nothing on top of the index scan that
bt_index_check() performs anyway, and hiding a cheap check behind an
expensive option (indexallkeysmatch or heapallindexed both read the
whole heap) would make it much less likely to actually run.

I also made the 008 TAP test skip unless the server was built with
--with-segsize-blocks - otherwise the test tables have no second
segment to lose, and the test failed on a standard build.

3. This is more a question than finding: assuming extremly big tables, wouldn't
we benefit from some form of caching for EState in
bt_verify_index_tuple_points_to_heap()?

Good point, fixed in 0001. The executor state, the slot and the index
fetch context are now created once per verification and reused across
all heap lookups (with ResetPerTupleExprContext() per lookup), so
index expressions are compiled only once.

While rebasing I also noticed that the v4-0005 documentation patch
(two Bloom filters each up to maintenance_work_mem, ~2% probability
of missing an inconsistency) was lost from the v5b series, so 0004
restores those notes and documents which dangling entry cases are
reported at which lock level.

One thing still on my TODO list for a next version: the key
comparison relies on bt_normalize_tuple(), and normalization has
historically been a rich source of subtle bugs for heapallindexed
(TOAST compression differences, and a whole series of follow-up
fixes for various datum representations). indexallkeysmatch compares
freshly formed index tuples against on-page ones from the other
direction, so the same corner cases - and possibly new ones - apply.
I want to go through those known heapallindexed normalization issues
carefully and add targeted tests before we consider this ready, but
I would rather do that as a separate step on top of this patchset
than hold up review of the overall structure. Pointers to cases you
think are most likely to bite here are very welcome.

Best regards, Andrey Borodin.

On Tue, Jul 21, 2026 at 8:08 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Hi Andrey!

Many thanks for your reviews, patches and performance measurements!
Sorry for the long pause. Attached is v6, a reworked patchset that
incorporates your work and Wenbo's findings. It consists of 4 steps:

0001 - core indexallkeysmatch verification
0002 - detection of dangling index entries (based on your "take 2")
0003 - detection of lost heap segments (your patch)
0004 - documentation

Below is how your notes were addressed.

Here's my attempt at the fixing the LP_DEAD tuples being reported as false
positives by the v4 patchset as stated earlier.

I took a slightly different route in 0001. The root cause of Wenbo's
false positive was using SnapshotAny to make an existence check that
a regular index scan never makes. In v6 the heap lookup mirrors an
index scan exactly: table_index_fetch_tuple() with the same MVCC
snapshot used for fingerprinting. It follows LP_REDIRECT and HOT
chains (which table_tuple_fetch_row_version() does not - a redirected
HOT chain root would previously return "not found" and never get its
keys verified), and a "not found" result is simply skipped, just like
an index scan would do. This kills the LP_DEAD race by construction:
no page state inspection is needed on the common path.

Your line pointer inspection can be found as 0002, a follow-up check
when the MVCC fetch finds nothing. There I had to restrict it
somewhat:

[..]

So the answer to your question about taking ShareUpdateExclusiveLock
in bt_index_check() is: with the MVCC approach it is not needed. The
key-match verification itself works fine under AccessShareLock; only
the LP_UNUSED detection needs the stronger lock, and users who want
it can run bt_index_parent_check().

Awesome, great!

Note that the pg_amcheck/003_check (not contrib/amcheck) still blows-up:
[...] IMHO, we should remove that tes there, but I have not done so far this
far as I simply do not know if that's acceptable.

I think I found a way that does not require touching the test. In
0003 the beyond-EOF checks are now disabled in one narrow case: when
the first segment of the heap's main fork is missing entirely
(smgrexists() returns false). That case is different from the silent
data loss your patch targets: with segment 0 gone, every heap access
already fails loudly with "could not open file", so there is nothing
silent to detect. The silent case - a missing or truncated
higher-numbered segment, where mdnblocks() happily returns a short
size without any error - is still detected exactly as in your patch.

It is worth spelling out that today PostgreSQL simply has no way to
notice that a relation file is missing or shorter than it should be:
the relation size is defined by whatever files are present, there is
no record of the expected size to compare against. This patchset
creates the first such way, by cross-checking heap size against
index TIDs.

The reason for the skip is that bt_index_check() without heap-related
options historically never touches the heap at all, and pg_amcheck
relies on that: in the 003_check test the user explicitly excludes
the corrupt tables with --exclude-table but still checks their
indexes. Failing the pure index-structure check because the
(deliberately excluded) heap is unreadable would be a behavior
regression. With this change the whole pg_amcheck test suite passes.

As for your question whether to make this an option of
indexallkeysmatch: I kept the check unconditional instead. It is a
simple comparison of a leaf tuple's TID against a cached block count,
so it costs nearly nothing on top of the index scan that
bt_index_check() performs anyway, and hiding a cheap check behind an
expensive option (indexallkeysmatch or heapallindexed both read the
whole heap) would make it much less likely to actually run.

I also made the 008 TAP test skip unless the server was built with
--with-segsize-blocks - otherwise the test tables have no second
segment to lose, and the test failed on a standard build.

Thanks for positive response, incorporation this small beyond EOF checking
idea into the patchset and for those many plenty fixes and ideas :) I'm
most happy about the conclustion that ShareUpdateExclusiveLock is not need.

It's been some time, so I've reviewed it from scratch and found only mostly
minor stuff:

000[23]: I think they somehwat redundant in terms of beyond EOF error. The
0003's check in bt_target_page_check() always runs, so the beyond-EOF
detection in bt_report_dangling_index_entrybecomes() from 0002 is not
reachable when both are enabled. Maybe we could merge those two patches into
one ?

0004 (docs):

a. Altough we say 'an additional phase verifies that each index tuple points
to a heap tuple', but I'm wondering - even if the commitmsg mention it - if
we shouldn't put into the docs that this means additional heap sequential
scan as for for multi-TB tables, this will be a big cost. I propose simply
writing
'A <new>additional dedicated</new> heap scan is ...'

b. The docs within indexallkeysmatch tags reads as if the beyond-EOF check
would be only happening with indexallkeysmatch enabled, but it's not.

0001:

a. it seems that ordering of verify_common.h may be flagged as an
potential issue by committer (?)

b. it adds pg_surgery into Makefile, but doesn't seem to use it? Is it
leftover or was the intention to use it to test something?
BTW: we seem to miss tests for those LP_UNUSED errors in 0002, not
sure maybe that's related and You misseds some tests there?

0002:

Altough I remember stress-testing this on standby, now I'm not really sure now
today, if we don't need to invalidate the cached file size in recovery. I mean
that state->heapnblocks = RelationGetNumberOfBlocks(state->heaprel) might
still get cached and old value from smgr_cached_nblocks as it's an lseek()
optimization. One way to do that would be to 'reln->smgr_cached_nblocks[forknum]
= InvalidBlockNumber', thoughts on this?

One thing still on my TODO list for a next version: the key
comparison relies on bt_normalize_tuple(), and normalization has
historically been a rich source of subtle bugs for heapallindexed
(TOAST compression differences, and a whole series of follow-up
fixes for various datum representations).[..]

It's way outside of my typical area, can You please share some sample pointer or
link?

-J.

Jakub, thanks for the next review round! I'll provide thoughtful answers later.
Here's some context:

On 27 Jul 2026, at 17:15, Jakub Wartak <jakub.wartak@enterprisedb.com> wrote:

One thing still on my TODO list for a next version: the key
comparison relies on bt_normalize_tuple(), and normalization has
historically been a rich source of subtle bugs for heapallindexed
(TOAST compression differences, and a whole series of follow-up
fixes for various datum representations).[..]

It's way outside of my typical area, can You please share some sample pointer or
link?

e.g. see ab65dfb0fb2, eba775345d2, b1fe8efdf1.
The thing is index key fingerprint might be different under different circumstances.

Thanks!

Best regards, Andrey Borodin.

[0]: https://github.com/postgres/postgres/commit/ab65dfb0fb2
[1]: https://github.com/postgres/postgres/commit/eba775345d2
[2]: https://github.com/postgres/postgres/commit/b1fe8efdf17