From 6e674514f93d73f4d129ddb31a298ad252866247 Mon Sep 17 00:00:00 2001 From: jian he Date: Tue, 21 Jul 2026 12:38:25 +0900 Subject: [PATCH] Check RPR DEFINE volatility after expression preprocessing A bespoke walker used to reject volatile functions in an RPR DEFINE clause before the planner preprocessed the expression. Running ahead of constant-folding and function inlining, it missed volatility that those steps introduce -- a "default random()" argument, or a volatile SQL function body that gets inlined -- and it rejected volatility that folds away and never executes. Replace it with a single contain_volatile_functions() check on the folded defineClause in subquery_planner(), matching the convention of contain_volatile_functions_after_planning() and the FOR PORTION OF check just below it. An unused RPR WINDOW is dropped when its subquery is flattened, so also check the defineClause in pull_up_subqueries_recurse() to keep a discarded window's DEFINE validated. Remove the now-unused validate_rpr_define_volatility() and its helpers, the stale parse_rpr.c comment, and the includes they needed. Adjust the regression tests accordingly: a volatile that folds away is now accepted, and the error no longer reports a cursor position. --- src/backend/optimizer/plan/planner.c | 22 +++--- src/backend/optimizer/plan/rpr.c | 77 ------------------- src/backend/optimizer/prep/prepjointree.c | 22 ++++++ src/backend/parser/parse_rpr.c | 3 - src/include/optimizer/rpr.h | 1 - src/test/regress/expected/rpr.out | 6 -- src/test/regress/expected/rpr_base.out | 57 +++++++++++++- src/test/regress/expected/rpr_integration.out | 2 - src/test/regress/sql/rpr_base.sql | 48 +++++++++++- 9 files changed, 131 insertions(+), 107 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 2570eec78ea..2dcc27ba93e 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -1070,18 +1070,6 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name, { WindowClause *wc = lfirst_node(WindowClause, l); - /* - * Reject volatile functions (and sequence operations) in an RPR - * DEFINE clause. This is done here, not during parse analysis, to - * follow the convention of not checking expression volatility while - * parsing; debug_query_string still lets us report the offending - * location. Every window clause is visited, including ones not used - * by any OVER, so the check does not depend on the window surviving - * select_active_windows(). - */ - if (wc->rpPattern && wc->defineClause) - validate_rpr_define_volatility(wc->defineClause); - /* partitionClause/orderClause are sort/group expressions */ wc->startOffset = preprocess_expression(root, wc->startOffset, EXPRKIND_LIMIT); @@ -1090,6 +1078,16 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name, wc->defineClause = (List *) preprocess_expression(root, (Node *) wc->defineClause, EXPRKIND_TARGET); + + /* + * Reject volatile expressions in an RPR DEFINE clause. This is done + * here, not during parse analysis, to follow the convention of not + * checking expression volatility while parsing. + */ + if (contain_volatile_functions((Node *) wc->defineClause)) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("volatile functions are not allowed in DEFINE clause")); } parse->limitOffset = preprocess_expression(root, parse->limitOffset, diff --git a/src/backend/optimizer/plan/rpr.c b/src/backend/optimizer/plan/rpr.c index b529be44b14..8f76800d66e 100644 --- a/src/backend/optimizer/plan/rpr.c +++ b/src/backend/optimizer/plan/rpr.c @@ -37,13 +37,8 @@ #include "postgres.h" -#include "catalog/pg_proc.h" -#include "mb/pg_wchar.h" #include "miscadmin.h" -#include "nodes/nodeFuncs.h" #include "optimizer/rpr.h" -#include "tcop/tcopprot.h" -#include "utils/lsyscache.h" /* Forward declarations */ static bool rprPatternEqual(RPRPatternNode *a, RPRPatternNode *b); @@ -1997,78 +1992,6 @@ computeAbsorbability(RPRPattern *pattern) pattern->isAbsorbable = hasAbsorbable; } -/* - * rpr_volatile_func_checker - * check_functions_in_node callback: true if funcid is VOLATILE. - */ -static bool -rpr_volatile_func_checker(Oid funcid, void *context) -{ - return (func_volatile(funcid) == PROVOLATILE_VOLATILE); -} - -/* - * rpr_define_errposition - * Error cursor position for a DEFINE subexpression. - * - * The planner has no ParseState, but the original query text is available in - * debug_query_string, so we can still point at the offending location exactly - * as parser_errposition() would. - */ -static int -rpr_define_errposition(int location) -{ - if (location < 0 || debug_query_string == NULL) - return 0; - return errposition(pg_mbstrlen_with_len(debug_query_string, location) + 1); -} - -/* - * reject_volatile_in_define_walker - * Reject volatile callees and sequence operations anywhere in a DEFINE - * expression: they are non-deterministic across the multiple predicate - * evaluations that NFA backtracking and PREV/NEXT navigation may trigger - * for a single row. - * - * NextValueExpr is checked separately because it is not a function call and - * so is not caught by check_functions_in_node(). - */ -static bool -reject_volatile_in_define_walker(Node *node, void *context) -{ - if (node == NULL) - return false; - if (check_functions_in_node(node, rpr_volatile_func_checker, NULL)) - ereport(ERROR, - errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("volatile functions are not allowed in DEFINE clause"), - rpr_define_errposition(exprLocation(node))); - if (IsA(node, NextValueExpr)) - ereport(ERROR, - errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("sequence operations are not allowed in DEFINE clause"), - rpr_define_errposition(exprLocation(node))); - return expression_tree_walker(node, reject_volatile_in_define_walker, context); -} - -/* - * validate_rpr_define_volatility - * Reject volatile functions / sequence operations in a DEFINE clause. - * - * Called from the planner (subquery_planner) for every RPR WindowClause, - * including windows not referenced by any OVER clause, so the check is applied - * regardless of whether the window survives to execution -- matching the - * coverage of the former parse-time check. - */ -void -validate_rpr_define_volatility(List *defineClause) -{ - foreach_node(TargetEntry, te, defineClause) - { - (void) reject_volatile_in_define_walker((Node *) te->expr, NULL); - } -} - /* * buildRPRPattern * Compile pattern parse tree to flat bytecode array. diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index c9ec3f6a2a7..4342debe162 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -1276,6 +1276,28 @@ pull_up_subqueries_recurse(PlannerInfo *root, Node *jtnode, int varno = ((RangeTblRef *) jtnode)->rtindex; RangeTblEntry *rte = rt_fetch(varno, root->parse->rtable); + if (rte->rtekind == RTE_SUBQUERY && + rte->subquery->windowClause != NIL) + { + foreach_node(WindowClause, wc, rte->subquery->windowClause) + { + /* + * An unused RPR WINDOW in a subquery is dropped when the + * subquery is flattened, so its DEFINE would never reach the + * volatility check in subquery_planner(). Check it here + * instead. Use the after-planning form so a DEFINE whose + * volatility folds away is accepted the same as at top level + * (post-fold), not rejected only because it sits one level + * down. + */ + if (wc->defineClause && + contain_volatile_functions_after_planning((Expr *) wc->defineClause)) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("volatile functions are not allowed in DEFINE clause")); + } + } + /* * Is this a subquery RTE, and if so, is the subquery simple enough to * pull up? diff --git a/src/backend/parser/parse_rpr.c b/src/backend/parser/parse_rpr.c index ac29c26e208..0b8766269e1 100644 --- a/src/backend/parser/parse_rpr.c +++ b/src/backend/parser/parse_rpr.c @@ -440,9 +440,6 @@ transformDefineClause(ParseState *pstate, WindowDef *windef, * - offset_arg / compound_offset_arg must not contain column refs * or nested navigation operations * - * Volatile callees (and sequence operations) are rejected later in the - * planner via validate_rpr_define_volatility(); see optimizer/plan/rpr.c. - * * The walker uses a phase tag to know which subtree it is in: DEFINE * body (top-level), inside a nav.arg, or inside a nav.offset_arg / * compound_offset_arg. When entering an outer nav (PHASE_BODY), it diff --git a/src/include/optimizer/rpr.h b/src/include/optimizer/rpr.h index 20847d89a4a..229f5784c7b 100644 --- a/src/include/optimizer/rpr.h +++ b/src/include/optimizer/rpr.h @@ -79,7 +79,6 @@ #define RPRElemIsFin(e) ((e)->varId == RPR_VARID_FIN) #define RPRElemCanSkip(e) ((e)->min == 0) -extern void validate_rpr_define_volatility(List *defineClause); extern RPRPattern *buildRPRPattern(RPRPatternNode *pattern, List *defineClause, RPSkipTo rpSkipTo, int frameOptions, bool hasMatchStartDependent); diff --git a/src/test/regress/expected/rpr.out b/src/test/regress/expected/rpr.out index 71ac279f6fc..2ca30aa3979 100644 --- a/src/test/regress/expected/rpr.out +++ b/src/test/regress/expected/rpr.out @@ -1218,8 +1218,6 @@ WINDOW w AS ( DEFINE A AS PREV(price, random()::int) > 0 ); ERROR: volatile functions are not allowed in DEFINE clause -LINE 7: DEFINE A AS PREV(price, random()::int) > 0 - ^ -- Non-constant offset: subquery as offset SELECT price FROM stock WINDOW w AS ( @@ -1255,8 +1253,6 @@ WINDOW w AS ( DEFINE A AS PREV(price + random() * 0) >= 0 ); ERROR: volatile functions are not allowed in DEFINE clause -LINE 8: DEFINE A AS PREV(price + random() * 0) >= 0 - ^ -- nextval is volatile (per pg_proc), so it is rejected via the FuncExpr -- path with the "volatile functions" message CREATE SEQUENCE rpr_seq; @@ -1269,8 +1265,6 @@ WINDOW w AS ( DEFINE A AS price > nextval('rpr_seq') ); ERROR: volatile functions are not allowed in DEFINE clause -LINE 7: DEFINE A AS price > nextval('rpr_seq') - ^ DROP SEQUENCE rpr_seq; -- A volatile DEFINE is now rejected in the planner, not at parse time, so a -- view that hides one is created successfully and only errors when read. diff --git a/src/test/regress/expected/rpr_base.out b/src/test/regress/expected/rpr_base.out index 2dd2f9f7b2c..81e03c76daa 100644 --- a/src/test/regress/expected/rpr_base.out +++ b/src/test/regress/expected/rpr_base.out @@ -2126,8 +2126,8 @@ SELECT id, val, count(*) OVER w AS cnt, last_value(id) OVER w AS last_id -- A qualified call invokes the function, so its volatility still matters -- VOLATILE: unqualified is nav; qualified is rejected as a volatile function -CREATE FUNCTION prev(integer) RETURNS integer AS 'SELECT -999' - LANGUAGE sql VOLATILE; +CREATE FUNCTION prev(integer) RETURNS integer + LANGUAGE plpgsql VOLATILE AS 'BEGIN RETURN -999; END'; SELECT id, val, count(*) OVER w AS cnt, last_value(id) OVER w AS last_id FROM nt WINDOW w AS (PARTITION BY g ORDER BY id @@ -2152,8 +2152,57 @@ SELECT id, val, count(*) OVER w AS cnt, last_value(id) OVER w AS last_id DEFINE A AS rpr_navns.prev(val) = -999) ORDER BY id; ERROR: volatile functions are not allowed in DEFINE clause -LINE 6: DEFINE A AS rpr_navns.prev(val) = -999) - ^ +-- error +SELECT id FROM ( + SELECT id FROM nt + WINDOW w AS ( + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS random() > 0.5)) s; +ERROR: volatile functions are not allowed in DEFINE clause +-- error +SELECT id FROM ( + SELECT id FROM nt + WINDOW w AS ( + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS random() > 0.5) OFFSET 0) sub; +ERROR: volatile functions are not allowed in DEFINE clause +-- accepted: the volatile is in a dead CASE arm that folds away, so a +-- pulled-up subquery window is no stricter here than at top level +SELECT id FROM ( + SELECT id FROM nt + WINDOW w AS ( + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS CASE WHEN false THEN random()::int > 0 + ELSE val > 5 END)) s +ORDER BY id; + id +---- + 1 + 2 + 3 + 4 + 5 +(5 rows) + +-- error: a volatile spliced in by folding after the gate -- a STABLE function +-- whose default argument is volatile -- is still caught by the post-fold check +CREATE FUNCTION rpr_off_leak(n bigint DEFAULT (random() * 5)::bigint) + RETURNS bigint LANGUAGE sql STABLE AS 'SELECT n'; +SELECT count(*) OVER w FROM generate_series(1, 100) g(v) + WINDOW w AS (ORDER BY v ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS v > PREV(v, rpr_off_leak())); +ERROR: volatile functions are not allowed in DEFINE clause +DROP FUNCTION rpr_off_leak(bigint); +-- error: a UNION ALL leaf reaches the pull-up guard too, so its unused RPR +-- window with a volatile DEFINE is rejected like the top-level case +SELECT id FROM ( + SELECT id FROM nt + WINDOW w AS ( + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS random() > 0.5) + UNION ALL + SELECT id FROM nt) s; +ERROR: volatile functions are not allowed in DEFINE clause DROP FUNCTION prev(integer); -- IMMUTABLE: unqualified is nav; qualified is the escape hatch and succeeds CREATE FUNCTION prev(integer) RETURNS integer AS 'SELECT -999' diff --git a/src/test/regress/expected/rpr_integration.out b/src/test/regress/expected/rpr_integration.out index 3017b387ecc..bf9f9766836 100644 --- a/src/test/regress/expected/rpr_integration.out +++ b/src/test/regress/expected/rpr_integration.out @@ -1495,8 +1495,6 @@ WINDOW w AS (ORDER BY id DEFINE B AS val > PREV(val) AND random() >= 0.0) ORDER BY id; ERROR: volatile functions are not allowed in DEFINE clause -LINE 6: DEFINE B AS val > PREV(val) AND random() >= 0.0) - ^ -- ============================================================ -- B10. RPR + Correlated subquery in WHERE -- ============================================================ diff --git a/src/test/regress/sql/rpr_base.sql b/src/test/regress/sql/rpr_base.sql index 8cb05137ab5..09b7dc56596 100644 --- a/src/test/regress/sql/rpr_base.sql +++ b/src/test/regress/sql/rpr_base.sql @@ -1510,8 +1510,8 @@ SELECT id, val, count(*) OVER w AS cnt, last_value(id) OVER w AS last_id -- A qualified call invokes the function, so its volatility still matters -- VOLATILE: unqualified is nav; qualified is rejected as a volatile function -CREATE FUNCTION prev(integer) RETURNS integer AS 'SELECT -999' - LANGUAGE sql VOLATILE; +CREATE FUNCTION prev(integer) RETURNS integer + LANGUAGE plpgsql VOLATILE AS 'BEGIN RETURN -999; END'; SELECT id, val, count(*) OVER w AS cnt, last_value(id) OVER w AS last_id FROM nt WINDOW w AS (PARTITION BY g ORDER BY id @@ -1526,6 +1526,50 @@ SELECT id, val, count(*) OVER w AS cnt, last_value(id) OVER w AS last_id PATTERN (A+) DEFINE A AS rpr_navns.prev(val) = -999) ORDER BY id; + +-- error +SELECT id FROM ( + SELECT id FROM nt + WINDOW w AS ( + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS random() > 0.5)) s; + +-- error +SELECT id FROM ( + SELECT id FROM nt + WINDOW w AS ( + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS random() > 0.5) OFFSET 0) sub; + +-- accepted: the volatile is in a dead CASE arm that folds away, so a +-- pulled-up subquery window is no stricter here than at top level +SELECT id FROM ( + SELECT id FROM nt + WINDOW w AS ( + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS CASE WHEN false THEN random()::int > 0 + ELSE val > 5 END)) s +ORDER BY id; + +-- error: a volatile spliced in by folding after the gate -- a STABLE function +-- whose default argument is volatile -- is still caught by the post-fold check +CREATE FUNCTION rpr_off_leak(n bigint DEFAULT (random() * 5)::bigint) + RETURNS bigint LANGUAGE sql STABLE AS 'SELECT n'; +SELECT count(*) OVER w FROM generate_series(1, 100) g(v) + WINDOW w AS (ORDER BY v ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS v > PREV(v, rpr_off_leak())); +DROP FUNCTION rpr_off_leak(bigint); + +-- error: a UNION ALL leaf reaches the pull-up guard too, so its unused RPR +-- window with a volatile DEFINE is rejected like the top-level case +SELECT id FROM ( + SELECT id FROM nt + WINDOW w AS ( + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS random() > 0.5) + UNION ALL + SELECT id FROM nt) s; + DROP FUNCTION prev(integer); -- IMMUTABLE: unqualified is nav; qualified is the escape hatch and succeeds CREATE FUNCTION prev(integer) RETURNS integer AS 'SELECT -999' -- 2.50.1 (Apple Git-155)