Re: Fix missing EvalPlanQual recheck for TID scans

> On Sep 10, 2025, at 02:20, Sophie Alpert <pg(at)sophiebits(dot)com> wrote:
>
> Hi Chao,
>
> Thanks for taking a look.
>
> On Tue, Sep 9, 2025 at 12:52 AM, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>> However, I noticed that, when “TidRecheck()” is called, it is actually passed with “epqstate->recheckplanstate” that is always newly created, which means we repeatedly call “TidListEval(node)” and run “bsearch()” when there are multiple row involved in the update. If update qual is just “ctid = <a single ctid>”, that should fine. But if we do “update .. where ctid in (a list of ctid)”, then duplicately call “TidListEval()” might not be a good thing.
>
> Based on my current understanding, the EPQ PlanState is shared across all EPQ
> checks for a given plan and that ReScan is called at similar times as it is for
> the initial access path with, ExecScanFetch delegating to the appropriate *Next
> or *Recheck method as each row is processed. Therefore, my patch does not
> recompute the TidList for every row.

No, that’s not true. If you extend David’s procedure and use 3 sessions to reproduce the problem, s1 update (0,1), s2 update (0,2) and s3 update (0,1) or (0,2), and trace the backend process of s3, you will see every time when TidRecheck() is called, “node” is brand new, so it has to call TidListEval(node) every time.

>
>> As “TidRecheck(TidScanState *node, TupleTableSlot *slot)” gets the second parameter “slot” with the updated slot, so I am thinking the other solution could be that, we just check if the updated slot is visible to current transaction. So I made a local change like this:
>>
>> ...
>> tuple = ExecCopySlotHeapTuple(slot);
>> visible = HeapTupleSatisfiesVisibility(tuple, GetActiveSnapshot(), slot->tts_tableOid);
>
> This is semantically different from the patch I've proposed. My test
> `permutation tid1 tidsucceed2 c1 c2 read` fails against your code, despite
> passing with the `SET enable_tidscan = OFF;` flag that I understand we want to
> match the behavior of. (In addition, I understand HeapTupleSatisfiesVisibility
> is specific to the heap access method and can't be used here, though perhaps
> tuple_fetch_row_version could be used instead.)
>

I think TidScan only applies to HeapTable, so we can use HeapTupleSatisfiesVisibility(0 in TidRecheck().

With my proposal, my theory is that, TidNext() fetches a tuple by ctid, if other concurrent transaction update/delete the tuple, the tuple's visibility changes.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/