Hi Chao, Thanks for taking a look.
On Tue, Sep 9, 2025 at 12:52 AM, Chao Li <li.evan.c...@gmail.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. > 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.) Sophie
