MIME-Version: 1.0
References: <432626F9-65DF-4F0D-B345-26CFC3E2CFAC@yandex-team.ru>
In-Reply-To: <432626F9-65DF-4F0D-B345-26CFC3E2CFAC@yandex-team.ru>
From: Zsolt Parragi <zsolt.parragi@percona.com>
Date: Wed, 25 Feb 2026 23:18:04 +0000
Message-ID: <CAN4CZFNCyEOC8exfEn9SVV=uhzUA4KQYcVRUea2E0ts-rvXg3w@mail.gmail.com>
Subject: Re: amcheck: add index-all-keys-match verification for B-Tree
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAN4CZFNCyEOC8exfEn9SVV%3DuhzUA4KQYcVRUea2E0ts-rvXg3w%40mail.gmail.com>
Precedence: bulk

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;