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.

diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 0ccd862710df07749566bb3b265416dbc4e0f074..70331890a979200cbe25e5a58ab37cdfe994aeb0 100644 (file)
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -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 1725594acab16060117c081c1780470c633379dc..4d932a331609bde644b770b8b620f0c39ce0efb5 100644 (file)
--- a/src/backend/executor/nodeBitmapIndexscan.c
+++ b/src/backend/executor/nodeBitmapIndexscan.c
@@ -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 905e1be2005e0399dd4a74c8dc1a2a817002d72a..e2ad067049ce7dd259278ba84100aab138f367f4 100644 (file)
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -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 461a2cb8c40f80c5f6aff7beb86fbb257bafc292..43395e9e6d84e250558e3d7dab6fadc3c2f6fbc6 100644 (file)
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -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 6158d122c0a761fcabd03fd89c48eb95e0aaaced..ae79bd2b8bd88bf1892c9266173cea66ad39e22e 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -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;