From ec210914c751e715913405e25d734f859f79a47a Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 13 Jun 2024 13:37:46 -0400 Subject: [PATCH] When replanning a plpgsql "simple expression", check it's still simple. The previous coding here assumed that we didn't need to recheck any of the querytree tests made in exec_simple_check_plan(). I think we supposed that those properties were fully determined by the syntax of the source text and hence couldn't change. That is true for most of them, but at least hasTargetSRFs and hasAggs can change by dint of forcibly dropping an originally-referenced function and recreating it with new properties. That leads to "unexpected plan node type" or similar failures. These tests are pretty cheap compared to the cost of replanning, so rather than sweat over exactly which properties need to be rechecked, let's just recheck them all. Hence, factor out those tests into a new function exec_is_simple_query(), and rearrange callers as needed. A second problem in the same area was that if we failed during replanning or during exec_save_simple_expr(), we'd potentially leave behind now-dangling pointers to the old simple expression, potentially resulting in crashes later. To fix, clear those pointers before replanning. The v12 code looks quite different in this area but still has the bug about needing to recheck query simplicity. I chose to back-patch all of the plpgsql_simple.sql test script, which formerly didn't exist in this branch. Per bug #18497 from Nikita Kalinin. Back-patch to all supported branches. Discussion: https://postgr.es/m/18497-fe93b6da82ce31d4@postgresql.org --- src/pl/plpgsql/src/Makefile | 2 +- .../plpgsql/src/expected/plpgsql_simple.out | 120 ++++++++++++++++++ src/pl/plpgsql/src/pl_exec.c | 89 ++++++++----- src/pl/plpgsql/src/sql/plpgsql_simple.sql | 104 +++++++++++++++ 4 files changed, 285 insertions(+), 30 deletions(-) create mode 100644 src/pl/plpgsql/src/expected/plpgsql_simple.out create mode 100644 src/pl/plpgsql/src/sql/plpgsql_simple.sql diff --git a/src/pl/plpgsql/src/Makefile b/src/pl/plpgsql/src/Makefile index af9f6fb209..2057e8a99c 100644 --- a/src/pl/plpgsql/src/Makefile +++ b/src/pl/plpgsql/src/Makefile @@ -27,7 +27,7 @@ DATA = plpgsql.control plpgsql--1.0.sql plpgsql--unpackaged--1.0.sql REGRESS_OPTS = --dbname=$(PL_TESTDB) REGRESS = plpgsql_call plpgsql_control plpgsql_domain plpgsql_record \ - plpgsql_cache plpgsql_transaction plpgsql_trap \ + plpgsql_cache plpgsql_simple plpgsql_transaction plpgsql_trap \ plpgsql_trigger plpgsql_varprops # where to find gen_keywordlist.pl and subsidiary files diff --git a/src/pl/plpgsql/src/expected/plpgsql_simple.out b/src/pl/plpgsql/src/expected/plpgsql_simple.out new file mode 100644 index 0000000000..7b22e60f19 --- /dev/null +++ b/src/pl/plpgsql/src/expected/plpgsql_simple.out @@ -0,0 +1,120 @@ +-- +-- Tests for plpgsql's handling of "simple" expressions +-- +-- Check that changes to an inline-able function are handled correctly +create function simplesql(int) returns int language sql +as 'select $1'; +create function simplecaller() returns int language plpgsql +as $$ +declare + sum int := 0; +begin + for n in 1..10 loop + sum := sum + simplesql(n); + if n = 5 then + create or replace function simplesql(int) returns int language sql + as 'select $1 + 100'; + end if; + end loop; + return sum; +end$$; +select simplecaller(); + simplecaller +-------------- + 555 +(1 row) + +-- Check that changes in search path are dealt with correctly +create schema simple1; +create function simple1.simpletarget(int) returns int language plpgsql +as $$begin return $1; end$$; +create function simpletarget(int) returns int language plpgsql +as $$begin return $1 + 100; end$$; +create or replace function simplecaller() returns int language plpgsql +as $$ +declare + sum int := 0; +begin + for n in 1..10 loop + sum := sum + simpletarget(n); + if n = 5 then + set local search_path = 'simple1'; + end if; + end loop; + return sum; +end$$; +select simplecaller(); + simplecaller +-------------- + 555 +(1 row) + +-- try it with non-volatile functions, too +alter function simple1.simpletarget(int) immutable; +alter function simpletarget(int) immutable; +select simplecaller(); + simplecaller +-------------- + 555 +(1 row) + +-- make sure flushing local caches changes nothing +\c - +select simplecaller(); + simplecaller +-------------- + 555 +(1 row) + +-- Check case where first attempt to determine if it's simple fails +create function simplesql() returns int language sql +as $$select 1 / 0$$; +create or replace function simplecaller() returns int language plpgsql +as $$ +declare x int; +begin + select simplesql() into x; + return x; +end$$; +select simplecaller(); -- division by zero occurs during simple-expr check +ERROR: division by zero +CONTEXT: SQL function "simplesql" during inlining +SQL statement "select simplesql()" +PL/pgSQL function simplecaller() line 4 at SQL statement +create or replace function simplesql() returns int language sql +as $$select 2 + 2$$; +select simplecaller(); + simplecaller +-------------- + 4 +(1 row) + +-- Check case where called function changes from non-SRF to SRF (bug #18497) +create or replace function simplecaller() returns int language plpgsql +as $$ +declare x int; +begin + x := simplesql(); + return x; +end$$; +select simplecaller(); + simplecaller +-------------- + 4 +(1 row) + +drop function simplesql(); +create function simplesql() returns setof int language sql +as $$select 22 + 22$$; +select simplecaller(); + simplecaller +-------------- + 44 +(1 row) + +select simplecaller(); + simplecaller +-------------- + 44 +(1 row) + diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 9fc92a77bf..bfb70135d2 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -339,6 +339,7 @@ static void exec_prepare_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr, int cursorOptions, bool keepplan); static void exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr); +static bool exec_is_simple_query(PLpgSQL_expr *expr); static void exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan); static void exec_check_rw_parameter(PLpgSQL_expr *expr, int target_dno); static bool contains_target_param(Node *node, int *target_dno); @@ -6216,6 +6217,19 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, */ Assert(cplan != NULL); + /* + * However, the plan might now be non-simple, in edge-case scenarios such + * as a non-SRF having been replaced with a SRF. + */ + if (!exec_is_simple_query(expr)) + { + /* Release SPI_plan_get_cached_plan's refcount */ + ReleaseCachedPlan(cplan, true); + /* Mark expression as non-simple, and fail */ + expr->expr_simple_expr = NULL; + return false; + } + /* If it got replanned, update our copy of the simple expression */ if (cplan->generation != expr->expr_simple_generation) { @@ -8023,9 +8037,6 @@ get_cast_hashentry(PLpgSQL_execstate *estate, static void exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) { - List *plansources; - CachedPlanSource *plansource; - Query *query; CachedPlan *cplan; MemoryContext oldcontext; @@ -8040,31 +8051,66 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) * called immediately after creating the CachedPlanSource, we need not * worry about the query being stale. */ + if (!exec_is_simple_query(expr)) + return; /* - * We can only test queries that resulted in exactly one CachedPlanSource + * Get the generic plan for the query. If replanning is needed, do that + * work in the eval_mcontext. (Note that replanning could throw an error, + * in which case the expr is left marked "not simple", which is fine.) + */ + oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate)); + cplan = SPI_plan_get_cached_plan(expr->plan); + MemoryContextSwitchTo(oldcontext); + + /* Can't fail, because we checked for a single CachedPlanSource above */ + Assert(cplan != NULL); + + /* Share the remaining work with replan code path */ + exec_save_simple_expr(expr, cplan); + + /* Release our plan refcount */ + ReleaseCachedPlan(cplan, true); +} + +/* + * exec_is_simple_query - precheck a query tree to see if it might be simple + * + * Check the analyzed-and-rewritten form of a query to see if we will be + * able to treat it as a simple expression. It is caller's responsibility + * that the CachedPlanSource be up-to-date. + */ +static bool +exec_is_simple_query(PLpgSQL_expr *expr) +{ + List *plansources; + CachedPlanSource *plansource; + Query *query; + + /* + * We can only test queries that resulted in exactly one CachedPlanSource. */ plansources = SPI_plan_get_plan_sources(expr->plan); if (list_length(plansources) != 1) - return; + return false; plansource = (CachedPlanSource *) linitial(plansources); /* * 1. There must be one single querytree. */ if (list_length(plansource->query_list) != 1) - return; + return false; query = (Query *) linitial(plansource->query_list); /* - * 2. It must be a plain SELECT query without any input tables + * 2. It must be a plain SELECT query without any input tables. */ if (!IsA(query, Query)) - return; + return false; if (query->commandType != CMD_SELECT) - return; + return false; if (query->rtable != NIL) - return; + return false; /* * 3. Can't have any subplans, aggregates, qual clauses either. (These @@ -8088,33 +8134,18 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) query->limitOffset || query->limitCount || query->setOperations) - return; + return false; /* - * 4. The query must have a single attribute as result + * 4. The query must have a single attribute as result. */ if (list_length(query->targetList) != 1) - return; + return false; /* * OK, we can treat it as a simple plan. - * - * Get the generic plan for the query. If replanning is needed, do that - * work in the eval_mcontext. (Note that replanning could throw an error, - * in which case the expr is left marked "not simple", which is fine.) */ - oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate)); - cplan = SPI_plan_get_cached_plan(expr->plan); - MemoryContextSwitchTo(oldcontext); - - /* Can't fail, because we checked for a single CachedPlanSource above */ - Assert(cplan != NULL); - - /* Share the remaining work with replan code path */ - exec_save_simple_expr(expr, cplan); - - /* Release our plan refcount */ - ReleaseCachedPlan(cplan, true); + return true; } /* diff --git a/src/pl/plpgsql/src/sql/plpgsql_simple.sql b/src/pl/plpgsql/src/sql/plpgsql_simple.sql new file mode 100644 index 0000000000..143bf09dce --- /dev/null +++ b/src/pl/plpgsql/src/sql/plpgsql_simple.sql @@ -0,0 +1,104 @@ +-- +-- Tests for plpgsql's handling of "simple" expressions +-- + +-- Check that changes to an inline-able function are handled correctly +create function simplesql(int) returns int language sql +as 'select $1'; + +create function simplecaller() returns int language plpgsql +as $$ +declare + sum int := 0; +begin + for n in 1..10 loop + sum := sum + simplesql(n); + if n = 5 then + create or replace function simplesql(int) returns int language sql + as 'select $1 + 100'; + end if; + end loop; + return sum; +end$$; + +select simplecaller(); + + +-- Check that changes in search path are dealt with correctly +create schema simple1; + +create function simple1.simpletarget(int) returns int language plpgsql +as $$begin return $1; end$$; + +create function simpletarget(int) returns int language plpgsql +as $$begin return $1 + 100; end$$; + +create or replace function simplecaller() returns int language plpgsql +as $$ +declare + sum int := 0; +begin + for n in 1..10 loop + sum := sum + simpletarget(n); + if n = 5 then + set local search_path = 'simple1'; + end if; + end loop; + return sum; +end$$; + +select simplecaller(); + +-- try it with non-volatile functions, too +alter function simple1.simpletarget(int) immutable; +alter function simpletarget(int) immutable; + +select simplecaller(); + +-- make sure flushing local caches changes nothing +\c - + +select simplecaller(); + + +-- Check case where first attempt to determine if it's simple fails + +create function simplesql() returns int language sql +as $$select 1 / 0$$; + +create or replace function simplecaller() returns int language plpgsql +as $$ +declare x int; +begin + select simplesql() into x; + return x; +end$$; + +select simplecaller(); -- division by zero occurs during simple-expr check + +create or replace function simplesql() returns int language sql +as $$select 2 + 2$$; + +select simplecaller(); + + +-- Check case where called function changes from non-SRF to SRF (bug #18497) + +create or replace function simplecaller() returns int language plpgsql +as $$ +declare x int; +begin + x := simplesql(); + return x; +end$$; + +select simplecaller(); + +drop function simplesql(); + +create function simplesql() returns setof int language sql +as $$select 22 + 22$$; + +select simplecaller(); + +select simplecaller();