Hi Chao,
Thanks for taking a look.
On Tue, Sep 9, 2025 at 12:52 AM, Chao Li <li.evan.chao@gmail.com> wrote:
> However, I noticed that, when “TidRecheck()” is called, it is actually passed with “epqstate->recheckplanstate” that
isalways newly created, which means we repeatedly call “TidListEval(node)” and run “bsearch()” when there are multiple
rowinvolved in the update. If update qual is just “ctid = <a single ctid>”, that should fine. But if we do “update ..
wherectid 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.
> As “TidRecheck(TidScanState *node, TupleTableSlot *slot)” gets the second parameter “slot” with the updated slot, so
Iam thinking the other solution could be that, we just check if the updated slot is visible to current transaction. So
Imade 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.)
Sophie
