From a4451f72219115db77c5a23e7a8e3fbfedbfd1ac Mon Sep 17 00:00:00 2001 From: jian he Date: Thu, 2 Jul 2026 11:44:36 +0800 Subject: [PATCH v50 1/3] refactor DEFINE volatile expression check Based on https://github.com/assam258-5892/postgres/commits/RPR validate_rpr_define_volatility function, along with its helper functions is removed. contain_volatile_functions() already covers both volatile FuncExpr callees and NextValueExpr, so a separate walker is not needed. The trade-off is that we lose the error cursor position, but that seems better than to maintaining extra code. The check is also moved to after preprocess_expression(), following the same pattern as contain_volatile_functions_after_planning. ``` CREATE FUNCTION prev(integer) RETURNS integer AS 'SELECT -999' LANGUAGE sql VOLATILE; ``` SQL language function will b inlined to constant by preprocess_expression, so this is not a VOLATILE function. Discussion: https://postgr.es/m/CACJufxG57=ddtbN=5RZCzhxWDYXvocKmB7NtZy+DoqZuhxb_DA@mail.gmail.com --- src/backend/optimizer/plan/planner.c | 23 +++--- src/backend/optimizer/plan/rpr.c | 72 ------------------- src/backend/parser/parse_rpr.c | 2 +- src/include/optimizer/rpr.h | 1 - src/test/regress/expected/rpr.out | 6 -- src/test/regress/expected/rpr_base.out | 8 +-- src/test/regress/expected/rpr_integration.out | 2 - src/test/regress/sql/rpr_base.sql | 7 +- 8 files changed, 21 insertions(+), 100 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index f01c746615..0f1d347c9c 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -1049,26 +1049,25 @@ 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); wc->endOffset = preprocess_expression(root, wc->endOffset, EXPRKIND_LIMIT); + 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 fac3d7bcf3..1f7e9b4980 100644 --- a/src/backend/optimizer/plan/rpr.c +++ b/src/backend/optimizer/plan/rpr.c @@ -1776,78 +1776,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 ac29c26e20..8cf6d49e90 100644 --- a/src/backend/parser/parse_rpr.c +++ b/src/backend/parser/parse_rpr.c @@ -441,7 +441,7 @@ transformDefineClause(ParseState *pstate, WindowDef *windef, * 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. + * planner time in subquery_planner. * * 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 / diff --git a/src/include/optimizer/rpr.h b/src/include/optimizer/rpr.h index 1de7f86f86..b3841d55a7 100644 --- a/src/include/optimizer/rpr.h +++ b/src/include/optimizer/rpr.h @@ -74,7 +74,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(WindowClause *wc, bool hasMatchStartDependent); /* diff --git a/src/test/regress/expected/rpr.out b/src/test/regress/expected/rpr.out index 52be54039e..241937b44a 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 eb9f43ca2c..1ce63a40ca 100644 --- a/src/test/regress/expected/rpr_base.out +++ b/src/test/regress/expected/rpr_base.out @@ -1971,8 +1971,10 @@ 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 INT AS $$ +BEGIN + RETURN -999; +END; $$ LANGUAGE plpgsql 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 @@ -1997,8 +1999,6 @@ 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) - ^ 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 541ad35de4..1c90e00527 100644 --- a/src/test/regress/expected/rpr_integration.out +++ b/src/test/regress/expected/rpr_integration.out @@ -1401,8 +1401,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 61a5bcf0b8..c9ce59a723 100644 --- a/src/test/regress/sql/rpr_base.sql +++ b/src/test/regress/sql/rpr_base.sql @@ -1420,8 +1420,11 @@ 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 INT AS $$ +BEGIN + RETURN -999; +END; $$ LANGUAGE plpgsql 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 -- 2.34.1