From 257cccbe5ea9f20988ddaa6654a6b1ccf1925620 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 16 Sep 2004 20:17:49 +0000
Subject: [PATCH] Add some marginal tweaks to eliminate memory leakages
associated with subtransactions. Trivial subxacts (such as a plpgsql
exception block containing no database access) now demonstrably leak zero
bytes.
---
src/backend/access/transam/xact.c | 57 +++++++++++++++++++++++++++----
src/backend/executor/spi.c | 23 +++++++++++--
src/backend/utils/mmgr/aset.c | 24 ++++++++++++-
src/backend/utils/mmgr/mcxt.c | 24 +++++++++++--
src/include/nodes/memnodes.h | 3 +-
src/include/utils/memutils.h | 3 +-
6 files changed, 118 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index fe99ba15f36..17db7dd78d5 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -10,7 +10,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.189 2004/09/16 16:58:26 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.190 2004/09/16 20:17:16 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -861,9 +861,6 @@ AtCommit_Memory(void)
/*
* AtSubCommit_Memory
- *
- * We do not throw away the child's CurTransactionContext, since the data
- * it contains will be needed at upper commit.
*/
static void
AtSubCommit_Memory(void)
@@ -875,6 +872,18 @@ AtSubCommit_Memory(void)
/* Return to parent transaction level's memory context. */
CurTransactionContext = s->parent->curTransactionContext;
MemoryContextSwitchTo(CurTransactionContext);
+
+ /*
+ * Ordinarily we cannot throw away the child's CurTransactionContext,
+ * since the data it contains will be needed at upper commit. However,
+ * if there isn't actually anything in it, we can throw it away. This
+ * avoids a small memory leak in the common case of "trivial" subxacts.
+ */
+ if (MemoryContextIsEmpty(s->curTransactionContext))
+ {
+ MemoryContextDelete(s->curTransactionContext);
+ s->curTransactionContext = NULL;
+ }
}
/*
@@ -890,13 +899,27 @@ AtSubCommit_childXids(void)
Assert(s->parent != NULL);
- old_cxt = MemoryContextSwitchTo(s->parent->curTransactionContext);
+ /*
+ * We keep the child-XID lists in TopTransactionContext; this avoids
+ * setting up child-transaction contexts for what might be just a few
+ * bytes of grandchild XIDs.
+ */
+ old_cxt = MemoryContextSwitchTo(TopTransactionContext);
s->parent->childXids = lappend_xid(s->parent->childXids,
s->transactionId);
- s->parent->childXids = list_concat(s->parent->childXids, s->childXids);
- s->childXids = NIL; /* ensure list not doubly referenced */
+ if (s->childXids != NIL)
+ {
+ s->parent->childXids = list_concat(s->parent->childXids,
+ s->childXids);
+ /*
+ * list_concat doesn't free the list header for the second list;
+ * do so here to avoid memory leakage (kluge)
+ */
+ pfree(s->childXids);
+ s->childXids = NIL;
+ }
MemoryContextSwitchTo(old_cxt);
}
@@ -1092,6 +1115,23 @@ AtSubAbort_Memory(void)
MemoryContextSwitchTo(TopTransactionContext);
}
+/*
+ * AtSubAbort_childXids
+ */
+static void
+AtSubAbort_childXids(void)
+{
+ TransactionState s = CurrentTransactionState;
+
+ /*
+ * We keep the child-XID lists in TopTransactionContext (see
+ * AtSubCommit_childXids). This means we'd better free the list
+ * explicitly at abort to avoid leakage.
+ */
+ list_free(s->childXids);
+ s->childXids = NIL;
+}
+
/*
* RecordSubTransactionAbort
*/
@@ -3317,7 +3357,10 @@ AbortSubTransaction(void)
/* Advertise the fact that we aborted in pg_clog. */
if (TransactionIdIsValid(s->transactionId))
+ {
RecordSubTransactionAbort();
+ AtSubAbort_childXids();
+ }
/* Post-abort cleanup */
CallSubXactCallbacks(SUBXACT_EVENT_ABORT_SUB, s->subTransactionId,
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index ab26a91c1aa..3845b94eb92 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -8,7 +8,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.128 2004/09/16 16:58:29 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.129 2004/09/16 20:17:20 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -104,6 +104,8 @@ SPI_connect(void)
_SPI_current = &(_SPI_stack[_SPI_connected]);
_SPI_current->processed = 0;
_SPI_current->tuptable = NULL;
+ _SPI_current->procCxt = NULL; /* in case we fail to create 'em */
+ _SPI_current->execCxt = NULL;
_SPI_current->connectSubid = GetCurrentSubTransactionId();
/*
@@ -144,7 +146,9 @@ SPI_finish(void)
/* Release memory used in procedure call */
MemoryContextDelete(_SPI_current->execCxt);
+ _SPI_current->execCxt = NULL;
MemoryContextDelete(_SPI_current->procCxt);
+ _SPI_current->procCxt = NULL;
/*
* Reset result variables, especially SPI_tuptable which is probably
@@ -214,11 +218,24 @@ AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid)
found = true;
+ /*
+ * Release procedure memory explicitly (see note in SPI_connect)
+ */
+ if (connection->execCxt)
+ {
+ MemoryContextDelete(connection->execCxt);
+ connection->execCxt = NULL;
+ }
+ if (connection->procCxt)
+ {
+ MemoryContextDelete(connection->procCxt);
+ connection->procCxt = NULL;
+ }
+
/*
* Pop the stack entry and reset global variables. Unlike
* SPI_finish(), we don't risk switching to memory contexts that
- * might be already gone, or deleting memory contexts that have
- * been or will be thrown away anyway.
+ * might be already gone.
*/
_SPI_connected--;
_SPI_curid = _SPI_connected;
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 79da5fe0175..b25d870059a 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -11,7 +11,7 @@
* Portions Copyright (c) 1994, Regents of the University of California
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/utils/mmgr/aset.c,v 1.57 2004/08/29 05:06:51 momjian Exp $
+ * $PostgreSQL: pgsql/src/backend/utils/mmgr/aset.c,v 1.58 2004/09/16 20:17:33 tgl Exp $
*
* NOTE:
* This is a new (Feb. 05, 1999) implementation of the allocation set
@@ -205,6 +205,7 @@ static void AllocSetInit(MemoryContext context);
static void AllocSetReset(MemoryContext context);
static void AllocSetDelete(MemoryContext context);
static Size AllocSetGetChunkSpace(MemoryContext context, void *pointer);
+static bool AllocSetIsEmpty(MemoryContext context);
static void AllocSetStats(MemoryContext context);
#ifdef MEMORY_CONTEXT_CHECKING
@@ -222,6 +223,7 @@ static MemoryContextMethods AllocSetMethods = {
AllocSetReset,
AllocSetDelete,
AllocSetGetChunkSpace,
+ AllocSetIsEmpty,
AllocSetStats
#ifdef MEMORY_CONTEXT_CHECKING
,AllocSetCheck
@@ -991,6 +993,26 @@ AllocSetGetChunkSpace(MemoryContext context, void *pointer)
return chunk->size + ALLOC_CHUNKHDRSZ;
}
+/*
+ * AllocSetIsEmpty
+ * Is an allocset empty of any allocated space?
+ */
+static bool
+AllocSetIsEmpty(MemoryContext context)
+{
+ AllocSet set = (AllocSet) context;
+
+ /*
+ * For now, we say "empty" only if the context never contained any
+ * space at all. We could examine the freelists to determine if all
+ * space has been freed, but it's not really worth the trouble for
+ * present uses of this functionality.
+ */
+ if (set->blocks == NULL)
+ return true;
+ return false;
+}
+
/*
* AllocSetStats
* Displays stats about memory consumption of an allocset.
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index f8e0af4f8b0..581c7f396d8 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -14,7 +14,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/utils/mmgr/mcxt.c,v 1.50 2004/08/29 05:06:51 momjian Exp $
+ * $PostgreSQL: pgsql/src/backend/utils/mmgr/mcxt.c,v 1.51 2004/09/16 20:17:33 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -291,6 +291,25 @@ GetMemoryChunkContext(void *pointer)
return header->context;
}
+/*
+ * MemoryContextIsEmpty
+ * Is a memory context empty of any allocated space?
+ */
+bool
+MemoryContextIsEmpty(MemoryContext context)
+{
+ AssertArg(MemoryContextIsValid(context));
+
+ /*
+ * For now, we consider a memory context nonempty if it has any children;
+ * perhaps this should be changed later.
+ */
+ if (context->firstchild != NULL)
+ return false;
+ /* Otherwise use the type-specific inquiry */
+ return (*context->methods->is_empty) (context);
+}
+
/*
* MemoryContextStats
* Print statistics about the named context and all its descendants.
@@ -662,7 +681,6 @@ void
pgport_pfree(void *pointer)
{
pfree(pointer);
- return;
}
-#endif
+#endif /* WIN32 */
diff --git a/src/include/nodes/memnodes.h b/src/include/nodes/memnodes.h
index 1fc13af47f7..e57ee2aad27 100644
--- a/src/include/nodes/memnodes.h
+++ b/src/include/nodes/memnodes.h
@@ -7,7 +7,7 @@
* Portions Copyright (c) 1996-2004, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
- * $PostgreSQL: pgsql/src/include/nodes/memnodes.h,v 1.28 2004/08/29 04:13:07 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/nodes/memnodes.h,v 1.29 2004/09/16 20:17:42 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -43,6 +43,7 @@ typedef struct MemoryContextMethods
void (*reset) (MemoryContext context);
void (*delete) (MemoryContext context);
Size (*get_chunk_space) (MemoryContext context, void *pointer);
+ bool (*is_empty) (MemoryContext context);
void (*stats) (MemoryContext context);
#ifdef MEMORY_CONTEXT_CHECKING
void (*check) (MemoryContext context);
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 8cf7d8bc469..671cbbf1ef2 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -10,7 +10,7 @@
* Portions Copyright (c) 1996-2004, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
- * $PostgreSQL: pgsql/src/include/utils/memutils.h,v 1.57 2004/08/29 04:13:11 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/utils/memutils.h,v 1.58 2004/09/16 20:17:49 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -91,6 +91,7 @@ extern void MemoryContextDeleteChildren(MemoryContext context);
extern void MemoryContextResetAndDeleteChildren(MemoryContext context);
extern Size GetMemoryChunkSpace(void *pointer);
extern MemoryContext GetMemoryChunkContext(void *pointer);
+extern bool MemoryContextIsEmpty(MemoryContext context);
extern void MemoryContextStats(MemoryContext context);
#ifdef MEMORY_CONTEXT_CHECKING
--
GitLab