From 1640248483186fe013a8dd84c3c74a07c6ae4e94 Mon Sep 17 00:00:00 2001 From: jian he Date: Wed, 26 Aug 2026 15:26:46 +0900 Subject: [PATCH] Do not substitute a row-independent value into a navigation argument The argument of a row pattern navigation operation is evaluated at the row the navigation lands on, not at the current row. Pulling up a one-row VALUES, a subquery, or a function RTE that folded to a constant substitutes that value for the columns the argument reads, and that is not what the column stood for there. Constant folding is where the substitution shows itself. A navigation argument has always gone through it -- RPRNavExpr has no case of its own in eval_const_expressions_mutator, so it falls to the generic arm -- and once the column has become a value there is nothing left to stop the fold: the argument of PREV(v / 0) over a one-row VALUES reads 10 / 0 and raises while planning, where execution, finding no target row, never divides. Wrap such a replacement in a PlaceHolderVar instead. A replacement that still depends on the row needs nothing: it is correct at the target row either way, and folding cannot reduce it. The flag rides in replace_rte_variables_context, which pullup_replace_vars_callback() already reads, and the mutator sets it only while it walks the argument. Nothing in the folding pass changes. What folds inside an argument folded before this commit too, and a subexpression that folds is one that never depended on the row. Three regression cases go in for the rewrites that have to reach a navigation argument: a CollateExpr that must become a RelabelType, named arguments that must become positional, and an omitted default that must be inserted. preprocess_expression() calls those mandatory rather than optional and none has an executor step, so an argument that kept one would fail where the same expression one level outside the navigation runs. --- src/backend/optimizer/prep/prepjointree.c | 8 +- src/backend/rewrite/rewriteManip.c | 27 ++++ src/include/rewrite/rewriteManip.h | 1 + src/test/regress/expected/rpr_base.out | 152 +++++++++++++++++++++- src/test/regress/sql/rpr_base.sql | 100 +++++++++++++- 5 files changed, 275 insertions(+), 13 deletions(-) diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index a343ed16ba1..08f043e3503 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -2813,9 +2813,15 @@ pullup_replace_vars_callback(const Var *var, * a Var or PlaceHolderVar that we can just add the nullingrels to). We * also need one if the caller has instructed us that certain expression * replacements need to be wrapped for identification purposes. + * + * A Var below the argument of a row pattern navigation operation needs + * one too, so that a replacement that does not depend on the row is not + * folded in: that argument reads the row the navigation lands on, not + * this one. */ need_phv = (var->varnullingrels != NULL) || - (rcon->wrap_option != REPLACE_WRAP_NONE); + (rcon->wrap_option != REPLACE_WRAP_NONE) || + context->in_rpr_nav_arg; /* * If PlaceHolderVars are needed, we cache the modified expressions in diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c index 3653f00d383..3bd18a128e6 100644 --- a/src/backend/rewrite/rewriteManip.c +++ b/src/backend/rewrite/rewriteManip.c @@ -1448,6 +1448,7 @@ replace_rte_variables(Node *node, int target_varno, int sublevels_up, context.callback_arg = callback_arg; context.target_varno = target_varno; context.sublevels_up = sublevels_up; + context.in_rpr_nav_arg = false; /* * We try to initialize inserted_sublink to true if there is no need to @@ -1506,6 +1507,32 @@ replace_rte_variables_mutator(Node *node, } /* otherwise fall through to copy the var normally */ } + else if (IsA(node, RPRNavExpr)) + { + /* + * The argument of a row pattern navigation operation is evaluated at + * the row the navigation lands on, so flag it for the callback. The + * offsets beside it are ordinary expressions. + */ + RPRNavExpr *nav = (RPRNavExpr *) node; + RPRNavExpr *newnode = makeNode(RPRNavExpr); + bool save_in_rpr_nav_arg = context->in_rpr_nav_arg; + + memcpy(newnode, nav, sizeof(RPRNavExpr)); + + context->in_rpr_nav_arg = true; + newnode->arg = (Expr *) + replace_rte_variables_mutator((Node *) nav->arg, context); + context->in_rpr_nav_arg = save_in_rpr_nav_arg; + + newnode->offset_arg = (Expr *) + replace_rte_variables_mutator((Node *) nav->offset_arg, context); + newnode->compound_offset_arg = (Expr *) + replace_rte_variables_mutator((Node *) nav->compound_offset_arg, + context); + + return (Node *) newnode; + } else if (IsA(node, Query)) { /* Recurse into RTE subquery or not-yet-planned sublink subquery */ diff --git a/src/include/rewrite/rewriteManip.h b/src/include/rewrite/rewriteManip.h index 2d15d8f8f95..a8af1820477 100644 --- a/src/include/rewrite/rewriteManip.h +++ b/src/include/rewrite/rewriteManip.h @@ -32,6 +32,7 @@ struct replace_rte_variables_context int target_varno; /* RTE index to search for */ int sublevels_up; /* (current) nesting depth */ bool inserted_sublink; /* have we inserted a SubLink? */ + bool in_rpr_nav_arg; /* below a row pattern navigation argument? */ }; typedef enum ReplaceVarsNoMatchOption diff --git a/src/test/regress/expected/rpr_base.out b/src/test/regress/expected/rpr_base.out index 3f767959e8d..411dad51488 100644 --- a/src/test/regress/expected/rpr_base.out +++ b/src/test/regress/expected/rpr_base.out @@ -2063,6 +2063,29 @@ SELECT id, count(*) OVER w AS cnt FROM rpr_nav t WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS NEXT(val / 0) > 0); ERROR: division by zero +-- A constant subexpression of the argument is folded away, so the pattern +-- does not recompute it per row. +SELECT id, count(*) OVER w AS cnt +FROM rpr_nav +WINDOW w AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS PREV(val + 2 * 3) > 0); + id | cnt +----+----- + 1 | 0 + 2 | 4 + 3 | 0 + 4 | 0 + 5 | 0 +(5 rows) + +-- Folding a constant subexpression can raise where the whole argument would +-- not have: unlike val / 0 above, 1 / 0 does not depend on the row, so it is +-- reached at plan time even on the row PREV misses on. +SELECT id, count(*) OVER w AS cnt +FROM rpr_nav t +WHERE id = 1 +WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS PREV(val + 1 / 0) > 0); +ERROR: division by zero -- Here the null reaches the DEFINE predicate itself instead of an IS NULL -- An all-NULL target row would have made v IS NULL true and matched the -- first row, so this pins the predicate side of the same behaviour. @@ -2076,9 +2099,10 @@ WINDOW w AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTER 2 | 0 (2 rows) --- Constant folding can leave a navigation argument with no column reference --- at all (v folds to 10, so PREV(v IS NULL) becomes PREV(false)), which the --- planner has to accept rather than re-run the parse-time rejection. +-- Pulling up the VALUES substitutes 10 for v, which is not what the column +-- stood for under a navigation: the argument reads the row the navigation +-- lands on, not this one. The replacement is wrapped in a PlaceHolderVar +-- rather than folded through. WITH t(id, v) AS (VALUES (1, 10)) SELECT id, count(*) OVER w AS cnt FROM t @@ -2088,14 +2112,130 @@ WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE 1 | 0 (1 row) --- XXX Folding evaluates the argument while planning, with the current row's --- value standing in for the target row's, so this divides by zero even though --- PREV has no row to navigate to. +-- That wrapping is what keeps this one from raising: the divisor is constant +-- but the dividend is not folded through, so the division stands until +-- execution, where PREV has no row to navigate to and never reaches it. WITH t(id, v) AS (VALUES (1, 10)) SELECT id, count(*) OVER w AS cnt FROM t WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS PREV(v / 0) > 0); + id | cnt +----+----- + 1 | 0 +(1 row) + +-- A pulled-up subquery and a function RTE that folded to a constant reach a +-- navigation argument the same way, so both are wrapped as well. +SELECT id, count(*) OVER w AS cnt +FROM (SELECT 1 AS id, 10 AS v) t +WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS PREV(v / 0) > 0); + id | cnt +----+----- + 1 | 0 +(1 row) + +SELECT count(*) OVER w AS cnt +FROM abs(-10) AS v +WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS PREV(v / 0) > 0); + cnt +----- + 0 +(1 row) + +-- Only the argument is protected. One level outside the navigation the same +-- column is replaced and folded as it is anywhere else, and the division +-- raises at plan time -- as it does for the same WHERE clause over the same +-- one-row VALUES, with no pattern in sight. +WITH t(id, v) AS (VALUES (1, 10)) +SELECT id, count(*) OVER w AS cnt +FROM t +WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS v / 0 > 0); ERROR: division by zero +-- A replacement that still depends on the row is left unwrapped, because it +-- is what the column meant at whichever row the navigation lands on. +SELECT id, count(*) OVER w AS cnt +FROM (SELECT id, val + 1 AS v FROM rpr_nav) t +WINDOW w AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS PREV(v) > 0); + id | cnt +----+----- + 1 | 0 + 2 | 4 + 3 | 0 + 4 | 0 + 5 | 0 +(5 rows) + +-- Nesting: the inner navigation's argument is below the outer one's, so the +-- column there is wrapped too. +WITH t(id, v) AS (VALUES (1, 10)) +SELECT id, count(*) OVER w AS cnt +FROM t +WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A) DEFINE A AS PREV(LAST(v / 0, 1), 2) > 0); + id | cnt +----+----- + 1 | 0 +(1 row) + +-- eval_const_expressions() must perform a few rewrites on every expression +-- it is handed -- a CollateExpr becomes a RelabelType, named arguments become +-- positional, omitted defaults are inserted -- and preprocess_expression() +-- documents them as mandatory, not as optimizations. Each of the three below +-- reaches the executor only if those rewrites reach inside a navigation +-- argument, and each returns what the same expression one level outside the +-- navigation returns. +CREATE TABLE rpr_nav_txt (id int, s text); +INSERT INTO rpr_nav_txt VALUES (1, 'b'), (2, 'c'), (3, 'a'); +CREATE FUNCTION rpr_nav_named(a int, b int) RETURNS int + LANGUAGE sql IMMUTABLE AS 'SELECT $1 * 10 + $2'; +CREATE FUNCTION rpr_nav_dflt(a int, b int DEFAULT 100) RETURNS int + LANGUAGE sql IMMUTABLE AS 'SELECT $2'; +-- COLLATE under a navigation: the executor has no CollateExpr step, so the +-- RelabelType rewrite has to reach here. +SELECT id, count(*) OVER w AS cnt +FROM rpr_nav_txt +WINDOW w AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS PREV(s COLLATE "C") > 'a'); + id | cnt +----+----- + 1 | 0 + 2 | 2 + 3 | 0 +(3 rows) + +-- Named arguments under a navigation: the executor has no NamedArgExpr step. +SELECT id, count(*) OVER w AS cnt +FROM rpr_nav +WINDOW w AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS PREV(rpr_nav_named(b => 7, a => val)) > 0); + id | cnt +----+----- + 1 | 0 + 2 | 4 + 3 | 0 + 4 | 0 + 5 | 0 +(5 rows) + +-- An omitted default under a navigation: without the insertion the call is +-- initialized with one fewer argument than the callee reads. +SELECT id, count(*) OVER w AS cnt +FROM rpr_nav +WINDOW w AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS PREV(rpr_nav_dflt(val)) = 100); + id | cnt +----+----- + 1 | 0 + 2 | 4 + 3 | 0 + 4 | 0 + 5 | 0 +(5 rows) + +DROP FUNCTION rpr_nav_dflt(int, int); +DROP FUNCTION rpr_nav_named(int, int); +DROP TABLE rpr_nav_txt; -- PREV function - reference previous row in pattern SELECT id, val, COUNT(*) OVER w as cnt FROM rpr_nav diff --git a/src/test/regress/sql/rpr_base.sql b/src/test/regress/sql/rpr_base.sql index 7ba578dc50a..41c18e96e3e 100644 --- a/src/test/regress/sql/rpr_base.sql +++ b/src/test/regress/sql/rpr_base.sql @@ -1484,6 +1484,21 @@ SELECT id, count(*) OVER w AS cnt FROM rpr_nav t WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS NEXT(val / 0) > 0); +-- A constant subexpression of the argument is folded away, so the pattern +-- does not recompute it per row. +SELECT id, count(*) OVER w AS cnt +FROM rpr_nav +WINDOW w AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS PREV(val + 2 * 3) > 0); + +-- Folding a constant subexpression can raise where the whole argument would +-- not have: unlike val / 0 above, 1 / 0 does not depend on the row, so it is +-- reached at plan time even on the row PREV misses on. +SELECT id, count(*) OVER w AS cnt +FROM rpr_nav t +WHERE id = 1 +WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS PREV(val + 1 / 0) > 0); + -- Here the null reaches the DEFINE predicate itself instead of an IS NULL -- An all-NULL target row would have made v IS NULL true and matched the -- first row, so this pins the predicate side of the same behaviour. @@ -1492,22 +1507,95 @@ SELECT id, count(*) OVER w AS cnt FROM t WINDOW w AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS PREV(v IS NULL)); --- Constant folding can leave a navigation argument with no column reference --- at all (v folds to 10, so PREV(v IS NULL) becomes PREV(false)), which the --- planner has to accept rather than re-run the parse-time rejection. +-- Pulling up the VALUES substitutes 10 for v, which is not what the column +-- stood for under a navigation: the argument reads the row the navigation +-- lands on, not this one. The replacement is wrapped in a PlaceHolderVar +-- rather than folded through. WITH t(id, v) AS (VALUES (1, 10)) SELECT id, count(*) OVER w AS cnt FROM t WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS PREV(v IS NULL)); --- XXX Folding evaluates the argument while planning, with the current row's --- value standing in for the target row's, so this divides by zero even though --- PREV has no row to navigate to. +-- That wrapping is what keeps this one from raising: the divisor is constant +-- but the dividend is not folded through, so the division stands until +-- execution, where PREV has no row to navigate to and never reaches it. WITH t(id, v) AS (VALUES (1, 10)) SELECT id, count(*) OVER w AS cnt FROM t WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS PREV(v / 0) > 0); +-- A pulled-up subquery and a function RTE that folded to a constant reach a +-- navigation argument the same way, so both are wrapped as well. +SELECT id, count(*) OVER w AS cnt +FROM (SELECT 1 AS id, 10 AS v) t +WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS PREV(v / 0) > 0); + +SELECT count(*) OVER w AS cnt +FROM abs(-10) AS v +WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS PREV(v / 0) > 0); + +-- Only the argument is protected. One level outside the navigation the same +-- column is replaced and folded as it is anywhere else, and the division +-- raises at plan time -- as it does for the same WHERE clause over the same +-- one-row VALUES, with no pattern in sight. +WITH t(id, v) AS (VALUES (1, 10)) +SELECT id, count(*) OVER w AS cnt +FROM t +WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN (A) DEFINE A AS v / 0 > 0); + +-- A replacement that still depends on the row is left unwrapped, because it +-- is what the column meant at whichever row the navigation lands on. +SELECT id, count(*) OVER w AS cnt +FROM (SELECT id, val + 1 AS v FROM rpr_nav) t +WINDOW w AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS PREV(v) > 0); + +-- Nesting: the inner navigation's argument is below the outer one's, so the +-- column there is wrapped too. +WITH t(id, v) AS (VALUES (1, 10)) +SELECT id, count(*) OVER w AS cnt +FROM t +WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A) DEFINE A AS PREV(LAST(v / 0, 1), 2) > 0); + +-- eval_const_expressions() must perform a few rewrites on every expression +-- it is handed -- a CollateExpr becomes a RelabelType, named arguments become +-- positional, omitted defaults are inserted -- and preprocess_expression() +-- documents them as mandatory, not as optimizations. Each of the three below +-- reaches the executor only if those rewrites reach inside a navigation +-- argument, and each returns what the same expression one level outside the +-- navigation returns. +CREATE TABLE rpr_nav_txt (id int, s text); +INSERT INTO rpr_nav_txt VALUES (1, 'b'), (2, 'c'), (3, 'a'); +CREATE FUNCTION rpr_nav_named(a int, b int) RETURNS int + LANGUAGE sql IMMUTABLE AS 'SELECT $1 * 10 + $2'; +CREATE FUNCTION rpr_nav_dflt(a int, b int DEFAULT 100) RETURNS int + LANGUAGE sql IMMUTABLE AS 'SELECT $2'; + +-- COLLATE under a navigation: the executor has no CollateExpr step, so the +-- RelabelType rewrite has to reach here. +SELECT id, count(*) OVER w AS cnt +FROM rpr_nav_txt +WINDOW w AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS PREV(s COLLATE "C") > 'a'); + +-- Named arguments under a navigation: the executor has no NamedArgExpr step. +SELECT id, count(*) OVER w AS cnt +FROM rpr_nav +WINDOW w AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS PREV(rpr_nav_named(b => 7, a => val)) > 0); + +-- An omitted default under a navigation: without the insertion the call is +-- initialized with one fewer argument than the callee reads. +SELECT id, count(*) OVER w AS cnt +FROM rpr_nav +WINDOW w AS (ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS PREV(rpr_nav_dflt(val)) = 100); + +DROP FUNCTION rpr_nav_dflt(int, int); +DROP FUNCTION rpr_nav_named(int, int); +DROP TABLE rpr_nav_txt; + -- PREV function - reference previous row in pattern SELECT id, val, COUNT(*) OVER w as cnt FROM rpr_nav