Move FOR PORTION OF checks out of analysis

On 2026-May-15, Paul A Jungwirth wrote:

Starting a dedicated thread for this, based on [1]. To recap:
[...]
There is no commitfest entry yet, so I'll make one from this thread.

Please note that this doesn't need a commitfest entry -- since it's a
bug introduced during PG19, it needs to be listed as an open item
instead, to make sure we get it fixed before releasing 19. Please add
it to the list here
https://wiki.postgresql.org/wiki/Open_Items

Thanks,

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Los trabajadores menos efectivos son sistematicamente llevados al lugar
donde pueden hacer el menor daño posible: gerencia." (El principio Dilbert)

On Fri, May 15, 2026 at 3:09 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

Please note that this doesn't need a commitfest entry -- since it's a
bug introduced during PG19, it needs to be listed as an open item
instead, to make sure we get it fixed before releasing 19. Please add
it to the list here
https://wiki.postgresql.org/wiki/Open_Items

Ah, thanks for clarifying! I went through all the items I've been
tracking and found a few missing from Open Items, so I added them.

Yours,

--
Paul ~{:-)
pj@illuminatedcomputing.com

On Sat, May 16, 2026 at 5:17 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

Starting a dedicated thread for this, based on [1]. To recap:

- FOR PORTION OF needs to reject FDWs in the planner/executor, not in
analysis, first to guarantee the status hasn't changed but also
because we need to check child partitions as well.
- We need to postpone checking for volatile functions too.

The first part is done by jian he's patch from that thread (with some
test revisions).

The second part is done by the second patch here.

+ if (contain_volatile_functions(parse->forPortionOf->targetRange))
+ ereport(ERROR,
+ (errmsg("FOR PORTION OF bounds cannot contain volatile functions")))

missing errcode, we can change it to

+        if (contain_volatile_functions(parse->forPortionOf->targetRange))
+            ereport(ERROR,
+                    errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                    errmsg("FOR PORTION OF bounds cannot contain
volatile functions"));

On Tue, Jun 9, 2026 at 1:11 AM jian he <jian.universality@gmail.com> wrote:

missing errcode, we can change it to

+        if (contain_volatile_functions(parse->forPortionOf->targetRange))
+            ereport(ERROR,
+                    errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                    errmsg("FOR PORTION OF bounds cannot contain
volatile functions"));

Thanks for the note! Here is v9 with that change.

Yours,

--
Paul ~{:-)
pj@illuminatedcomputing.com

On 11.06.26 23:37, Paul A Jungwirth wrote:

On Tue, Jun 9, 2026 at 1:11 AM jian he <jian.universality@gmail.com> wrote:

missing errcode, we can change it to

+        if (contain_volatile_functions(parse->forPortionOf->targetRange))
+            ereport(ERROR,
+                    errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                    errmsg("FOR PORTION OF bounds cannot contain
volatile functions"));

Thanks for the note! Here is v9 with that change.

I have checked the patch v9 0002 "Move FOR PORTION OF volatile check
into planner". Functionality-wise, this seems correct.

Could you explain why you used EXPRKIND_TARGET? Maybe there is a good
reason, but otherwise it seems kind of arbitrary.

The new ereport call in planner.c has some funny parentheses: You have
parentheses around errmsg, but not around errcode. This is probably
because of the way this was copy and pasted from its previous location.

On Wed, Jun 24, 2026 at 2:54 AM Peter Eisentraut <peter@eisentraut.org> wrote:

I have checked the patch v9 0002 "Move FOR PORTION OF volatile check
into planner". Functionality-wise, this seems correct.

Could you explain why you used EXPRKIND_TARGET? Maybe there is a good
reason, but otherwise it seems kind of arbitrary.

This expression is used in the target list to set the application-time
column. We intersect the old value with the FOR PORTION OF bounds.

The new ereport call in planner.c has some funny parentheses: You have
parentheses around errmsg, but not around errcode. This is probably
because of the way this was copy and pasted from its previous location.

Here are new patches with the parens cleaned up.

Yours,

--
Paul ~{:-)
pj@illuminatedcomputing.com

On 24.06.26 18:39, Paul A Jungwirth wrote:

On Wed, Jun 24, 2026 at 2:54 AM Peter Eisentraut <peter@eisentraut.org> wrote:

I have checked the patch v9 0002 "Move FOR PORTION OF volatile check
into planner". Functionality-wise, this seems correct.

Could you explain why you used EXPRKIND_TARGET? Maybe there is a good
reason, but otherwise it seems kind of arbitrary.

This expression is used in the target list to set the application-time
column. We intersect the old value with the FOR PORTION OF bounds.

The new ereport call in planner.c has some funny parentheses: You have
parentheses around errmsg, but not around errcode. This is probably
because of the way this was copy and pasted from its previous location.

Here are new patches with the parens cleaned up.

Ok, I have committed that v10 0002 patch. I will look at the other one
next.

On 27.06.26 14:00, Peter Eisentraut wrote:

On 24.06.26 18:39, Paul A Jungwirth wrote:

On Wed, Jun 24, 2026 at 2:54 AM Peter Eisentraut
<peter@eisentraut.org> wrote:

I have checked the patch v9 0002 "Move FOR PORTION OF volatile check
into planner".  Functionality-wise, this seems correct.

Could you explain why you used EXPRKIND_TARGET?  Maybe there is a good
reason, but otherwise it seems kind of arbitrary.

This expression is used in the target list to set the application-time
column. We intersect the old value with the FOR PORTION OF bounds.

The new ereport call in planner.c has some funny parentheses: You have
parentheses around errmsg, but not around errcode.  This is probably
because of the way this was copy and pasted from its previous location.

Here are new patches with the parens cleaned up.

Ok, I have committed that v10 0002 patch.  I will look at the other one
next.

Committed the 0001 patch as well. I added an errdetail that identifies
the name of the foreign table, which could be helpful when you're
dealing with a partition hierarchy.

I wonder whether contain_volatile_functions_after_planning
and its sibling contain_mutable_functions_after_planning
shouldn't be documented to warn against this sort of misuse.

Ideally we'd not have them at all because of the TOCTOU hazard.
The remaining callers are in DDL callers like generated-column
creation and expression-index creation, where we basically just
have to trust that users won't redefine their functions later.
But we don't want developers putting such calls into places that
are any further upstream of execution than they have to be.

regards, tom lane