Hi jian,
I applied the "resolve offset evaluation once for all again." patch
you sent off-list. Below is what I took and what I left, keyed to the
numbered items in your commit message, and one defect that came out of
it. The patch attached is that state.
> 1. Remove some unnecessary structs.
> 2. resolve_one_nav() resolves an entry's offsets ... uses each
> offset's isnull marker to skip entries already resolved
Neither is taken. The "unnecessary structs" is the validate field of
EvalDefineOffsetsContext, and validate is what lets the resolution
build_define_offsets() does at init, for EXPLAIN's sake, meet a null
or negative constant offset without raising -- EXPLAIN does not
execute. Retiring that path means rejecting such a constant while
planning, which is the part I did not take, for the reason below.
The isnull marker skip goes stale across a rescan as worded.
build_nav_offsets() sets both markers true once at init, and
ExecReScanWindowAgg re-arms only navResolvePending, so a
PARAM_EXEC/PARAM_EXTERN offset resolved on the first scan is skipped
on every later one and the stale value is reused -- along with the
per-scan validation.
> 3. Teach eval_const_expressions about RPRNavExpr.
This one is taken, without the four ereports it raises over a folded
Const. Those two messages 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.
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.
There is a defect in that case, though.
Leaving the argument out stops more than folding. That pass is not
only an optimization: it is also where nodes the executor cannot
handle get rewritten away -- named arguments become positional,
omitted defaults are filled in, and COLLATE is turned into a form the
executor knows. The navigation argument used to get all of that for
free by going through the generic path. Giving the node a case of its
own and leaving the argument out takes it away.
So an expression carrying any of those three dies under a navigation
where it runs one level out:
DEFINE A AS PREV(s COLLATE "C") > 'a'
ERROR: unrecognized node type: 32
-- (s COLLATE "C") > 'a' returns rows
DEFINE A AS PREV(f(b => 7, a => v)) > 0
ERROR: unrecognized node type: 17
-- f(b => 7, a => v) > 0 returns rows
DEFINE A AS PREV(g(v)) = 100 -- g(a int, b int DEFAULT 100)
ExecInitFunc sizes the FunctionCallInfo from the shortened
argument list and the callee reads past it. In plpgsql I got a
different b on different runs; as a SQL function it raises
"no value found for parameter 2".
-- g(v) = 100 returns rows
I would guess the way out is to let the argument through that pass
with the evaluation turned off for the navigated argument alone,
rather than leaving it out altogether. That would still rewrite away
the nodes the executor cannot handle, while leaving uncomputed the
part that could raise when there is no target row. There seems to be
no reason to turn it off for the offsets: they are resolved once per
execution whether or not a target row is ever reached, so folding
them raises nothing execution would not.
That is a guess, though, and you may well see a better way.
Could you fix this? What is attached is that state: the three cases
go into the regression with their current failures as the expected
output and an XXX saying what each should return. When you fix it,
could you move the expected output to the correct form and take the
XXX out with it?
Best regards,
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
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
