Re: Fix missing EvalPlanQual recheck for TID scans - Mailing list pgsql-hackers

Thanks for taking a look.

On Sun, Sep  7, 2025 at  9:51 PM, David Rowley <dgrowleyml@gmail.com> 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 enable_tidscan = off;` is added to both session
setups.

> 1. This part is quite annoying:
>
> + if (node->tss_TidList == NULL)
> + TidListEval(node);
>
> Of course, it's required since ExecReScanTidScan() wiped out that list.
>
> Maybe we can add a boolean variable named tts_inScan to TidScanState
> after tss_isCurrentOf (so as it consumes the existing padding bytes
> and does not enlarge that struct), and have ExecReScanTidScan() set
> that to false rather than wipe out the tss_TidList. Then just reuse
> the existing list in TidRecheck(). That should save on the extra
> overhead of having to rebuild the list each time there's an EPQ
> recheck.
>
> 2. For TidRangeRecheck, I don't see why this part is needed:
>
> + if (!TidRangeEval(node))
> + return false;
>
> The TID range is preserved already and shouldn't need to be recalculated.

I am a bit unclear on the exact logic around what order Next, ReScan, and Recheck are called in, so I may have written
thecode too defensively. Concretely: For ExecReScanTidScan wiping out tss_TidList, I was thinking that when
ExecReScanTidScanis called, there may be changed params that require the list to be recalculated. I also wasn't sure if
Recheckcan be called without a preceding Next and/or ReScan having been called with the same params, or if it can
alwaysrely on those having been called immediately prior. (I had reviewed IndexRecheck as I figured it seems the most
likelyto be a correct example here, but I wasn't able to infer from how it's written.)
 

If you could help clarify the guarantees here I'd appreciate it. For what it's worth, if I test removing the
tss_TidListclearing in ExecReScanTidScan and recomputation in TidRecheck (of course this is not correct in general),
thenmy `tid1 tidsucceed2 c1 c2` test now fails due to TidRecheck incorrectly returning false.
 

I'm actually digging in more now and if I'm reading the code correctly, EvalPlanQualStart calls ExecInitNode to create
anentirely separate PlanState tree for EPQ so I'm unsure if any of the state is carried over to the Recheck calls? If
I'munderstandign correctly, then it seems the best we can do in this patch is to leave TidRecheck as I had it but for
TidRangeRecheckI can add a node->trss_boundsInitialized flag, so that we recompute it once per EPQ instead of per tuple
checked.

(I'll be happy to make the stylistic changes you mentioned.)

Appreciate your time,

Sophie