From 011eae60effb07af476255be754ee491ce37164d Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 18 Jul 2009 19:15:42 +0000 Subject: [PATCH] Fix error cleanup failure caused by 8.4 changes in plpgsql to try to avoid memory leakage in error recovery. We were calling FreeExprContext, and therefore invoking ExprContextCallback callbacks, in both normal and error exits from subtransactions. However this isn't very safe, as shown in recent trouble report from Frank van Vugt, in which releasing a tupledesc refcount failed. It's also unnecessary, since the resources that callbacks might wish to release should be cleaned up by other error recovery mechanisms (ie the resource owners). We only really want FreeExprContext to release memory attached to the exprcontext in the error-exit case. So, add a bool parameter to FreeExprContext to tell it not to call the callbacks. A more general solution would be to pass the isCommit bool parameter on to the callbacks, so they could do only safe things during error exit. But that would make the patch significantly more invasive and possibly break third-party code that registers ExprContextCallback callbacks. We might want to do that later in HEAD, but for now I'll just do what seems reasonable to back-patch. --- src/backend/executor/execUtils.c | 26 ++++++++++++++++++-------- src/backend/executor/nodeBitmapIndexscan.c | 4 ++-- src/backend/executor/nodeIndexscan.c | 4 ++-- src/include/executor/executor.h | 4 ++-- src/pl/plpgsql/src/pl_exec.c | 7 ++++--- 5 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c index 360ee40858..9411718667 100644 --- a/src/backend/executor/execUtils.c +++ b/src/backend/executor/execUtils.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/executor/execUtils.c,v 1.159 2009/06/11 14:48:57 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/executor/execUtils.c,v 1.160 2009/07/18 19:15:41 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -68,7 +68,7 @@ int NIndexTupleProcessed; static bool get_last_attnums(Node *node, ProjectionInfo *projInfo); -static void ShutdownExprContext(ExprContext *econtext); +static void ShutdownExprContext(ExprContext *econtext, bool isCommit); /* ---------------------------------------------------------------- @@ -257,7 +257,8 @@ FreeExecutorState(EState *estate) * XXX: seems there ought to be a faster way to implement this than * repeated list_delete(), no? */ - FreeExprContext((ExprContext *) linitial(estate->es_exprcontexts)); + FreeExprContext((ExprContext *) linitial(estate->es_exprcontexts), + true); /* FreeExprContext removed the list link for us */ } @@ -408,16 +409,21 @@ CreateStandaloneExprContext(void) * Since we free the temporary context used for expression evaluation, * any previously computed pass-by-reference expression result will go away! * + * If isCommit is false, we are being called in error cleanup, and should + * not call callbacks but only release memory. (It might be better to call + * the callbacks and pass the isCommit flag to them, but that would require + * more invasive code changes than currently seems justified.) + * * Note we make no assumption about the caller's memory context. * ---------------- */ void -FreeExprContext(ExprContext *econtext) +FreeExprContext(ExprContext *econtext, bool isCommit) { EState *estate; /* Call any registered callbacks */ - ShutdownExprContext(econtext); + ShutdownExprContext(econtext, isCommit); /* And clean up the memory used */ MemoryContextDelete(econtext->ecxt_per_tuple_memory); /* Unlink self from owning EState, if any */ @@ -442,7 +448,7 @@ void ReScanExprContext(ExprContext *econtext) { /* Call any registered callbacks */ - ShutdownExprContext(econtext); + ShutdownExprContext(econtext, true); /* And clean up the memory used */ MemoryContextReset(econtext->ecxt_per_tuple_memory); } @@ -1222,9 +1228,12 @@ UnregisterExprContextCallback(ExprContext *econtext, * * The callback list is emptied (important in case this is only a rescan * reset, and not deletion of the ExprContext). + * + * If isCommit is false, just clean the callback list but don't call 'em. + * (See comment for FreeExprContext.) */ static void -ShutdownExprContext(ExprContext *econtext) +ShutdownExprContext(ExprContext *econtext, bool isCommit) { ExprContext_CB *ecxt_callback; MemoryContext oldcontext; @@ -1245,7 +1254,8 @@ ShutdownExprContext(ExprContext *econtext) while ((ecxt_callback = econtext->ecxt_callbacks) != NULL) { econtext->ecxt_callbacks = ecxt_callback->next; - (*ecxt_callback->function) (ecxt_callback->arg); + if (isCommit) + (*ecxt_callback->function) (ecxt_callback->arg); pfree(ecxt_callback); } diff --git a/src/backend/executor/nodeBitmapIndexscan.c b/src/backend/executor/nodeBitmapIndexscan.c index 1ef6c988ce..3ffa18bb60 100644 --- a/src/backend/executor/nodeBitmapIndexscan.c +++ b/src/backend/executor/nodeBitmapIndexscan.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/executor/nodeBitmapIndexscan.c,v 1.30 2009/06/11 14:48:57 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/executor/nodeBitmapIndexscan.c,v 1.31 2009/07/18 19:15:41 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -182,7 +182,7 @@ ExecEndBitmapIndexScan(BitmapIndexScanState *node) */ #ifdef NOT_USED if (node->biss_RuntimeContext) - FreeExprContext(node->biss_RuntimeContext); + FreeExprContext(node->biss_RuntimeContext, true); #endif /* diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c index d0f1899fca..48290d14cb 100644 --- a/src/backend/executor/nodeIndexscan.c +++ b/src/backend/executor/nodeIndexscan.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/executor/nodeIndexscan.c,v 1.132 2009/06/11 14:48:57 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/executor/nodeIndexscan.c,v 1.133 2009/07/18 19:15:41 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -423,7 +423,7 @@ ExecEndIndexScan(IndexScanState *node) #ifdef NOT_USED ExecFreeExprContext(&node->ss.ps); if (node->iss_RuntimeContext) - FreeExprContext(node->iss_RuntimeContext); + FreeExprContext(node->iss_RuntimeContext, true); #endif /* diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index d4dc3672c5..ba625e8ed8 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/executor/executor.h,v 1.155 2009/06/11 14:49:11 momjian Exp $ + * $PostgreSQL: pgsql/src/include/executor/executor.h,v 1.156 2009/07/18 19:15:42 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -255,7 +255,7 @@ extern EState *CreateExecutorState(void); extern void FreeExecutorState(EState *estate); extern ExprContext *CreateExprContext(EState *estate); extern ExprContext *CreateStandaloneExprContext(void); -extern void FreeExprContext(ExprContext *econtext); +extern void FreeExprContext(ExprContext *econtext, bool isCommit); extern void ReScanExprContext(ExprContext *econtext); #define ResetExprContext(econtext) \ diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 2a3682f1d9..f9344a395d 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.244 2009/06/17 13:46:12 petere Exp $ + * $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.245 2009/07/18 19:15:42 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -5237,7 +5237,7 @@ plpgsql_destroy_econtext(PLpgSQL_execstate *estate) pfree(simple_econtext_stack); simple_econtext_stack = next; - FreeExprContext(estate->eval_econtext); + FreeExprContext(estate->eval_econtext, true); estate->eval_econtext = NULL; } @@ -5292,7 +5292,8 @@ plpgsql_subxact_cb(SubXactEvent event, SubTransactionId mySubid, { SimpleEcontextStackEntry *next; - FreeExprContext(simple_econtext_stack->stack_econtext); + FreeExprContext(simple_econtext_stack->stack_econtext, + (event == SUBXACT_EVENT_COMMIT_SUB)); next = simple_econtext_stack->next; pfree(simple_econtext_stack); simple_econtext_stack = next; -- 2.11.0