From b131bdc3e25cc063d33c8bab403087f40f198792 Mon Sep 17 00:00:00 2001 From: Henson Choi Date: Wed, 10 Jun 2026 16:58:55 +0900 Subject: [PATCH 73/77] Reject volatile DEFINE expressions in the planner, not at parse time The DEFINE clause walker rejected volatile functions and sequence operations during parse analysis, but checking expression volatility belongs after parsing. Move the check to subquery_planner, which visits every RPR WindowClause -- including windows no OVER references -- via the new validate_rpr_define_volatility() in rpr.c, preserving the former coverage. The planner has no ParseState, so the error cursor is recovered from debug_query_string, reproducing the parse-time position for ordinary query execution. A side effect is that a volatile DEFINE hidden in a view is now accepted at CREATE VIEW time and only rejected when the view is read; add a regression test covering that. No change for direct queries. --- src/backend/optimizer/plan/planner.c | 13 +++ src/backend/optimizer/plan/rpr.c | 80 +++++++++++++++++++ src/backend/parser/parse_rpr.c | 43 ++-------- src/include/optimizer/rpr.h | 1 + src/test/regress/expected/rpr.out | 17 +++- src/test/regress/expected/rpr_integration.out | 2 +- src/test/regress/sql/rpr.sql | 17 +++- src/test/regress/sql/rpr_integration.sql | 2 +- 8 files changed, 133 insertions(+), 42 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index f43cc0edb37..d20015cd862 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -49,6 +49,7 @@ #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" @@ -1048,6 +1049,18 @@ 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); diff --git a/src/backend/optimizer/plan/rpr.c b/src/backend/optimizer/plan/rpr.c index 09445391e8c..175777a8ffc 100644 --- a/src/backend/optimizer/plan/rpr.c +++ b/src/backend/optimizer/plan/rpr.c @@ -39,10 +39,14 @@ #include +#include "catalog/pg_proc.h" +#include "mb/pg_wchar.h" #include "miscadmin.h" #include "nodes/makefuncs.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); @@ -1875,6 +1879,82 @@ collectPatternVariables(RPRPatternNode *pattern) return varNames; } +/* + * 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) +{ + ListCell *lc; + + foreach(lc, defineClause) + { + TargetEntry *te = lfirst_node(TargetEntry, lc); + + (void) reject_volatile_in_define_walker((Node *) te->expr, NULL); + } +} + /* * buildDefineVariableList * Build defineVariableList from defineClause. diff --git a/src/backend/parser/parse_rpr.c b/src/backend/parser/parse_rpr.c index d12a0d45c94..a1cd5ad8d38 100644 --- a/src/backend/parser/parse_rpr.c +++ b/src/backend/parser/parse_rpr.c @@ -25,7 +25,6 @@ #include "postgres.h" -#include "catalog/pg_proc.h" #include "miscadmin.h" #include "nodes/makefuncs.h" #include "nodes/nodeFuncs.h" @@ -36,7 +35,6 @@ #include "parser/parse_expr.h" #include "parser/parse_rpr.h" #include "parser/parse_target.h" -#include "utils/lsyscache.h" /* DEFINE clause walker context -- see define_walker for usage. */ typedef enum @@ -61,7 +59,6 @@ static void validateRPRPatternVarCount(ParseState *pstate, RPRPatternNode *node, List *rpDefs, List **varNames); static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist); -static bool nav_volatile_func_checker(Oid funcid, void *context); static bool define_walker(Node *node, void *context); /* @@ -473,16 +470,15 @@ transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, * * One walker function (define_walker) visits every node in a DEFINE * expression exactly once and enforces every rule: - * - Volatile callees and NextValueExpr are rejected at parse time - * (RPR's NFA may evaluate the same row's predicate multiple times - * during backtracking, so a volatile result would make matching - * non-deterministic). * - For each outer RPRNavExpr (per ISO/IEC 19075-5 5.6.4 nesting rules): * * arg must contain at least one column reference * * PREV/NEXT wrapping FIRST/LAST flattens to a compound kind * * Other nestings are rejected (FIRST(PREV()), PREV(PREV()), ...) * * offset_arg / compound_offset_arg must not contain column refs * + * 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 @@ -492,30 +488,18 @@ transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, * constant-offset rule. No subtree is walked twice. */ -/* - * nav_volatile_func_checker - * check_functions_in_node callback: true if funcid is VOLATILE. - */ -static bool -nav_volatile_func_checker(Oid funcid, void *context) -{ - return (func_volatile(funcid) == PROVOLATILE_VOLATILE); -} - /* * define_walker * Single-pass DEFINE clause validator. At each node, enforces: * - * [1] no volatile callees (and no NextValueExpr) -- anywhere in - * the tree, regardless of phase - * [2] for each outer RPRNavExpr (PHASE_BODY -> PHASE_NAV_ARG): + * [1] for each outer RPRNavExpr (PHASE_BODY -> PHASE_NAV_ARG): * - nav.arg must contain at least one column reference * - PREV/NEXT wrapping FIRST/LAST is flattened in place * to a compound kind (PREV_FIRST, PREV_LAST, NEXT_FIRST, * NEXT_LAST) * - any other nesting is rejected (FIRST(PREV()), * PREV(PREV()), FIRST(FIRST()), three-or-more deep) - * [3] for each nav offset (PHASE_NAV_OFFSET): + * [2] for each nav offset (PHASE_NAV_OFFSET): * - must be a run-time constant (no column references) * * Var sightings feed the column-ref rule for the enclosing nav scope; @@ -531,23 +515,6 @@ define_walker(Node *node, void *context) if (node == NULL) return false; - /* - * Reject volatile callees and sequence operations anywhere in the DEFINE - * clause: they are non-deterministic across the multiple predicate - * evaluations that NFA backtracking and PREV/NEXT navigation may trigger - * for a single row. - */ - if (check_functions_in_node(node, nav_volatile_func_checker, NULL)) - ereport(ERROR, - errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("volatile functions are not allowed in DEFINE clause"), - parser_errposition(ctx->pstate, exprLocation(node))); - if (IsA(node, NextValueExpr)) - ereport(ERROR, - errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("sequence operations are not allowed in DEFINE clause"), - parser_errposition(ctx->pstate, exprLocation(node))); - /* Var sighting feeds the column-ref rule for the enclosing nav scope. */ if (IsA(node, Var) && (ctx->phase == DEFINE_PHASE_NAV_ARG || diff --git a/src/include/optimizer/rpr.h b/src/include/optimizer/rpr.h index 8e0bc7efc53..802d2f1dd69 100644 --- a/src/include/optimizer/rpr.h +++ b/src/include/optimizer/rpr.h @@ -67,6 +67,7 @@ #define RPRElemCanSkip(e) ((e)->min == 0) extern List *collectPatternVariables(RPRPatternNode *pattern); +extern void validate_rpr_define_volatility(List *defineClause); extern void buildDefineVariableList(List *defineClause, List **defineVariableList); extern RPRPattern *buildRPRPattern(RPRPatternNode *pattern, List *defineVariableList, diff --git a/src/test/regress/expected/rpr.out b/src/test/regress/expected/rpr.out index 1b409b923dd..c02c9d75a9a 100644 --- a/src/test/regress/expected/rpr.out +++ b/src/test/regress/expected/rpr.out @@ -1267,7 +1267,7 @@ WINDOW w AS ( ERROR: cannot use subquery in DEFINE expression LINE 7: DEFINE A AS PREV(price + (SELECT 1)) > 0 ^ --- Volatile function inside nav.arg is rejected at parse time +-- Volatile function inside nav.arg is rejected in the planner SELECT company, tdate, price, first_value(price) OVER w, last_value(price) OVER w, count(*) OVER w FROM stock @@ -1295,6 +1295,21 @@ 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. +CREATE TEMP VIEW rpr_volatile_view AS +SELECT company, tdate, price, count(*) OVER w +FROM stock +WINDOW w AS ( + PARTITION BY company ORDER BY tdate + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + INITIAL + PATTERN (A+) + DEFINE A AS price > random() * 0 +); +SELECT * FROM rpr_volatile_view; +ERROR: volatile functions are not allowed in DEFINE clause +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" -- elog that pull_var_clause would otherwise raise. diff --git a/src/test/regress/expected/rpr_integration.out b/src/test/regress/expected/rpr_integration.out index 2133e2dfe13..80a32cca9ab 100644 --- a/src/test/regress/expected/rpr_integration.out +++ b/src/test/regress/expected/rpr_integration.out @@ -1370,7 +1370,7 @@ DROP INDEX rpr_integ_id_idx; -- ============================================================ -- B9. RPR + Volatile function in DEFINE -- ============================================================ --- Volatile functions in DEFINE are rejected at parse time. Under +-- Volatile functions in DEFINE are rejected in the planner. Under -- RPR's NFA engine the same row's DEFINE predicate may be evaluated -- multiple times (backtracking, PREV/NEXT navigation), so a volatile -- result would make pattern matching non-deterministic. STABLE and diff --git a/src/test/regress/sql/rpr.sql b/src/test/regress/sql/rpr.sql index 56dff9b6725..b15b30c85ac 100644 --- a/src/test/regress/sql/rpr.sql +++ b/src/test/regress/sql/rpr.sql @@ -643,7 +643,7 @@ WINDOW w AS ( DEFINE A AS PREV(price + (SELECT 1)) > 0 ); --- Volatile function inside nav.arg is rejected at parse time +-- Volatile function inside nav.arg is rejected in the planner SELECT company, tdate, price, first_value(price) OVER w, last_value(price) OVER w, count(*) OVER w FROM stock @@ -667,6 +667,21 @@ WINDOW w AS ( ); 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. +CREATE TEMP VIEW rpr_volatile_view AS +SELECT company, tdate, price, count(*) OVER w +FROM stock +WINDOW w AS ( + PARTITION BY company ORDER BY tdate + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + INITIAL + PATTERN (A+) + DEFINE A AS price > random() * 0 +); +SELECT * FROM rpr_volatile_view; +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" -- elog that pull_var_clause would otherwise raise. diff --git a/src/test/regress/sql/rpr_integration.sql b/src/test/regress/sql/rpr_integration.sql index 24b0b1811b9..1d6075265bb 100644 --- a/src/test/regress/sql/rpr_integration.sql +++ b/src/test/regress/sql/rpr_integration.sql @@ -860,7 +860,7 @@ DROP INDEX rpr_integ_id_idx; -- ============================================================ -- B9. RPR + Volatile function in DEFINE -- ============================================================ --- Volatile functions in DEFINE are rejected at parse time. Under +-- Volatile functions in DEFINE are rejected in the planner. Under -- RPR's NFA engine the same row's DEFINE predicate may be evaluated -- multiple times (backtracking, PREV/NEXT navigation), so a volatile -- result would make pattern matching non-deterministic. STABLE and -- 2.50.1 (Apple Git-155)