From bd3300f63633a6963b52f67c81e07578db4e29e1 Mon Sep 17 00:00:00 2001 From: Muzzammil Sarwar Date: Thu, 15 Oct 2026 04:44:45 +0500 Subject: [PATCH v1] Fix plpgsql's R/W optimization for simple expressions run via SPI. plpgsql_param_compile() installs plpgsql_param_eval_var_check() on Params referencing an assignment's target variable whenever the expression is simple, but that callback and exec_check_rw_parameter() assume the Param belongs to expr->expr_simple_expr. That is not so when the simple expression is busy (in a recursive call, or after an error abandoned an evaluation earlier in the transaction), so that we run it through SPI instead, and SPI has replanned it since then: the new plan's Params get the same callback. Assertion-enabled builds failed an assertion. Other builds could apply the "transfer" method of 6c7251db0ce through a Param it was not chosen for; if the new plan referenced the variable twice (say, after an inlined SQL function was redefined), the first reference took the variable's value and the second read a null, giving a wrong result. Install the checking callback only while compiling the simple expression itself. As exec_check_rw_parameter() already says, the optimization is pointless in the exec_run_select() code path anyway, since that path flattens the result. Back-patch to v18, where the "transfer" method was introduced. --- src/pl/plpgsql/src/expected/plpgsql_array.out | 38 +++++++++++++++++++ src/pl/plpgsql/src/pl_exec.c | 13 ++++++- src/pl/plpgsql/src/sql/plpgsql_array.sql | 29 ++++++++++++++ 3 files changed, 78 insertions(+), 2 deletions(-) diff --git a/src/pl/plpgsql/src/expected/plpgsql_array.out b/src/pl/plpgsql/src/expected/plpgsql_array.out index 4c6b3ce998a..7668d1c859b 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_array.out +++ b/src/pl/plpgsql/src/expected/plpgsql_array.out @@ -62,6 +62,44 @@ begin raise notice 'a = %', a; end$$; NOTICE: a = {1,1,2,3,42,3,1,1,2,3,42,3} +-- "transfer" assignment whose plan is rebuilt while an outer evaluation of +-- it is in progress: the new plan's references to the target must not +-- take its value +set plan_cache_mode = force_generic_plan; +create function rw_g(a text[], x text) returns text[] language sql +as 'select array_append(a, x)'; +create function rw_f(d int) returns text language plpgsql as $$ +declare a text[] := '{x,y}'; +begin + if d = 0 then return ''; end if; + if d = 1 then + -- invalidates the plan of the assignment below, whose new plan + -- refers to "a" twice + create or replace function rw_g(a text[], x text) returns text[] + language sql as 'select a || a'; + end if; + a[1] := 'z'; + a := rw_g(a, rw_f(d - 1)); + return d || ':' || array_to_string(a, ','); +end $$; +select rw_f(3); + rw_f +----------------------- + 3:z,y,2:z,y,1:z,y,z,y +(1 row) + +-- likewise when the outer evaluation's plan does not refer to "a" at all +create or replace function rw_g(a text[], x text) returns text[] +language sql as 'select array[x]'; +select rw_f(3); + rw_f +--------------- + 3:2:1:z,y,z,y +(1 row) + +reset plan_cache_mode; +drop function rw_f(int); +drop function rw_g(text[], text); create temp table onecol as select array[1,2] as f1; do $$ declare a int[]; begin a := f1 from onecol; raise notice 'a = %', a; end$$; diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index bc1667f7a13..7124db967bb 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -6563,7 +6563,14 @@ plpgsql_param_compile(ParamListInfo params, Param *param, * then it's possible we will be able to optimize handling of R/W expanded * datums. We don't want to do the work needed to determine that unless * we actually see a R/W expanded datum at runtime, so install a checking - * function that will figure that out when needed. + * function that will figure that out when needed. Do that only while + * compiling the simple expression's own tree, though. If the simple + * expression is busy (we are in a recursive call, or an earlier + * evaluation in this transaction was abandoned by an error), + * exec_eval_expr runs the expression through SPI instead, and SPI's plan + * might have been rebuilt since, so its Params need not be the ones + * exec_check_rw_parameter examines. The optimization would be pointless + * in that code path anyway, since it flattens the result. * * Otherwise, it seems worth special-casing DTYPE_VAR and DTYPE_RECFIELD * for performance. Also, we can determine in advance whether @@ -6574,7 +6581,9 @@ plpgsql_param_compile(ParamListInfo params, Param *param, { bool isvarlena = (((PLpgSQL_var *) datum)->datatype->typlen == -1); - if (isvarlena && dno == expr->target_param && expr->expr_simple_expr) + if (isvarlena && dno == expr->target_param && + expr->expr_simple_expr != NULL && + state->expr == expr->expr_simple_expr) scratch.d.cparam.paramfunc = plpgsql_param_eval_var_check; else if (isvarlena) scratch.d.cparam.paramfunc = plpgsql_param_eval_var_ro; diff --git a/src/pl/plpgsql/src/sql/plpgsql_array.sql b/src/pl/plpgsql/src/sql/plpgsql_array.sql index da984a99414..2f92a4661aa 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_array.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_array.sql @@ -58,6 +58,35 @@ begin raise notice 'a = %', a; end$$; +-- "transfer" assignment whose plan is rebuilt while an outer evaluation of +-- it is in progress: the new plan's references to the target must not +-- take its value +set plan_cache_mode = force_generic_plan; +create function rw_g(a text[], x text) returns text[] language sql +as 'select array_append(a, x)'; +create function rw_f(d int) returns text language plpgsql as $$ +declare a text[] := '{x,y}'; +begin + if d = 0 then return ''; end if; + if d = 1 then + -- invalidates the plan of the assignment below, whose new plan + -- refers to "a" twice + create or replace function rw_g(a text[], x text) returns text[] + language sql as 'select a || a'; + end if; + a[1] := 'z'; + a := rw_g(a, rw_f(d - 1)); + return d || ':' || array_to_string(a, ','); +end $$; +select rw_f(3); +-- likewise when the outer evaluation's plan does not refer to "a" at all +create or replace function rw_g(a text[], x text) returns text[] +language sql as 'select array[x]'; +select rw_f(3); +reset plan_cache_mode; +drop function rw_f(int); +drop function rw_g(text[], text); + create temp table onecol as select array[1,2] as f1; do $$ declare a int[]; -- 2.47.3