git.postgresql.org Git - postgresql.git/commitdiff

git projects / postgresql.git / commitdiff

Fix error cleanup failure caused by 8.4 changes in plpgsql to try to avoid

2009年7月18日 19:15:42 +0000 (19:15 +0000)

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 patch | blob | blame | history

src/backend/executor/nodeBitmapIndexscan.c patch | blob | blame | history

src/backend/executor/nodeIndexscan.c patch | blob | blame | history

src/include/executor/executor.h patch | blob | blame | history

src/pl/plpgsql/src/pl_exec.c patch | blob | blame | history

diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c

index 360ee408586df181bdf8906a30efa484191ac8c1..9411718667f5e2ba9abd71e8354c9c050baa3e69 100644 (file)

--- 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 1ef6c988ce42dd9430333bb8bd53259b99d3602c..3ffa18bb6003939a1a1cd1ffb91d641bf4fc1b91 100644 (file)

--- 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 d0f1899fca325adaa52779a6d0bf70bd23a46982..48290d14cb5a28cc92a8d013ee19efca7e48a943 100644 (file)

--- 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 d4dc3672c52bc6618f4c8f1e476cc827547cb271..ba625e8ed8c4d9cce4c08de33d7869aa678cabba 100644 (file)

--- a/src/include/executor/executor.h

+++ b/src/include/executor/executor.h

@@ -7,7 +7,7 @@

- * $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 2a3682f1d9dfdb33b91336271f5d2321a5e12c92..f9344a395d98d522e635d40405188d4bb43996da 100644 (file)

--- 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;

This is the main PostgreSQL git repository.

RSS Atom