From c7b849a89645212121da480091f87d11fac82495 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 15 Nov 2013 13:52:03 -0500 Subject: [PATCH] Prevent leakage of cached plans and execution trees in plpgsql DO blocks. plpgsql likes to cache query plans and simple-expression execution state trees across calls. This is a considerable win for multiple executions of the same function. However, it's useless for DO blocks, since by definition those are executed only once and discarded. Nonetheless, we were allowing a DO block's expression execution trees to survive until end of transaction, resulting in a significant intra-transaction memory leak, as reported by Yeb Havinga. Worse, if the DO block exited with an error, the compiled form of the block's code was leaked till end of session --- along with subsidiary plancache entries. To fix, make DO blocks keep their expression execution trees in a private EState that's deleted at exit from the block, and add a PG_TRY block to plpgsql_inline_handler to make sure that memory cleanup happens even on error exits. Also add a regression test covering error handling in a DO block, because my first try at this broke that. (The test is not meant to prove that we don't leak memory anymore, though it could be used for that with a much larger loop count.) Ideally we'd back-patch this into all versions supporting DO blocks; but the patch needs to add a field to struct PLpgSQL_execstate, and that would break ABI compatibility for third-party plugins such as the plpgsql debugger. Given the small number of complaints so far, fixing this in HEAD only seems like an acceptable choice. --- src/pl/plpgsql/src/pl_exec.c | 67 +++++++++++++++++++-------- src/pl/plpgsql/src/pl_handler.c | 49 +++++++++++++++++++- src/pl/plpgsql/src/plpgsql.h | 7 ++- src/test/regress/expected/plpgsql.out | 29 ++++++++++++ src/test/regress/sql/plpgsql.sql | 20 ++++++++ 5 files changed, 150 insertions(+), 22 deletions(-) diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index bc31fe9085..f5f1892e69 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -66,6 +66,15 @@ typedef struct * so that we don't have to re-prepare simple expressions on each trip through * a function. (We assume the case to optimize is many repetitions of a * function within a transaction.) + * + * However, there's no value in trying to amortize simple expression setup + * across multiple executions of a DO block (inline code block), since there + * can never be any. If we use the shared EState for a DO block, the expr + * state trees are effectively leaked till end of transaction, and that can + * add up if the user keeps on submitting DO blocks. Therefore, each DO block + * has its own simple-expression EState, which is cleaned up at exit from + * plpgsql_inline_handler(). DO blocks still use the simple_econtext_stack, + * though, so that subxact abort cleanup does the right thing. */ typedef struct SimpleEcontextStackEntry { @@ -74,7 +83,7 @@ typedef struct SimpleEcontextStackEntry struct SimpleEcontextStackEntry *next; /* next stack entry up */ } SimpleEcontextStackEntry; -static EState *simple_eval_estate = NULL; +static EState *shared_simple_eval_estate = NULL; static SimpleEcontextStackEntry *simple_econtext_stack = NULL; /************************************************************ @@ -136,7 +145,8 @@ static int exec_stmt_dynfors(PLpgSQL_execstate *estate, static void plpgsql_estate_setup(PLpgSQL_execstate *estate, PLpgSQL_function *func, - ReturnSetInfo *rsi); + ReturnSetInfo *rsi, + EState *simple_eval_estate); static void exec_eval_cleanup(PLpgSQL_execstate *estate); static void exec_prepare_plan(PLpgSQL_execstate *estate, @@ -230,10 +240,17 @@ static char *format_preparedparamsdata(PLpgSQL_execstate *estate, /* ---------- * plpgsql_exec_function Called by the call handler for * function execution. + * + * This is also used to execute inline code blocks (DO blocks). The only + * difference that this code is aware of is that for a DO block, we want + * to use a private simple_eval_estate, which is created and passed in by + * the caller. For regular functions, pass NULL, which implies using + * shared_simple_eval_estate. * ---------- */ Datum -plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo fcinfo) +plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo fcinfo, + EState *simple_eval_estate) { PLpgSQL_execstate estate; ErrorContextCallback plerrcontext; @@ -243,7 +260,8 @@ plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo fcinfo) /* * Setup the execution state */ - plpgsql_estate_setup(&estate, func, (ReturnSetInfo *) fcinfo->resultinfo); + plpgsql_estate_setup(&estate, func, (ReturnSetInfo *) fcinfo->resultinfo, + simple_eval_estate); /* * Setup error traceback support for ereport() @@ -503,7 +521,7 @@ plpgsql_exec_trigger(PLpgSQL_function *func, /* * Setup the execution state */ - plpgsql_estate_setup(&estate, func, NULL); + plpgsql_estate_setup(&estate, func, NULL, NULL); /* * Setup error traceback support for ereport() @@ -782,7 +800,7 @@ plpgsql_exec_event_trigger(PLpgSQL_function *func, EventTriggerData *trigdata) /* * Setup the execution state */ - plpgsql_estate_setup(&estate, func, NULL); + plpgsql_estate_setup(&estate, func, NULL, NULL); /* * Setup error traceback support for ereport() @@ -3087,7 +3105,8 @@ exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt) static void plpgsql_estate_setup(PLpgSQL_execstate *estate, PLpgSQL_function *func, - ReturnSetInfo *rsi) + ReturnSetInfo *rsi, + EState *simple_eval_estate) { /* this link will be restored at exit from plpgsql_call_handler */ func->cur_estate = estate; @@ -3126,6 +3145,12 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate, estate->datums = palloc(sizeof(PLpgSQL_datum *) * estate->ndatums); /* caller is expected to fill the datums array */ + /* set up for use of appropriate simple-expression EState */ + if (simple_eval_estate) + estate->simple_eval_estate = simple_eval_estate; + else + estate->simple_eval_estate = shared_simple_eval_estate; + estate->eval_tuptable = NULL; estate->eval_processed = 0; estate->eval_lastoid = InvalidOid; @@ -5148,7 +5173,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, */ if (expr->expr_simple_lxid != curlxid) { - oldcontext = MemoryContextSwitchTo(simple_eval_estate->es_query_cxt); + oldcontext = MemoryContextSwitchTo(estate->simple_eval_estate->es_query_cxt); expr->expr_simple_state = ExecInitExpr(expr->expr_simple_expr, NULL); expr->expr_simple_in_use = false; expr->expr_simple_lxid = curlxid; @@ -6190,8 +6215,8 @@ exec_set_found(PLpgSQL_execstate *estate, bool state) /* * plpgsql_create_econtext --- create an eval_econtext for the current function * - * We may need to create a new simple_eval_estate too, if there's not one - * already for the current transaction. The EState will be cleaned up at + * We may need to create a new shared_simple_eval_estate too, if there's not + * one already for the current transaction. The EState will be cleaned up at * transaction end. */ static void @@ -6203,20 +6228,25 @@ plpgsql_create_econtext(PLpgSQL_execstate *estate) * Create an EState for evaluation of simple expressions, if there's not * one already in the current transaction. The EState is made a child of * TopTransactionContext so it will have the right lifespan. + * + * Note that this path is never taken when executing a DO block; the + * required EState was already made by plpgsql_inline_handler. */ - if (simple_eval_estate == NULL) + if (estate->simple_eval_estate == NULL) { MemoryContext oldcontext; + Assert(shared_simple_eval_estate == NULL); oldcontext = MemoryContextSwitchTo(TopTransactionContext); - simple_eval_estate = CreateExecutorState(); + shared_simple_eval_estate = CreateExecutorState(); + estate->simple_eval_estate = shared_simple_eval_estate; MemoryContextSwitchTo(oldcontext); } /* * Create a child econtext for the current function. */ - estate->eval_econtext = CreateExprContext(simple_eval_estate); + estate->eval_econtext = CreateExprContext(estate->simple_eval_estate); /* * Make a stack entry so we can clean up the econtext at subxact end. @@ -6275,14 +6305,14 @@ plpgsql_xact_cb(XactEvent event, void *arg) /* Shouldn't be any econtext stack entries left at commit */ Assert(simple_econtext_stack == NULL); - if (simple_eval_estate) - FreeExecutorState(simple_eval_estate); - simple_eval_estate = NULL; + if (shared_simple_eval_estate) + FreeExecutorState(shared_simple_eval_estate); + shared_simple_eval_estate = NULL; } else if (event == XACT_EVENT_ABORT) { simple_econtext_stack = NULL; - simple_eval_estate = NULL; + shared_simple_eval_estate = NULL; } } @@ -6291,8 +6321,7 @@ plpgsql_xact_cb(XactEvent event, void *arg) * * Make sure any simple-expression econtexts created in the current * subtransaction get cleaned up. We have to do this explicitly because - * no other code knows which child econtexts of simple_eval_estate belong - * to which level of subxact. + * no other code knows which econtexts belong to which level of subxact. */ void plpgsql_subxact_cb(SubXactEvent event, SubTransactionId mySubid, diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c index 912422cd2e..5bfe3c3d8e 100644 --- a/src/pl/plpgsql/src/pl_handler.c +++ b/src/pl/plpgsql/src/pl_handler.c @@ -136,7 +136,7 @@ plpgsql_call_handler(PG_FUNCTION_ARGS) retval = (Datum) 0; } else - retval = plpgsql_exec_function(func, fcinfo); + retval = plpgsql_exec_function(func, fcinfo, NULL); } PG_CATCH(); { @@ -175,6 +175,7 @@ plpgsql_inline_handler(PG_FUNCTION_ARGS) PLpgSQL_function *func; FunctionCallInfoData fake_fcinfo; FmgrInfo flinfo; + EState *simple_eval_estate; Datum retval; int rc; @@ -203,7 +204,51 @@ plpgsql_inline_handler(PG_FUNCTION_ARGS) flinfo.fn_oid = InvalidOid; flinfo.fn_mcxt = CurrentMemoryContext; - retval = plpgsql_exec_function(func, &fake_fcinfo); + /* Create a private EState for simple-expression execution */ + simple_eval_estate = CreateExecutorState(); + + /* And run the function */ + PG_TRY(); + { + retval = plpgsql_exec_function(func, &fake_fcinfo, simple_eval_estate); + } + PG_CATCH(); + { + /* + * We need to clean up what would otherwise be long-lived resources + * accumulated by the failed DO block, principally cached plans for + * statements (which can be flushed with plpgsql_free_function_memory) + * and execution trees for simple expressions, which are in the + * private EState. + * + * Before releasing the private EState, we must clean up any + * simple_econtext_stack entries pointing into it, which we can do by + * invoking the subxact callback. (It will be called again later if + * some outer control level does a subtransaction abort, but no harm + * is done.) We cheat a bit knowing that plpgsql_subxact_cb does not + * pay attention to its parentSubid argument. + */ + plpgsql_subxact_cb(SUBXACT_EVENT_ABORT_SUB, + GetCurrentSubTransactionId(), + 0, NULL); + + /* Clean up the private EState */ + FreeExecutorState(simple_eval_estate); + + /* Function should now have no remaining use-counts ... */ + func->use_count--; + Assert(func->use_count == 0); + + /* ... so we can free subsidiary storage */ + plpgsql_free_function_memory(func); + + /* And propagate the error */ + PG_RE_THROW(); + } + PG_END_TRY(); + + /* Clean up the private EState */ + FreeExecutorState(simple_eval_estate); /* Function should now have no remaining use-counts ... */ func->use_count--; diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 9cb4f53334..3afcdf324c 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -773,10 +773,14 @@ typedef struct PLpgSQL_execstate ResourceOwner tuple_store_owner; ReturnSetInfo *rsi; + /* the datums representing the function's local variables */ int found_varno; int ndatums; PLpgSQL_datum **datums; + /* EState to use for "simple" expression evaluation */ + EState *simple_eval_estate; + /* temporary state for results from evaluation of query or expr */ SPITupleTable *eval_tuptable; uint32 eval_processed; @@ -943,7 +947,8 @@ extern Datum plpgsql_validator(PG_FUNCTION_ARGS); * ---------- */ extern Datum plpgsql_exec_function(PLpgSQL_function *func, - FunctionCallInfo fcinfo); + FunctionCallInfo fcinfo, + EState *simple_eval_estate); extern HeapTuple plpgsql_exec_trigger(PLpgSQL_function *func, TriggerData *trigdata); extern void plpgsql_exec_event_trigger(PLpgSQL_function *func, diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out index 22c16c4b3e..0405ef47dd 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -4651,6 +4651,35 @@ LINE 1: SELECT rtrim(roomno) AS roomno, foo FROM Room ORDER BY roomn... ^ QUERY: SELECT rtrim(roomno) AS roomno, foo FROM Room ORDER BY roomno CONTEXT: PL/pgSQL function inline_code_block line 4 at FOR over SELECT rows +-- Check handling of errors thrown from/into anonymous code blocks. +do $outer$ +begin + for i in 1..10 loop + begin + execute $ex$ + do $$ + declare x int = 0; + begin + x := 1 / x; + end; + $$; + $ex$; + exception when division_by_zero then + raise notice 'caught division by zero'; + end; + end loop; +end; +$outer$; +NOTICE: caught division by zero +NOTICE: caught division by zero +NOTICE: caught division by zero +NOTICE: caught division by zero +NOTICE: caught division by zero +NOTICE: caught division by zero +NOTICE: caught division by zero +NOTICE: caught division by zero +NOTICE: caught division by zero +NOTICE: caught division by zero -- Check variable scoping -- a var is not available in its own or prior -- default expressions. create function scope_test() returns int as $$ diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql index a685fa246e..d01011a85a 100644 --- a/src/test/regress/sql/plpgsql.sql +++ b/src/test/regress/sql/plpgsql.sql @@ -3751,6 +3751,26 @@ BEGIN END LOOP; END$$; +-- Check handling of errors thrown from/into anonymous code blocks. +do $outer$ +begin + for i in 1..10 loop + begin + execute $ex$ + do $$ + declare x int = 0; + begin + x := 1 / x; + end; + $$; + $ex$; + exception when division_by_zero then + raise notice 'caught division by zero'; + end; + end loop; +end; +$outer$; + -- Check variable scoping -- a var is not available in its own or prior -- default expressions. -- 2.40.0