Skip to content

Commit 381d6a0

Browse files
tglsfdctglsfdc
committed
Fix plpgsql's handling of "simple" expression evaluation.
In general, expression execution state trees aren't re-entrantly usable, since functions can store private state information in them. For efficiency reasons, plpgsql tries to cache and reuse state trees for "simple" expressions. It can get away with that most of the time, but it can fail if the state tree is dirty from a previous failed execution (as in an example from Alvaro) or is being used recursively (as noted by me). Fix by tracking whether a state tree is in use, and falling back to the "non-simple" code path if so. This results in a pretty considerable speed hit when the non-simple path is taken, but the available alternatives seem even more unpleasant because they add overhead in the simple path. Per idea from Heikki. Back-patch to all supported branches.
1 parent 4dd158e commit 381d6a0

File tree

4 files changed

+121
-4
lines changed

4 files changed

+121
-4
lines changed

‎src/pl/plpgsql/src/pl_exec.c

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4463,7 +4463,18 @@ exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_stmt_forq *stmt,
44634463
* a Datum by directly calling ExecEvalExpr().
44644464
*
44654465
* If successful, store results into *result, *isNull, *rettype and return
4466-
* TRUE. If the expression is not simple (any more), return FALSE.
4466+
* TRUE. If the expression cannot be handled by simple evaluation,
4467+
* return FALSE.
4468+
*
4469+
* Because we only store one execution tree for a simple expression, we
4470+
* can't handle recursion cases. So, if we see the tree is already busy
4471+
* with an evaluation in the current xact, we just return FALSE and let the
4472+
* caller run the expression the hard way. (Other alternatives such as
4473+
* creating a new tree for a recursive call either introduce memory leaks,
4474+
* or add enough bookkeeping to be doubtful wins anyway.) Another case that
4475+
* is covered by the expr_simple_in_use test is where a previous execution
4476+
* of the tree was aborted by an error: the tree may contain bogus state
4477+
* so we dare not re-use it.
44674478
*
44684479
* It is possible though unlikely for a simple expression to become non-simple
44694480
* (consider for example redefining a trivial view). We must handle that for
@@ -4499,6 +4510,12 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
44994510
if (expr->expr_simple_expr == NULL)
45004511
return false;
45014512

4513+
/*
4514+
* If expression is in use in current xact, don't touch it.
4515+
*/
4516+
if (expr->expr_simple_in_use && expr->expr_simple_lxid == curlxid)
4517+
return false;
4518+
45024519
/*
45034520
* Revalidate cached plan, so that we will notice if it became stale. (We
45044521
* also need to hold a refcount while using the plan.) Note that even if
@@ -4534,6 +4551,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
45344551
{
45354552
expr->expr_simple_state = ExecPrepareExpr(expr->expr_simple_expr,
45364553
simple_eval_estate);
4554+
expr->expr_simple_in_use = false;
45374555
expr->expr_simple_lxid = curlxid;
45384556
}
45394557

@@ -4568,6 +4586,11 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
45684586
paramLI = setup_param_list(estate, expr);
45694587
econtext->ecxt_param_list_info = paramLI;
45704588

4589+
/*
4590+
* Mark expression as busy for the duration of the ExecEvalExpr call.
4591+
*/
4592+
expr->expr_simple_in_use = true;
4593+
45714594
/*
45724595
* Finally we can call the executor to evaluate the expression
45734596
*/
@@ -4577,6 +4600,8 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
45774600
NULL);
45784601

45794602
/* Assorted cleanup */
4603+
expr->expr_simple_in_use = false;
4604+
45804605
estate->cur_expr = save_cur_expr;
45814606

45824607
if (!estate->readonly_func)
@@ -5308,6 +5333,7 @@ exec_simple_check_plan(PLpgSQL_expr *expr)
53085333
*/
53095334
expr->expr_simple_expr = tle->expr;
53105335
expr->expr_simple_state = NULL;
5336+
expr->expr_simple_in_use = false;
53115337
expr->expr_simple_lxid = InvalidLocalTransactionId;
53125338
/* Also stash away the expression result type */
53135339
expr->expr_simple_type = exprType((Node *) tle->expr);

‎src/pl/plpgsql/src/plpgsql.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -215,10 +215,12 @@ typedef struct PLpgSQL_expr
215215

