Updated patch attached.
On Sun, Sep 7, 2025 at 9:51 PM, David Rowley <dgrowleyml@gmail.com> 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 left this as is.
> 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've added a new trss_boundsInitialized flag such that we calculate the range once per EPQ rescan. In order to preserve
thesemantics when the min or max is NULL, I'm setting trss_mintid/trss_maxtid to have invalid ItemPointers as a
sentinelin the cases where TidRangeEval returns false. I added a ItemPointerIsValid assertion given that it's now more
relevantto correctness but I can remove it if it feels superfluous. Let me know if there is a more idiomatic way to
treatthis.
> 3. In TidRecheck(), can you make "match" an "ItemPointer" and do:
> match = (ItemPointer) bsearch(...);
> 4. Can you put this comment above the "if"?
> 5. Can you make TidRangeRecheck() look like this?
> 6. For the tests. It should be ok to make the Tid range scan test do
> ctid BETWEEN '(0,1)' AND '(0,1)'. I feel this is more aligned to the
> TID Range Scan version of what you're doing in the TID Scan test.
All done.
Do let me know if other changes would be helpful.
Sophie
