> On Sep 10, 2025, at 02:20, Sophie Alpert <[email protected]> wrote: > > Hi Chao, > > Thanks for taking a look. > > On Tue, Sep 9, 2025 at 12:52 AM, Chao Li <[email protected]> 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/
