From 639351144bdeccd149b09e60f859008dcf85ed19 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" Date: Thu, 3 Sep 2026 12:51:14 -0700 Subject: [PATCH v1] Evaluate the FOR PORTION OF target only once We currently build a range/multirange from the FOR PORTION OF bounds both in the target list (to update the column) and separately when computing the leftovers. If those two sites get a different result, we produce inconsistent results. One way would be a STABLE function that isn't really stable. Another is using current_setting() while a trigger calls set_config(). This commit detects if the target is non-constant, and if so it uses a PARAM_EXEC slot to pass the value, set once in ExecInitModifyTable(). To preserve EXPLAIN output, I updated find_param_referent() so it deparses to the original expression. No change is needed for DELETE, since there is no target list. Reported-by: Noah Misch Author: Paul A. Jungwirth --- src/backend/executor/nodeModifyTable.c | 16 ++++ src/backend/optimizer/plan/planner.c | 78 +++++++++++++++++++- src/backend/parser/analyze.c | 1 + src/backend/utils/adt/ruleutils.c | 25 ++++++- src/include/nodes/primnodes.h | 8 ++ src/test/regress/expected/for_portion_of.out | 38 ++++++++++ src/test/regress/sql/for_portion_of.sql | 39 ++++++++++ 7 files changed, 199 insertions(+), 6 deletions(-) diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 5681505d31c..00e5dc27021 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -5649,6 +5649,22 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) errmsg("FOR PORTION OF target must not be null"), executor_errposition(estate, forPortionOf->targetLocation))); + /* + * If the planner routed the range column's new value through a + * PARAM_EXEC slot, fill it in now, so that the value stored in the + * range column and the leftovers computed below come from this one + * evaluation of the target. + */ + if (forPortionOf->targetParamId >= 0) + { + ParamExecData *prm; + + prm = &(estate->es_param_exec_vals[forPortionOf->targetParamId]); + prm->execPlan = NULL; + prm->value = targetRange; + prm->isnull = false; + } + /* Create state for FOR PORTION OF operation */ fpoState = makeNode(ForPortionOfState); diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 24d964c993d..8b2f54a466f 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -145,6 +145,8 @@ typedef struct } having_grouping_ctx; /* Local functions */ +static Node *substitute_for_portion_of_target(Node *node, Node *targetRange, + Param *param); static Node *preprocess_expression(PlannerInfo *root, Node *expr, int kind); static void preprocess_qual_conditions(PlannerInfo *root, Node *jtnode); static Bitmapset *find_having_conflicts(Query *parse, Index group_rtindex); @@ -1092,14 +1094,37 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name, if (parse->forPortionOf) { - parse->forPortionOf->targetRange = + ForPortionOfExpr *forPortionOf = parse->forPortionOf; + + forPortionOf->targetRange = preprocess_expression(root, - parse->forPortionOf->targetRange, + forPortionOf->targetRange, EXPRKIND_TARGET); - if (contain_volatile_functions(parse->forPortionOf->targetRange)) + if (contain_volatile_functions(forPortionOf->targetRange)) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("FOR PORTION OF bounds cannot contain volatile functions"))); + + /* + * We pass a non-Const UPDATE FOR PORTION OF target as a PARAM_EXEC to + * avoid evaluating it more than once (in the target list and when + * computing temporal leftovers). + */ + if (parse->commandType == CMD_UPDATE && + !IsA(forPortionOf->targetRange, Const)) + { + Param *prm; + + prm = generate_new_exec_param(root, + exprType(forPortionOf->targetRange), + exprTypmod(forPortionOf->targetRange), + exprCollation(forPortionOf->targetRange)); + forPortionOf->targetParamId = prm->paramid; + parse->targetList = (List *) + substitute_for_portion_of_target((Node *) parse->targetList, + forPortionOf->targetRange, + prm); + } } foreach(l, parse->mergeActionList) @@ -1388,6 +1413,15 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name, */ SS_identify_outer_params(root); + /* + * The FOR PORTION OF target Param is supplied by the ModifyTable node + * above the plan that references it, so tell finalize_plan() it is + * validly referenceable here. + */ + if (parse->forPortionOf && parse->forPortionOf->targetParamId >= 0) + root->outer_params = bms_add_member(root->outer_params, + parse->forPortionOf->targetParamId); + /* * If any initPlans were created in this query level, adjust the surviving * Paths' costs and parallel-safety flags to account for them. The @@ -1407,6 +1441,44 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name, return root; } + +/* + * Replace every occurrence of the FOR PORTION OF target range with a Param. + * + * The copies we are looking for were made by transformForPortionOfClause() + * and have been through the same preprocessing as targetRange itself, so an + * equal() comparison finds them. + */ +typedef struct +{ + Node *targetRange; + Param *param; +} substitute_for_portion_of_target_context; + +static Node * +substitute_for_portion_of_target_mutator(Node *node, + substitute_for_portion_of_target_context *context) +{ + if (node == NULL) + return NULL; + if (equal(node, context->targetRange)) + return (Node *) copyObject(context->param); + return expression_tree_mutator(node, + substitute_for_portion_of_target_mutator, + context); +} + +static Node * +substitute_for_portion_of_target(Node *node, Node *targetRange, Param *param) +{ + substitute_for_portion_of_target_context context; + + context.targetRange = targetRange; + context.param = param; + + return substitute_for_portion_of_target_mutator(node, &context); +} + /* * preprocess_expression * Do subquery_planner's preprocessing work for an expression, diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index 581457c69c9..e8042e3b5d7 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -1345,6 +1345,7 @@ transformForPortionOfClause(ParseState *pstate, errmsg("WHERE CURRENT OF with FOR PORTION OF is not implemented")); result = makeNode(ForPortionOfExpr); + result->targetParamId = -1; /* Look up the FOR PORTION OF name requested. */ range_attno = attnameAttNum(targetrel, forPortionOf->range_name, false); diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index a12b804e6dc..9dc9c49a4be 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -9018,9 +9018,9 @@ find_param_referent(Param *param, deparse_context *context, *ancestor_cell_p = NULL; /* - * If it's a PARAM_EXEC parameter, look for a matching NestLoopParam or - * SubPlan argument. This will necessarily be in some ancestor of the - * current expression's Plan node. + * If it's a PARAM_EXEC parameter, look for a matching NestLoopParam, + * SubPlan argument, or FOR PORTION OF bound. This will necessarily be in + * some ancestor of the current expression's Plan node. */ if (param->paramkind == PARAM_EXEC) { @@ -9058,6 +9058,25 @@ find_param_referent(Param *param, deparse_context *context, } } + /* + * If ancestor is a ModifyTable, see if it's a FOR PORTION OF bound. + */ + if (IsA(ancestor, ModifyTable)) + { + ForPortionOfExpr *forPortionOf; + + forPortionOf = (ForPortionOfExpr *) + ((ModifyTable *) ancestor)->forPortionOf; + + if (forPortionOf != NULL && + forPortionOf->targetParamId == param->paramid) + { + *dpns_p = dpns; + *ancestor_cell_p = lc; + return forPortionOf->targetRange; + } + } + /* * If ancestor is a SubPlan, check the arguments it provides. */ diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index 5fbeae2f954..159ea236619 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -2449,6 +2449,14 @@ typedef struct ForPortionOfExpr List *rangeTargetList; /* List of TargetEntrys to set the time * column(s) */ Oid withoutPortionProc; /* SRF proc for old_range - target_range */ + + /* + * PARAM_EXEC slot holding the once-per-statement value of targetRange, or + * -1 if we didn't need one. The planner substitutes it into + * rangeTargetList so that the range column's new value and the leftovers + * computed from the same value can never disagree. + */ + int targetParamId; ParseLoc location; /* token location, or -1 if unknown */ ParseLoc targetLocation; /* token location, or -1 if unknown */ } ForPortionOfExpr; diff --git a/src/test/regress/expected/for_portion_of.out b/src/test/regress/expected/for_portion_of.out index 64789d1777b..152fc2955eb 100644 --- a/src/test/regress/expected/for_portion_of.out +++ b/src/test/regress/expected/for_portion_of.out @@ -2793,4 +2793,42 @@ SELECT * FROM fpo_rls ORDER BY valid_at; DROP TABLE fpo_rls; DROP ROLE regress_fpo_rls; +-- +-- The range column's new value and the leftovers must come from a single +-- evaluation of the FOR PORTION OF target. +-- +-- Only volatile functions are rejected in the bounds, so a function that is +-- labelled STABLE without being stable can hand out a different value at each +-- evaluation site. Whatever value it gives, the pieces the statement leaves +-- behind must still tile the row's original range exactly: no overlapping and +-- no gapped history. +-- +CREATE TABLE fpo_drift ( + id int, + valid_at daterange, + name text +); +INSERT INTO fpo_drift VALUES + (1, daterange('2000-01-01', '2010-01-01'), 'one'); +CREATE SEQUENCE fpo_drift_seq; +CREATE FUNCTION fpo_drift_bound() RETURNS date LANGUAGE sql STABLE AS + $$ SELECT '2003-01-01'::date + nextval('fpo_drift_seq')::int $$; +UPDATE fpo_drift + FOR PORTION OF valid_at FROM '2002-01-01' TO fpo_drift_bound() + SET name = 'x'; +-- The total length of the pieces equals the original 3653 days (so nothing +-- overlaps), and they merge back into exactly the original range (so nothing +-- is missing). Deliberately not asserting how many times the bound was +-- evaluated, which is an implementation detail. +SELECT sum(upper(valid_at) - lower(valid_at)) AS total_days, + range_agg(valid_at) AS covered + FROM fpo_drift; + total_days | covered +------------+--------------------------- + 3653 | {[2000-01-01,2010-01-01)} +(1 row) + +DROP TABLE fpo_drift; +DROP FUNCTION fpo_drift_bound(); +DROP SEQUENCE fpo_drift_seq; RESET datestyle; diff --git a/src/test/regress/sql/for_portion_of.sql b/src/test/regress/sql/for_portion_of.sql index b61fe10478e..cae3b5ab551 100644 --- a/src/test/regress/sql/for_portion_of.sql +++ b/src/test/regress/sql/for_portion_of.sql @@ -1849,4 +1849,43 @@ SELECT * FROM fpo_rls ORDER BY valid_at; DROP TABLE fpo_rls; DROP ROLE regress_fpo_rls; +-- +-- The range column's new value and the leftovers must come from a single +-- evaluation of the FOR PORTION OF target. +-- +-- Only volatile functions are rejected in the bounds, so a function that is +-- labelled STABLE without being stable can hand out a different value at each +-- evaluation site. Whatever value it gives, the pieces the statement leaves +-- behind must still tile the row's original range exactly: no overlapping and +-- no gapped history. +-- + +CREATE TABLE fpo_drift ( + id int, + valid_at daterange, + name text +); +INSERT INTO fpo_drift VALUES + (1, daterange('2000-01-01', '2010-01-01'), 'one'); + +CREATE SEQUENCE fpo_drift_seq; +CREATE FUNCTION fpo_drift_bound() RETURNS date LANGUAGE sql STABLE AS + $$ SELECT '2003-01-01'::date + nextval('fpo_drift_seq')::int $$; + +UPDATE fpo_drift + FOR PORTION OF valid_at FROM '2002-01-01' TO fpo_drift_bound() + SET name = 'x'; + +-- The total length of the pieces equals the original 3653 days (so nothing +-- overlaps), and they merge back into exactly the original range (so nothing +-- is missing). Deliberately not asserting how many times the bound was +-- evaluated, which is an implementation detail. +SELECT sum(upper(valid_at) - lower(valid_at)) AS total_days, + range_agg(valid_at) AS covered + FROM fpo_drift; + +DROP TABLE fpo_drift; +DROP FUNCTION fpo_drift_bound(); +DROP SEQUENCE fpo_drift_seq; + RESET datestyle; -- 2.47.3