216216
/*
217217
* if expr is simple AND prepared in current transaction,
218-
* expr_simple_state is valid. Test validity by seeing if expr_simple_lxid
219-
* matches current LXID.
218+
* expr_simple_state and expr_simple_in_use are valid. Test validity by
219+
* seeing if expr_simple_lxid matches current LXID. (If not,
220+
* expr_simple_state probably points at garbage!)
220221
*/
221-
ExprState *expr_simple_state;
222+
ExprState *expr_simple_state; /* eval tree for expr_simple_expr */
223+
bool expr_simple_in_use; /* true if eval tree is active */
222224
LocalTransactionId expr_simple_lxid;
223225
} PLpgSQL_expr;
224226

‎src/test/regress/expected/plpgsql.out

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3959,6 +3959,53 @@ SELECT nonsimple_expr_test();
39593959
(1 row)
39603960

39613961
DROP FUNCTION nonsimple_expr_test();
3962+
--
3963+
-- Test cases involving recursion and error recovery in simple expressions
3964+
-- (bugs in all versions before October 2010). The problems are most
3965+
-- easily exposed by mutual recursion between plpgsql and sql functions.
3966+
--
3967+
create function recurse(float8) returns float8 as
3968+
$$
3969+
begin
3970+
if ($1 < 10) then
3971+
return sql_recurse($1 + 1);
3972+
else
3973+
return $1;
3974+
end if;
3975+
end;
3976+
$$ language plpgsql;
3977+
-- "limit" is to prevent this from being inlined
3978+
create function sql_recurse(float8) returns float8 as
3979+
$$ select recurse($1) limit 1; $$ language sql;
3980+
select recurse(0);
3981+
recurse
3982+
---------
3983+
10
3984+
(1 row)
3985+
3986+
create function error1(text) returns text language sql as
3987+
$$ SELECT relname::text FROM pg_class c WHERE c.oid = $1::regclass $$;
3988+
create function error2(p_name_table text) returns text language plpgsql as $$
3989+
begin
3990+
return error1(p_name_table);
3991+
end$$;
3992+
BEGIN;
3993+
create table public.stuffs (stuff text);
3994+
SAVEPOINT a;
3995+
select error2('nonexistent.stuffs');
3996+
ERROR: schema "nonexistent" does not exist
3997+
CONTEXT: SQL function "error1" statement 1
3998+
PL/pgSQL function "error2" line 2 at RETURN
3999+
ROLLBACK TO a;
4000+
select error2('public.stuffs');
4001+
error2
4002+
--------
4003+
stuffs
4004+
(1 row)
4005+
4006+
rollback;
4007+
drop function error2(p_name_table text);
4008+
drop function error1(text);
39624009
-- Test handling of string literals.
39634010
set standard_conforming_strings = off;
39644011
create or replace function strtest() returns text as $$

‎src/test/regress/sql/plpgsql.sql

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3165,6 +3165,48 @@ SELECT nonsimple_expr_test();
31653165

31663166
DROP FUNCTION nonsimple_expr_test();
31673167

3168+
--
3169+
-- Test cases involving recursion and error recovery in simple expressions
3170+
-- (bugs in all versions before October 2010). The problems are most
3171+
-- easily exposed by mutual recursion between plpgsql and sql functions.
3172+
--
3173+
3174+
create function recurse(float8) returns float8 as
3175+
$$
3176+
begin
3177+
if ($1 < 10) then
3178+
return sql_recurse($1 + 1);
3179+
else
3180+
return $1;
3181+
end if;
3182+
end;
3183+
$$ language plpgsql;
3184+
3185+
-- "limit" is to prevent this from being inlined
3186+
create function sql_recurse(float8) returns float8 as
3187+
$$ select recurse($1) limit 1; $$ language sql;
3188+
3189+
select recurse(0);
3190+
3191+
create function error1(text) returns text language sql as
3192+
$$ SELECT relname::text FROM pg_class c WHERE c.oid = $1::regclass $$;
3193+
3194+
create function error2(p_name_table text) returns text language plpgsql as $$
3195+
begin
3196+
return error1(p_name_table);
3197+
end$$;
3198+
3199+
BEGIN;
3200+
create table public.stuffs (stuff text);
3201+
SAVEPOINT a;
3202+
select error2('nonexistent.stuffs');
3203+
ROLLBACK TO a;
3204+
select error2('public.stuffs');
3205+
rollback;
3206+
3207+
drop function error2(p_name_table text);
3208+
drop function error1(text);
3209+
31683210
-- Test handling of string literals.
31693211

31703212
set standard_conforming_strings = off;

0 commit comments

Comments
 (0)