From e41c796f7e6e0e8881c76def90136233519a09b5 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, it missed volatility that folding splices in -- the "default random()" argument of an otherwise STABLE function -- 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 of the FOR PORTION OF check above it. The rule that leaves is that the check sees what the executor would run: what the planner never plans is never examined, so an unreferenced CTE keeps its DEFINE, and so does a subquery whose window no OVER references, which flattening drops before the check is reached. Remove the now-unused validate_rpr_define_volatility() and its helpers, the stale parse_rpr.c comments, and the includes they needed. The separate "sequence operations" message goes too, since contain_volatile_functions() already rejects a NextValueExpr; spell the surviving message in the active voice, like the FOR PORTION OF one. Adjust the regression tests accordingly: a volatile that folds away is now accepted, and the error no longer reports a cursor position. The tests put the four unplanned shapes -- unreferenced CTE, flattened subquery, flattened UNION ALL leaf, and a subquery rel that WHERE false proves empty -- next to the planned ones they resemble, since what separates them is only whether the planner reached the DEFINE. --- src/backend/optimizer/plan/planner.c | 25 ++-- src/backend/optimizer/plan/rpr.c | 77 ---------- src/backend/parser/parse_rpr.c | 8 +- src/include/optimizer/rpr.h | 1 - src/test/regress/expected/rpr.out | 17 +-- src/test/regress/expected/rpr_base.out | 138 +++++++++++++++++- src/test/regress/expected/rpr_integration.out | 4 +- src/test/regress/sql/rpr.sql | 3 +- src/test/regress/sql/rpr_base.sql | 91 +++++++++++- 9 files changed, 241 insertions(+), 123 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index b32cf727ce1..6993d26488a 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -49,7 +49,6 @@ #include "optimizer/planmain.h" #include "optimizer/planner.h" #include "optimizer/prep.h" -#include "optimizer/rpr.h" #include "optimizer/subselect.h" #include "optimizer/tlist.h" #include "parser/analyze.h" @@ -1072,18 +1071,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); @@ -1092,6 +1079,18 @@ 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. A subquery the + * planner discards before reaching this point is therefore not + * checked, which is the same rule that lets a volatile fold away. + */ + if (contain_volatile_functions((Node *) wc->defineClause)) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("DEFINE clause cannot contain volatile functions")); } parse->limitOffset = preprocess_expression(root, parse->limitOffset, diff --git a/src/backend/optimizer/plan/rpr.c b/src/backend/optimizer/plan/rpr.c index dbfc71ab788..a77bfb45fd8 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); @@ -1995,78 +1990,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/parser/parse_rpr.c b/src/backend/parser/parse_rpr.c index 21d3b8899bb..005d25b4b60 100644 --- a/src/backend/parser/parse_rpr.c +++ b/src/backend/parser/parse_rpr.c @@ -402,8 +402,7 @@ transformDefineClause(ParseState *pstate, WindowDef *windef, /* * Validate DEFINE expressions: nested PREV/NEXT, column references, - * compound flatten, volatile callees -- all in a single walk per - * variable. + * compound flatten -- all in a single walk per variable. */ foreach_ptr(TargetEntry, te, defineClause) { @@ -438,9 +437,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 @@ -493,7 +489,7 @@ define_walker(Node *node, void *context) /* * Nested nav inside an outer nav.arg: record for the outer's * compound / nesting decision, then keep recursing so deeper Vars - * and volatile callees are still observed. + * are still observed. */ if (ctx->nav_count == 0) ctx->inner_kind = nav->kind; 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 82980c0e41d..bb7b2f44929 100644 --- a/src/test/regress/expected/rpr.out +++ b/src/test/regress/expected/rpr.out @@ -1217,9 +1217,7 @@ WINDOW w AS ( PATTERN (A) 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 - ^ +ERROR: DEFINE clause cannot contain volatile functions -- Non-constant offset: subquery as offset SELECT price FROM stock WINDOW w AS ( @@ -1254,11 +1252,8 @@ WINDOW w AS ( PATTERN (A+) 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 +ERROR: DEFINE clause cannot contain volatile functions +-- nextval is volatile, so a DEFINE that calls it is rejected CREATE SEQUENCE rpr_seq; SELECT price FROM stock WINDOW w AS ( @@ -1268,9 +1263,7 @@ WINDOW w AS ( PATTERN (A) 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') - ^ +ERROR: DEFINE clause cannot contain volatile functions 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. @@ -1285,7 +1278,7 @@ WINDOW w AS ( DEFINE A AS price > random() * 0 ); SELECT * FROM rpr_volatile_view; -ERROR: volatile functions are not allowed in DEFINE clause +ERROR: DEFINE clause cannot contain volatile functions DROP VIEW rpr_volatile_view; -- DEFINE cannot reference an outer query's column. A correlated outer -- reference must produce a clean error, not the internal "Upper-level Var" diff --git a/src/test/regress/expected/rpr_base.out b/src/test/regress/expected/rpr_base.out index 554a77784fa..95e9cdbf363 100644 --- a/src/test/regress/expected/rpr_base.out +++ b/src/test/regress/expected/rpr_base.out @@ -2233,9 +2233,9 @@ SELECT id, val, count(*) OVER w AS cnt, last_value(id) OVER w AS last_id (5 rows) -- 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; +-- VOLATILE: unqualified is nav; qualified is rejected unless it folds away +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 @@ -2259,9 +2259,135 @@ 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: volatile functions are not allowed in DEFINE clause -LINE 6: DEFINE A AS rpr_navns.prev(val) = -999) - ^ +ERROR: DEFINE clause cannot contain volatile functions +-- accepted: the SQL body inlines and folds to a constant, so no volatile call +-- is left for the check to find +CREATE OR REPLACE FUNCTION prev(integer) RETURNS integer AS 'SELECT -999' + LANGUAGE sql VOLATILE; +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 + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING INITIAL + PATTERN (A+) + DEFINE A AS rpr_navns.prev(val) = -999) + ORDER BY id; + id | val | cnt | last_id +----+-----+-----+--------- + 1 | 100 | 5 | 5 + 2 | 200 | 0 | + 3 | 150 | 0 | + 4 | 140 | 0 | + 5 | 150 | 0 | +(5 rows) + +-- accepted: no OVER references the window, so flattening the subquery drops +-- it before the check runs, the same way an unreferenced CTE is never planned +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 +ORDER BY id; + id +---- + 1 + 2 + 3 + 4 + 5 +(5 rows) + +-- error: OFFSET 0 keeps the subquery, so its DEFINE is checked +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: DEFINE clause cannot contain volatile functions +-- accepted: WHERE false makes the subquery rel dummy, so the planner never +-- plans it and nothing looks at its DEFINE +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 +WHERE false; + id +---- +(0 rows) + +-- accepted: the volatile is in a dead CASE arm that folds away, so nothing +-- volatile is left for the check to find +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: folding can splice in a volatile that parse analysis never saw -- a +-- STABLE function whose default argument is volatile -- and the check runs late +-- enough to catch it +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: DEFINE clause cannot contain volatile functions +DROP FUNCTION rpr_off_leak(bigint); +-- accepted: a UNION ALL leaf is flattened like any other subquery, so its +-- unreferenced window goes the same way +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; + id +---- + 1 + 2 + 3 + 4 + 5 + 1 + 2 + 3 + 4 + 5 +(10 rows) + +-- accepted: an unreferenced CTE is never planned, so nothing looks at its +-- DEFINE +WITH unused AS ( + SELECT id FROM nt + WINDOW w AS ( + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS random() > 0.5)) +SELECT 1; + ?column? +---------- + 1 +(1 row) + +-- error: referencing it plans the CTE, and the check reaches the DEFINE there +WITH used AS ( + SELECT id FROM nt + WINDOW w AS ( + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS random() > 0.5)) +SELECT count(*) FROM used; +ERROR: DEFINE clause cannot contain volatile functions 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 893f61290cc..6dccea30ee2 100644 --- a/src/test/regress/expected/rpr_integration.out +++ b/src/test/regress/expected/rpr_integration.out @@ -1536,9 +1536,7 @@ WINDOW w AS (ORDER BY id PATTERN (A B+) 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) - ^ +ERROR: DEFINE clause cannot contain volatile functions -- ============================================================ -- B10. RPR + Correlated subquery in WHERE -- ============================================================ diff --git a/src/test/regress/sql/rpr.sql b/src/test/regress/sql/rpr.sql index ab8b3e64e18..d2b858ca3a3 100644 --- a/src/test/regress/sql/rpr.sql +++ b/src/test/regress/sql/rpr.sql @@ -635,8 +635,7 @@ WINDOW w AS ( 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 +-- nextval is volatile, so a DEFINE that calls it is rejected CREATE SEQUENCE rpr_seq; SELECT price FROM stock WINDOW w AS ( diff --git a/src/test/regress/sql/rpr_base.sql b/src/test/regress/sql/rpr_base.sql index 18a42535ad0..5cb43d8cabd 100644 --- a/src/test/regress/sql/rpr_base.sql +++ b/src/test/regress/sql/rpr_base.sql @@ -1576,9 +1576,9 @@ SELECT id, val, count(*) OVER w AS cnt, last_value(id) OVER w AS last_id ORDER BY 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; +-- VOLATILE: unqualified is nav; qualified is rejected unless it folds away +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 @@ -1593,6 +1593,91 @@ 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; +-- accepted: the SQL body inlines and folds to a constant, so no volatile call +-- is left for the check to find +CREATE OR REPLACE FUNCTION prev(integer) RETURNS integer AS 'SELECT -999' + LANGUAGE sql VOLATILE; +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 + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING INITIAL + PATTERN (A+) + DEFINE A AS rpr_navns.prev(val) = -999) + ORDER BY id; + +-- accepted: no OVER references the window, so flattening the subquery drops +-- it before the check runs, the same way an unreferenced CTE is never planned +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 +ORDER BY id; + +-- error: OFFSET 0 keeps the subquery, so its DEFINE is checked +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: WHERE false makes the subquery rel dummy, so the planner never +-- plans it and nothing looks at its DEFINE +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 +WHERE false; + +-- accepted: the volatile is in a dead CASE arm that folds away, so nothing +-- volatile is left for the check to find +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: folding can splice in a volatile that parse analysis never saw -- a +-- STABLE function whose default argument is volatile -- and the check runs late +-- enough to catch it +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); + + +-- accepted: a UNION ALL leaf is flattened like any other subquery, so its +-- unreferenced window goes the same way +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; + +-- accepted: an unreferenced CTE is never planned, so nothing looks at its +-- DEFINE +WITH unused AS ( + SELECT id FROM nt + WINDOW w AS ( + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS random() > 0.5)) +SELECT 1; + +-- error: referencing it plans the CTE, and the check reaches the DEFINE there +WITH used AS ( + SELECT id FROM nt + WINDOW w AS ( + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A+) DEFINE A AS random() > 0.5)) +SELECT count(*) FROM used; + DROP FUNCTION prev(integer); -- IMMUTABLE: unqualified is nav; qualified is the escape hatch and succeeds CREATE FUNCTION prev(integer) RETURNS integer AS 'SELECT -999'