Skip to content

Commit 011eae6

Browse files
tglsfdctglsfdc
committed
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.
1 parent fb18055 commit 011eae6

File tree

5 files changed

+28
-17
lines changed

5 files changed

+28
-17
lines changed

‎src/backend/executor/execUtils.c

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/executor/execUtils.c,v 1.159 2009/06/11 14:48:57 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/executor/execUtils.c,v 1.160 2009/07/18 19:15:41 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -68,7 +68,7 @@ int NIndexTupleProcessed;
6868

6969

7070
static bool get_last_attnums(Node *node, ProjectionInfo *projInfo);
71-
static void ShutdownExprContext(ExprContext *econtext);
71+
static void ShutdownExprContext(ExprContext *econtext, bool isCommit);
7272

7373

7474
/* ----------------------------------------------------------------
@@ -257,7 +257,8 @@ FreeExecutorState(EState *estate)
257257
* XXX: seems there ought to be a faster way to implement this than
258258
* repeated list_delete(), no?
259259
*/
260-
FreeExprContext((ExprContext *) linitial(estate->es_exprcontexts));
260+
FreeExprContext((ExprContext *) linitial(estate->es_exprcontexts),
261+
true);
261262
/* FreeExprContext removed the list link for us */
262263
}
263264

@@ -408,16 +409,21 @@ CreateStandaloneExprContext(void)
408409
* Since we free the temporary context used for expression evaluation,
409410
* any previously computed pass-by-reference expression result will go away!
410411
*
412+
* If isCommit is false, we are being called in error cleanup, and should
413+
* not call callbacks but only release memory. (It might be better to call
414+
* the callbacks and pass the isCommit flag to them, but that would require
415+
* more invasive code changes than currently seems justified.)
416+
*
411417
* Note we make no assumption about the caller's memory context.
412418
* ----------------
413419
*/
414420
void
415-
FreeExprContext(ExprContext *econtext)
421+
FreeExprContext(ExprContext *econtext, bool isCommit)
416422
{
417423
EState *estate;
418424

419425
/* Call any registered callbacks */
420-
ShutdownExprContext(econtext);
426+
ShutdownExprContext(econtext, isCommit);
421427
/* And clean up the memory used */
422428
MemoryContextDelete(econtext->ecxt_per_tuple_memory);
423429
/* Unlink self from owning EState, if any */
@@ -442,7 +448,7 @@ void
442448
ReScanExprContext(ExprContext *econtext)
443449
{
444450
/* Call any registered callbacks */
445-
ShutdownExprContext(econtext);
451+
ShutdownExprContext(econtext, true);
446452
/* And clean up the memory used */
447453
MemoryContextReset(econtext->ecxt_per_tuple_memory);
448454
}
@@ -1222,9 +1228,12 @@ UnregisterExprContextCallback(ExprContext *econtext,
12221228
*
12231229
* The callback list is emptied (important in case this is only a rescan
12241230
* reset, and not deletion of the ExprContext).
1231+
*
1232+
* If isCommit is false, just clean the callback list but don't call 'em.
1233+
* (See comment for FreeExprContext.)
12251234
*/
12261235
static void
1227-
ShutdownExprContext(ExprContext *econtext)
1236+
ShutdownExprContext(ExprContext *econtext, bool isCommit)
12281237
{
12291238
ExprContext_CB *ecxt_callback;
12301239
MemoryContext oldcontext;
@@ -1245,7 +1254,8 @@ ShutdownExprContext(ExprContext *econtext)
12451254
while ((ecxt_callback = econtext->ecxt_callbacks) != NULL)
12461255
{
12471256
econtext->ecxt_callbacks = ecxt_callback->next;
1248-
(*ecxt_callback->function) (ecxt_callback->arg);
1257+
if (isCommit)
1258+
(*ecxt_callback->function) (ecxt_callback->arg);
12491259
pfree(ecxt_callback);
12501260
}
12511261

‎src/backend/executor/nodeBitmapIndexscan.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/executor/nodeBitmapIndexscan.c,v 1.30 2009/06/11 14:48:57 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/executor/nodeBitmapIndexscan.c,v 1.31 2009/07/18 19:15:41 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -182,7 +182,7 @@ ExecEndBitmapIndexScan(BitmapIndexScanState *node)
182182
*/
183183
#ifdef NOT_USED
184184
if (node->biss_RuntimeContext)
185-
FreeExprContext(node->biss_RuntimeContext);
185+
FreeExprContext(node->biss_RuntimeContext, true);
186186
#endif
187187

188188
/*

‎src/backend/executor/nodeIndexscan.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/executor/nodeIndexscan.c,v 1.132 2009/06/11 14:48:57 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/executor/nodeIndexscan.c,v 1.133 2009/07/18 19:15:41 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -423,7 +423,7 @@ ExecEndIndexScan(IndexScanState *node)
423423
#ifdef NOT_USED
424424
ExecFreeExprContext(&node->ss.ps);
425425
if (node->iss_RuntimeContext)
426-
FreeExprContext(node->iss_RuntimeContext);
426+
FreeExprContext(node->iss_RuntimeContext, true);
427427
#endif
428428

429429
/*

‎src/include/executor/executor.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $PostgreSQL: pgsql/src/include/executor/executor.h,v 1.155 2009/06/11 14:49:11 momjian Exp $
10+
* $PostgreSQL: pgsql/src/include/executor/executor.h,v 1.156 2009/07/18 19:15:42 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -255,7 +255,7 @@ extern EState *CreateExecutorState(void);
255255
extern void FreeExecutorState(EState *estate);
256256
extern ExprContext *CreateExprContext(EState *estate);
257257
extern ExprContext *CreateStandaloneExprContext(void);
258-
extern void FreeExprContext(ExprContext *econtext);
258+
extern void FreeExprContext(ExprContext *econtext, bool isCommit);
259259
extern void ReScanExprContext(ExprContext *econtext);
260260

261261
#define ResetExprContext(econtext) \

‎src/pl/plpgsql/src/pl_exec.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.244 2009/06/17 13:46:12 petere Exp $
11+
* $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.245 2009/07/18 19:15:42 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -5237,7 +5237,7 @@ plpgsql_destroy_econtext(PLpgSQL_execstate *estate)
52375237
pfree(simple_econtext_stack);
52385238
simple_econtext_stack = next;
52395239

5240-
FreeExprContext(estate->eval_econtext);
5240+
FreeExprContext(estate->eval_econtext, true);
52415241
estate->eval_econtext = NULL;
52425242
}
52435243

@@ -5292,7 +5292,8 @@ plpgsql_subxact_cb(SubXactEvent event, SubTransactionId mySubid,
52925292
{
52935293
SimpleEcontextStackEntry *next;
52945294

5295-
FreeExprContext(simple_econtext_stack->stack_econtext);
5295+
FreeExprContext(simple_econtext_stack->stack_econtext,
5296+
(event == SUBXACT_EVENT_COMMIT_SUB));
52965297
next = simple_econtext_stack->next;
52975298
pfree(simple_econtext_stack);
52985299
simple_econtext_stack = next;

0 commit comments

Comments
 (0)