From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: jian he Date: Wed, 26 Aug 2026 15:26:46 +0900 Subject: [PATCH] resolve offset evaluation once for all again. 1. Remove some unnecessary structs. 2. resolve_one_nav() resolves an entry's offsets, regardless of the offset expression's node type; whether to resolve is up to caller's decision. resolve_nav_offsets() uses each offset's isnull marker to skip entries already resolved, avoiding repeated evaluation of the offset expressions; see ExecWindowAgg -> resolve_nav_offsets. 3. Teach eval_const_expressions about RPRNavExpr. Fold a navigation's offset arguments, which must be a constants: reducing them to Consts enables planning time validation of null or negative offsets and lets EXPLAIN show fixed trim bounds. Leave the navigated expression (arg) unfolded: it is evaluated only when the navigation's target row exists, so folding could raise an error at plan time that execution never raises -- e.g. PREV(v / 0) after a VALUES pull-up has replaced v with a Const. Note: Provisional. Everything above the marker is the patch's own message, and it describes a design this commit mostly does not take -- the title included. The whole of it has to be rewritten before this is submitted; what stands here is a record of the reading, not a message meant to go out. Taken in part. What is kept is the T_RPRNavExpr case in clauses.c without the four ereports it raises over a folded Const, and the rpr_base regression the folding turns from an error into a result. The rest is left out; the reasons follow, and they mostly come back to one decision. * clauses.c keeps the folding and drops the plan-time checks. The two messages they raise are already in eval_nav_offset(), and that site has to stay: an offset written as a bind parameter, or the PARAM_EXEC a correlated SRF leaves behind, has no value while planning, so the executor is the only place that sees every offset. A second site puts the same two strings in two files that then have to agree. Both offsets in core this one resembles -- a window frame's, resolved in calculate_frame_offsets(), and LIMIT's, in recompute_limits() -- are checked at execution as well, even where the value is written as a literal: EXPLAIN SELECT 1 LIMIT -1 prints a plan. * nodeWindowAgg.c is not touched. Its change is the removal of eval_nav_offset()'s validate argument and what follows from that. validate is there so the resolution build_define_offsets() does at init, for EXPLAIN's sake, can meet a null or negative constant offset without raising -- EXPLAIN does not execute. Rejecting such a constant while planning is what makes that path unreachable, and that is the part not taken; without it validate is the only check there is. The "unnecessary structs" of the commit message is the same validate, as a field of EvalDefineOffsetsContext. Nothing in this file stands on its own. * execnodes.h keeps navResolvePending. Dropping it turns the guard in ExecWindowAgg from one predicted-false branch into an unconditional call that walks the whole rprNavOffsets list, and ExecWindowAgg runs once per output tuple; calculate_frame_offsets(), three lines above, answers the same question with a flag. The pass this adds at the end of resolve_nav_offsets(), re-reading the isnull markers to raise "must not be null", cannot fire either: resolve_one_nav() writes both markers false unconditionally, and a null offset has already raised in eval_nav_offset() by then. * rpr_explain.sql and its expected output are not touched. Their negative-offset cases exist to show that such a navigation contributes no reach while the plan still prints, which is exactly what validate buys; with it gone they cannot exist, and the patch retires them in place. Four comments saying what each case pins become a bare "-- error", PREV(FIRST(v, -3), 2) becomes PREV(FIRST(v, 3), -2), FIRST(v, -1) becomes FIRST(v, 1), and the NEXT(LAST()) case at the int64 limit is deleted. What it adds in exchange is already there: EXECUTE with (1, -1) and with (-1, 1) against a compound navigation is test_runtime_neg_compound_offset a hundred lines earlier in the same file, under force_generic_plan and with the same expected error, and the unbound $1 without EXPLAIN fails in parse analysis with "there is no parameter $1", short of any row pattern code. * execExprInterp.c is not touched at all. Every line it changes there is a comment or an Assert -- no executable statement moves -- and only one of them follows from this work: the paragraph describing how resolve_nav_offsets() settles both offsets, which stops being accurate once validate is gone, and which is accurate again here. The rest is loss. The function header records that the entry point is reached from JIT-compiled expressions through build_EvalXFunc, which is written down nowhere else, and the three Assert(!pg_sub_s64_overflow(...)) are what hold up the "cannot underflow" the comments claim right above them. Three things here are not in the patch. rpr_base.sql still carried an XXX calling the fold a defect; the patch corrects the expected output and leaves the marker standing, so it is replaced with what the case pins now. And the new case in clauses.c gets an XXX of its own: leaving arg alone gives up more than it has to, since only a subexpression that can raise needs to be spared, and T_CaseExpr a few hundred lines above takes the other side of that trade. The third is three regression cases pinning what that XXX costs, which is more than cycles. Before this commit RPRNavExpr had no case here and fell to ece_generic_processing(), whose expression_tree_mutator arm walks arg along with the two offsets. Taking the node over and skipping arg therefore stops more than folding: eval_const_expressions() is where a CollateExpr becomes a RelabelType, where named arguments become positional, and where omitted defaults are inserted, and preprocess_expression() calls those mandatory rather than optional. None of them has an executor step, so a navigation argument that keeps one now fails where the same expression one level outside the navigation runs -- "unrecognized node type: 32" for PREV(s COLLATE "C"), "unrecognized node type: 17" for named arguments, and a call initialized one argument short for an omitted default. The cases go in with the failures as their expected output and an XXX saying what each should return once the argument is walked again; committing them red would leave the suite unusable for the rest of the series. Nothing else in the commit changes: the offsets were already folded by the generic path, so this case's only net effect on the tree is the skip these cases now describe. --- src/backend/optimizer/util/clauses.c | 31 +++++++++++++++ src/test/regress/expected/rpr_base.out | 52 ++++++++++++++++++++++++-- src/test/regress/sql/rpr_base.sql | 47 +++++++++++++++++++++-- 3 files changed, 123 insertions(+), 7 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 8da4ed617b5..761862ce82a 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -2888,6 +2888,37 @@ eval_const_expressions_mutator(Node *node, newexpr->ignore_nulls = expr->ignore_nulls; newexpr->location = expr->location; + return (Node *) newexpr; + } + case T_RPRNavExpr: + { + RPRNavExpr *expr = (RPRNavExpr *) node; + RPRNavExpr *newexpr = makeNode(RPRNavExpr); + + memcpy(newexpr, expr, sizeof(RPRNavExpr)); + + /* + * Do not fold the navigated expression (arg): it is evaluated + * only when the navigation's target row exists. Folding + * could raise an error at plan time that execution never + * raises -- e.g. PREV(v / 0), where an earlier VALUES pull-up + * has already replaced v with a Const. + * + * XXX this gives up more than it has to. Only a + * subexpression that can raise has to be left alone; the rest + * of arg could still be folded, and every row of the match + * pays for what was not. T_CaseExpr above takes the other + * side of the same trade, folding its arms and accepting the + * plan-time error where the arm cannot be dropped outright. + */ + newexpr->offset_arg = (Expr *) + eval_const_expressions_mutator((Node *) expr->offset_arg, + context); + + newexpr->compound_offset_arg = (Expr *) + eval_const_expressions_mutator((Node *) expr->compound_offset_arg, + context); + return (Node *) newexpr; } case T_FuncExpr: diff --git a/src/test/regress/expected/rpr_base.out b/src/test/regress/expected/rpr_base.out index bd6904ecc66..ef7d9d4c610 100644 --- a/src/test/regress/expected/rpr_base.out +++ b/src/test/regress/expected/rpr_base.out @@ -2066,14 +2066,58 @@ 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. +-- The argument is left unfolded, so the division is never reached: PREV has +-- no row to navigate to and the navigation yields null. 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); -ERROR: division by zero + id | cnt +----+----- + 1 | 0 +(1 row) + +-- XXX these three are wrong. 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. The T_RPRNavExpr case in eval_const_expressions_mutator() +-- leaves the navigated expression alone, so none of them reaches a navigation +-- argument, and each case below fails where the same expression one level +-- outside the navigation runs. Once the case recurses into the argument -- +-- sparing only what can raise, which its own XXX already proposes -- the +-- expected output becomes 1|0 2|2 3|0 for the first and 1|0 2|4 3|0 4|0 5|0 +-- for the other two, which is what the unnavigated forms return today. +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'); +ERROR: unrecognized node type: 32 +-- 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); +ERROR: unrecognized node type: 17 +-- 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); +ERROR: no value found for parameter 2 +CONTEXT: SQL function "rpr_nav_dflt" statement 1 +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 361d0923c2a..05f808f1606 100644 --- a/src/test/regress/sql/rpr_base.sql +++ b/src/test/regress/sql/rpr_base.sql @@ -1476,14 +1476,55 @@ 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. +-- The argument is left unfolded, so the division is never reached: PREV has +-- no row to navigate to and the navigation yields null. 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); +-- XXX these three are wrong. 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. The T_RPRNavExpr case in eval_const_expressions_mutator() +-- leaves the navigated expression alone, so none of them reaches a navigation +-- argument, and each case below fails where the same expression one level +-- outside the navigation runs. Once the case recurses into the argument -- +-- sparing only what can raise, which its own XXX already proposes -- the +-- expected output becomes 1|0 2|2 3|0 for the first and 1|0 2|4 3|0 4|0 5|0 +-- for the other two, which is what the unnavigated forms return today. +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