diff options
| -rw-r--r-- | src/pl/plpgsql/src/pl_exec.c | 28 | ||||
| -rw-r--r-- | src/pl/plpgsql/src/plpgsql.h | 8 | ||||
| -rw-r--r-- | src/test/regress/expected/plpgsql.out | 47 | ||||
| -rw-r--r-- | src/test/regress/sql/plpgsql.sql | 42 |
4 files changed, 121 insertions, 4 deletions
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index a4a327a9062..16277ceaa0f 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -4463,7 +4463,18 @@ loop_exit: * a Datum by directly calling ExecEvalExpr(). * * If successful, store results into *result, *isNull, *rettype and return - * TRUE. If the expression is not simple (any more), return FALSE. + * TRUE. If the expression cannot be handled by simple evaluation, + * return FALSE. + * + * Because we only store one execution tree for a simple expression, we + * can't handle recursion cases. So, if we see the tree is already busy + * with an evaluation in the current xact, we just return FALSE and let the + * caller run the expression the hard way. (Other alternatives such as + * creating a new tree for a recursive call either introduce memory leaks, + * or add enough bookkeeping to be doubtful wins anyway.) Another case that + * is covered by the expr_simple_in_use test is where a previous execution + * of the tree was aborted by an error: the tree may contain bogus state + * so we dare not re-use it. * * It is possible though unlikely for a simple expression to become non-simple * (consider for example redefining a trivial view). We must handle that for @@ -4500,6 +4511,12 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, return false; /* + * If expression is in use in current xact, don't touch it. + */ + if (expr->expr_simple_in_use && expr->expr_simple_lxid == curlxid) + return false; + + /* * Revalidate cached plan, so that we will notice if it became stale. (We * also need to hold a refcount while using the plan.) Note that even if * replanning occurs, the length of plancache_list can't change, since it @@ -4534,6 +4551,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, { expr->expr_simple_state = ExecPrepareExpr(expr->expr_simple_expr, simple_eval_estate); + expr->expr_simple_in_use = false; expr->expr_simple_lxid = curlxid; } @@ -4569,6 +4587,11 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, econtext->ecxt_param_list_info = paramLI; /* + * Mark expression as busy for the duration of the ExecEvalExpr call. + */ + expr->expr_simple_in_use = true; + + /* * Finally we can call the executor to evaluate the expression */ *result = ExecEvalExpr(expr->expr_simple_state, @@ -4577,6 +4600,8 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, NULL); /* Assorted cleanup */ + expr->expr_simple_in_use = false; + estate->cur_expr = save_cur_expr; if (!estate->readonly_func) @@ -5308,6 +5333,7 @@ exec_simple_check_plan(PLpgSQL_expr *expr) */ expr->expr_simple_expr = tle->expr; expr->expr_simple_state = NULL; + expr->expr_simple_in_use = false; expr->expr_simple_lxid = InvalidLocalTransactionId; /* Also stash away the expression result type */ expr->expr_simple_type = exprType((Node *) tle->expr); diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 16e073c2109..0d30e228fbf 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -215,10 +215,12 @@ typedef struct PLpgSQL_expr /* * if expr is simple AND prepared in current transaction, - * expr_simple_state is valid. Test validity by seeing if expr_simple_lxid - * matches current LXID. + * expr_simple_state and expr_simple_in_use are valid. Test validity by + * seeing if expr_simple_lxid matches current LXID. (If not, + * expr_simple_state probably points at garbage!) */ - ExprState *expr_simple_state; + ExprState *expr_simple_state; /* eval tree for expr_simple_expr */ + bool expr_simple_in_use; /* true if eval tree is active */ LocalTransactionId expr_simple_lxid; } PLpgSQL_expr; diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out index 8d20cd90482..8e05cd0b560 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -3959,6 +3959,53 @@ SELECT nonsimple_expr_test(); (1 row) DROP FUNCTION nonsimple_expr_test(); +-- +-- Test cases involving recursion and error recovery in simple expressions +-- (bugs in all versions before October 2010). The problems are most +-- easily exposed by mutual recursion between plpgsql and sql functions. +-- +create function recurse(float8) returns float8 as +$$ +begin + if ($1 < 10) then + return sql_recurse($1 + 1); + else + return $1; + end if; +end; +$$ language plpgsql; +-- "limit" is to prevent this from being inlined +create function sql_recurse(float8) returns float8 as +$$ select recurse($1) limit 1; $$ language sql; +select recurse(0); + recurse +--------- + 10 +(1 row) + +create function error1(text) returns text language sql as +$$ SELECT relname::text FROM pg_class c WHERE c.oid = $1::regclass $$; +create function error2(p_name_table text) returns text language plpgsql as $$ +begin + return error1(p_name_table); +end$$; +BEGIN; +create table public.stuffs (stuff text); +SAVEPOINT a; +select error2('nonexistent.stuffs'); +ERROR: schema "nonexistent" does not exist +CONTEXT: SQL function "error1" statement 1 +PL/pgSQL function "error2" line 2 at RETURN +ROLLBACK TO a; +select error2('public.stuffs'); + error2 +-------- + stuffs +(1 row) + +rollback; +drop function error2(p_name_table text); +drop function error1(text); -- Test handling of string literals. set standard_conforming_strings = off; create or replace function strtest() returns text as $$ diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql index 74f95a34159..09fedd916d5 100644 --- a/src/test/regress/sql/plpgsql.sql +++ b/src/test/regress/sql/plpgsql.sql @@ -3165,6 +3165,48 @@ SELECT nonsimple_expr_test(); DROP FUNCTION nonsimple_expr_test(); +-- +-- Test cases involving recursion and error recovery in simple expressions +-- (bugs in all versions before October 2010). The problems are most +-- easily exposed by mutual recursion between plpgsql and sql functions. +-- + +create function recurse(float8) returns float8 as +$$ +begin + if ($1 < 10) then + return sql_recurse($1 + 1); + else + return $1; + end if; +end; +$$ language plpgsql; + +-- "limit" is to prevent this from being inlined +create function sql_recurse(float8) returns float8 as +$$ select recurse($1) limit 1; $$ language sql; + +select recurse(0); + +create function error1(text) returns text language sql as +$$ SELECT relname::text FROM pg_class c WHERE c.oid = $1::regclass $$; + +create function error2(p_name_table text) returns text language plpgsql as $$ +begin + return error1(p_name_table); +end$$; + +BEGIN; +create table public.stuffs (stuff text); +SAVEPOINT a; +select error2('nonexistent.stuffs'); +ROLLBACK TO a; +select error2('public.stuffs'); +rollback; + +drop function error2(p_name_table text); +drop function error1(text); + -- Test handling of string literals. set standard_conforming_strings = off; |