Hi, Since 18, PL/pgSQL can hand a local variable's read/write expanded value to the expression assigned to it, when the expression refers to the variable once (the "transfer" method of 6c7251db0ce [1]). When the assignment's simple expression is busy, PL/pgSQL runs the expression through SPI instead; if SPI has replanned it in the meantime, the new plan's references to the variable get that treatment too, and a build without assertions can give a wrong answer:
SET plan_cache_mode = force_generic_plan;
CREATE FUNCTION g(a text[], x text) RETURNS text[] LANGUAGE sql
AS 'SELECT array_append(a, x)';
CREATE FUNCTION 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
CREATE OR REPLACE FUNCTION g(a text[], x text) RETURNS text[]
LANGUAGE sql AS 'SELECT a || a';
END IF;
a[1] := 'z';
a := g(a, f(d - 1));
RETURN d || ':' || array_to_string(a, ',');
END $$;
SELECT f(3);
f
-------------------
3:z,y,2:z,y,1:z,y
(1 row)
That is what master and 18 built without assertions return. f(1)
should have doubled its array, giving
3:z,y,2:z,y,1:z,y,z,y, which is what 17 returns, and master with the
attached patch. On an assertion-enabled build of master, 19 or 18,
the same script fails an assertion instead (line 6570 on 18):
TRAP: failed Assert("param == expr->expr_rw_param"), File: "pl_exec.c",
Line: 6678
What happens: the outermost call evaluates the assignment as a simple
expression, and on the first read/write value that
plpgsql_param_eval_var_check() sees, exec_check_rw_parameter() decides
that "a" can be transferred and remembers that Param in
expr->expr_rw_param. The recursive calls find the simple
expression in use and run the same PLpgSQL_expr through SPI
(exec_run_select). With force_generic_plan, SPI uses the same generic
plan, so the Param is the same node. But the CREATE OR REPLACE at the
bottom of the recursion invalidates that plan, SPI replans, and
plpgsql_param_compile() gives the new plan's Params the same checking
callback, since expr_simple_expr is still set. The "transfer" case
asserts that it can only meet expr_rw_param; without assertions, the
first reference in the new plan (a || a) takes the value and leaves the
variable null, and the second reads the null. (The simple-expression
path resets expr_rwopt and expr_rw_param when it replans; the SPI path
doesn't touch them.)
force_generic_plan only makes it quick to reproduce: with the default
setting, the plan cache moves to the generic plan after five custom
plans, and the same functions without the SET give the wrong answer
for f(8). Recursion isn't needed either: an error raised inside the
expression and caught by an exception block leaves the expression
marked busy for the rest of the transaction, so later calls go through
SPI as well. And if the simple expression's own tree doesn't refer to
the variable (say, g's original body ignores "a"), the first read/write
value is seen by a Param of SPI's new plan, and
exec_check_rw_parameter() fails Assert(context.count > 0) while
examining the old tree.
The attached patch installs the checking callback only while compiling
the simple expression's own tree, which is what
exec_check_rw_parameter() assumes ("we only apply this optimization
within simple expressions"). Expressions run through SPI get read-only
references, as non-simple expressions always do; that costs a copy of
the value at those levels only, in a path that flattens its result
anyway.
A simpler trigger, ALTER FUNCTION ... COST on a function the
assignment calls, fails the same assertion, but gives the right answer
without assertions: the rebuilt plan refers to the variable once, so
transferring through it is harmless there.
The patch adds the script above, as a regression test, to
plpgsql_array.sql after the existing "transfer" and "inplace" tests,
with a second call after redefining g to ignore "a". On unpatched
master the first call returns the wrong answer, or fails the assertion;
with only the "transfer" case relaxed, the second call fails
Assert(context.count > 0). With the patch they return
3:z,y,2:z,y,1:z,y,z,y and 3:2:1:z,y,z,y.
On master, check-world passes (without the TAP tests), and the PL/pgSQL
tests pass with the server running under Valgrind.
The "transfer" method is new in 18, so I think the fix should be
back-patched to 18. The patch is against master and cherry-picks
without conflicts to 19 and 18.
[1]
https://postgr.es/m/CACxu=vjakfnsyxoosnw1wegsao5u_v1xybacfvj14wgjv_p...@mail.gmail.com
Regards,
Muzzammil Sarwar
v1-0001-Fix-plpgsql-s-R-W-optimization-for-simple-express.patch
Description: Binary data
