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.
