Re: Fix missing EvalPlanQual recheck for TID scans

2025-09-15 Thread Sophie Alpert
On Mon, Sep 15, 2025 at 4:23 AM, David Rowley wrote: > For the v2 patch, I've hacked on that a bit and stripped out the > trss_boundsInitialized stuff and just make it so we recalculate the > TID List/Range on every recheck. I also added another isolation test > permutation to have s1 rollback an

Re: Fix missing EvalPlanQual recheck for TID scans

2025-09-14 Thread Sophie Alpert
On Sun, Sep 14, 2025 at 6:49 PM, Chao Li wrote: > This is a wrong example and (0,3) should NOT be updated. According to the > definition of “read committed”: > https://www.postgresql.org/docs/current/transaction-iso.html#XACT-READ-COMMITTED > “A query sees only data committed before the query be

Re: Fix missing EvalPlanQual recheck for TID scans

2025-09-13 Thread Sophie Alpert
On Sat, Sep 13, 2025 at 3:12 PM, Sophie Alpert wrote: > And indeed, like I mentioned in my previous message, my isolation test > `permutation tid1 tidsucceed2 c1 c2 read` from eval-plan-qual.spec in > my patch will fail if Recheck were to return false in this case. Though > somewh

Re: Fix missing EvalPlanQual recheck for TID scans

2025-09-13 Thread Sophie Alpert
On Tue, Sep 9, 2025 at 10:18 PM, Chao Li wrote: > 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(

Re: Fix missing EvalPlanQual recheck for TID scans

2025-09-09 Thread Sophie Alpert
Updated patch attached. On Sun, Sep 7, 2025 at 9:51 PM, David Rowley wrote: > 1. This part is quite annoying: > > + if (node->tss_TidList == NULL) > + TidListEval(node); > > Of course, it's required since ExecReScanTidScan() wiped out that list. Given that EPQ uses separate PlanState, I've lef

Re: Fix missing EvalPlanQual recheck for TID scans

2025-09-09 Thread Sophie Alpert
Hi Chao, Thanks for taking a look. On Tue, Sep 9, 2025 at 12:52 AM, Chao Li 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 “bs

Re: Fix missing EvalPlanQual recheck for TID scans

2025-09-08 Thread Sophie Alpert
Thanks for taking a look. On Sun, Sep 7, 2025 at 9:51 PM, David Rowley wrote: > I agree that this is a bug and we should fix it. The behaviour should > match what you'd get if you ran it with "SET enable_tidscan = 0;". I've confirmed that my new tests already pass on current master if `SET e

Fix missing EvalPlanQual recheck for TID scans

2025-09-07 Thread Sophie Alpert
Hi all, I learned that the TID Scan node does not properly implement EvalPlanQual recheck, which has already been noted in comment added in 2002 (commit 6799a6ca21). This does seem to have the potential to manifest in observable and surprising ways when queries are filtering on `ctid`, like for