Re: Fix missing EvalPlanQual recheck for TID scans

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 the code too defensively. Concretely: For ExecReScanTidScan wiping out tss_TidList, I was thinking that when ExecReScanTidScan is called, there may be changed params that require the list to be recalculated. I also wasn't sure if Recheck can be called without a preceding Next and/or ReScan having been called with the same params, or if it can always rely on those having been called immediately prior. (I had reviewed IndexRecheck as I figured it seems the most likely to 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_TidList clearing in ExecReScanTidScan and recomputation in TidRecheck (of course this is not correct in general), then my `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 an entirely separate PlanState tree for EPQ so I'm unsure if any of the state is carried over to the Recheck calls? If I'm understandign correctly, then it seems the best we can do in this patch is to leave TidRecheck as I had it but for TidRangeRecheck I 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