Horton

Re: Fix missing EvalPlanQual recheck for TID scans

Sophie Alpert <pg@sophiebits.com>

View thread View on postgr.es
From: "Sophie Alpert" <pg@sophiebits.com>
To: "David Rowley" <dgrowleyml@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Date: 2025-09-08T16:35:53Z
Lists: pgsql-hackers

Attachments

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 the semantics when the min or max is NULL, I'm setting trss_mintid/trss_maxtid to have invalid ItemPointers as a sentinel in the cases where TidRangeEval returns false. I added a ItemPointerIsValid assertion given that it's now more relevant to correctness but I can remove it if it feels superfluous. Let me know if there is a more idiomatic way to treat this.

> 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

Commits

  1. Add missing EPQ recheck for TID Range Scan

  2. Add missing EPQ recheck for TID Scan

  3. Repair problems with EvalPlanQual where target table is scanned as

  4. Tid access method feature from Hiroshi Inoue, Inoue@tpf.co.jp