From fdd13f156814f81732c188788ab1b7b14c59f4da Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sat, 16 Oct 2004 18:57:26 +0000
Subject: [PATCH] Give the ResourceOwner mechanism full responsibility for
releasing buffer pins at end of transaction, and reduce AtEOXact_Buffers to
an Assert cross-check that this was done correctly. When not
USE_ASSERT_CHECKING, AtEOXact_Buffers is a complete no-op. This gets rid of
an O(NBuffers) bottleneck during transaction commit/abort, which recent
testing has shown becomes significant above a few tens of thousands of shared
buffers.
---
src/backend/access/transam/xact.c | 6 ++-
src/backend/storage/buffer/bufmgr.c | 75 ++++++++++++++++++++-------
src/backend/storage/buffer/localbuf.c | 19 ++-----
src/backend/storage/lmgr/proc.c | 10 ++--
src/backend/utils/resowner/resowner.c | 53 ++++++++-----------
src/include/storage/bufmgr.h | 4 +-
6 files changed, 95 insertions(+), 72 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 321a86f30c..7a6fd3a132 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.191 2004/10/04 21:52:14 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.192 2004/10/16 18:57:22 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -1528,6 +1528,9 @@ CommitTransaction(void)
RESOURCE_RELEASE_BEFORE_LOCKS,
true, true);
+ /* Check we've released all buffer pins */
+ AtEOXact_Buffers(true);
+
/*
* Make catalog changes visible to all backends. This has to happen
* after relcache references are dropped (see comments for
@@ -1684,6 +1687,7 @@ AbortTransaction(void)
ResourceOwnerRelease(TopTransactionResourceOwner,
RESOURCE_RELEASE_BEFORE_LOCKS,
false, true);
+ AtEOXact_Buffers(false);
AtEOXact_Inval(false);
smgrDoPendingDeletes(false);
ResourceOwnerRelease(TopTransactionResourceOwner,
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index a8cf506688..b9d8fc3ad5 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -8,7 +8,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/storage/buffer/bufmgr.c,v 1.179 2004/10/16 18:05:06 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/storage/buffer/bufmgr.c,v 1.180 2004/10/16 18:57:23 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -851,35 +851,45 @@ ResetBufferUsage(void)
/*
* AtEOXact_Buffers - clean up at end of transaction.
*
- * During abort, we need to release any buffer pins we're holding
- * (this cleans up in case ereport interrupted a routine that pins a
- * buffer). During commit, we shouldn't need to do that, but check
- * anyway to see if anyone leaked a buffer reference count.
+ * As of PostgreSQL 8.0, buffer pins should get released by the
+ * ResourceOwner mechanism. This routine is just a debugging
+ * cross-check that no pins remain.
*/
void
AtEOXact_Buffers(bool isCommit)
{
+#ifdef USE_ASSERT_CHECKING
int i;
+ for (i = 0; i < NBuffers; i++)
+ {
+ Assert(PrivateRefCount[i] == 0);
+ }
+
+ AtEOXact_LocalBuffers(isCommit);
+#endif
+}
+
+/*
+ * Ensure we have released all shared-buffer locks and pins during backend exit
+ */
+void
+AtProcExit_Buffers(void)
+{
+ int i;
+
+ AbortBufferIO();
+ UnlockBuffers();
+
for (i = 0; i < NBuffers; i++)
{
if (PrivateRefCount[i] != 0)
{
BufferDesc *buf = &(BufferDescriptors[i]);
- if (isCommit)
- elog(WARNING,
- "buffer refcount leak: [%03d] "
- "(rel=%u/%u/%u, blockNum=%u, flags=0x%x, refcount=%u %d)",
- i,
- buf->tag.rnode.spcNode, buf->tag.rnode.dbNode,
- buf->tag.rnode.relNode,
- buf->tag.blockNum, buf->flags,
- buf->refcount, PrivateRefCount[i]);
-
/*
- * We don't worry about updating the ResourceOwner structures;
- * resowner.c will clear them for itself.
+ * We don't worry about updating ResourceOwner; if we even got
+ * here, it suggests that ResourceOwners are messed up.
*/
PrivateRefCount[i] = 1; /* make sure we release shared pin */
LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
@@ -888,8 +898,37 @@ AtEOXact_Buffers(bool isCommit)
Assert(PrivateRefCount[i] == 0);
}
}
+}
- AtEOXact_LocalBuffers(isCommit);
+/*
+ * Helper routine to issue warnings when a buffer is unexpectedly pinned
+ */
+void
+PrintBufferLeakWarning(Buffer buffer)
+{
+ BufferDesc *buf;
+ int32 loccount;
+
+ Assert(BufferIsValid(buffer));
+ if (BufferIsLocal(buffer))
+ {
+ buf = &LocalBufferDescriptors[-buffer - 1];
+ loccount = LocalRefCount[-buffer - 1];
+ }
+ else
+ {
+ buf = &BufferDescriptors[buffer - 1];
+ loccount = PrivateRefCount[buffer - 1];
+ }
+
+ elog(WARNING,
+ "buffer refcount leak: [%03d] "
+ "(rel=%u/%u/%u, blockNum=%u, flags=0x%x, refcount=%u %d)",
+ buffer,
+ buf->tag.rnode.spcNode, buf->tag.rnode.dbNode,
+ buf->tag.rnode.relNode,
+ buf->tag.blockNum, buf->flags,
+ buf->refcount, loccount);
}
/*
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 6ccc18ddad..4df6befcac 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -9,7 +9,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/storage/buffer/localbuf.c,v 1.59 2004/08/29 05:06:47 momjian Exp $
+ * $PostgreSQL: pgsql/src/backend/storage/buffer/localbuf.c,v 1.60 2004/10/16 18:57:24 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -232,23 +232,12 @@ InitLocalBuffer(void)
void
AtEOXact_LocalBuffers(bool isCommit)
{
+#ifdef USE_ASSERT_CHECKING
int i;
for (i = 0; i < NLocBuffer; i++)
{
- if (LocalRefCount[i] != 0)
- {
- BufferDesc *buf = &(LocalBufferDescriptors[i]);
-
- if (isCommit)
- elog(WARNING,
- "local buffer leak: [%03d] (rel=%u/%u/%u, blockNum=%u, flags=0x%x, refcount=%u %d)",
- i,
- buf->tag.rnode.spcNode, buf->tag.rnode.dbNode,
- buf->tag.rnode.relNode, buf->tag.blockNum, buf->flags,
- buf->refcount, LocalRefCount[i]);
-
- LocalRefCount[i] = 0;
- }
+ Assert(LocalRefCount[i] == 0);
}
+#endif
}
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index ac04b5e804..e50ed2f08f 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -8,7 +8,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/storage/lmgr/proc.c,v 1.154 2004/09/29 15:15:55 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/storage/lmgr/proc.c,v 1.155 2004/10/16 18:57:24 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -461,9 +461,7 @@ ProcKill(int code, Datum arg)
* shutdown callback registered by the bufmgr ... but we must do this
* *after* LWLockReleaseAll and *before* zapping MyProc.
*/
- AbortBufferIO();
- UnlockBuffers();
- AtEOXact_Buffers(false);
+ AtProcExit_Buffers();
/* Get off any wait queue I might be on */
LockWaitCancel();
@@ -509,9 +507,7 @@ DummyProcKill(int code, Datum arg)
LWLockReleaseAll();
/* Release buffer locks and pins, too */
- AbortBufferIO();
- UnlockBuffers();
- AtEOXact_Buffers(false);
+ AtProcExit_Buffers();
/* I can't be on regular lock queues, so needn't check */
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index 4075e2c45e..293cb31a26 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -14,7 +14,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/utils/resowner/resowner.c,v 1.7 2004/08/30 02:54:40 momjian Exp $
+ * $PostgreSQL: pgsql/src/backend/utils/resowner/resowner.c,v 1.8 2004/10/16 18:57:25 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -191,37 +191,30 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
if (phase == RESOURCE_RELEASE_BEFORE_LOCKS)
{
- /* Release buffer pins */
- if (isTopLevel)
+ /*
+ * Release buffer pins. Note that ReleaseBuffer will
+ * remove the buffer entry from my list, so I just have to
+ * iterate till there are none.
+ *
+ * During a commit, there shouldn't be any remaining pins ---
+ * that would indicate failure to clean up the executor correctly ---
+ * so issue warnings. In the abort case, just clean up quietly.
+ *
+ * XXX this is fairly inefficient due to multiple BufMgrLock
+ * grabs if there are lots of buffers to be released, but we
+ * don't expect many (indeed none in the success case) so it's
+ * probably not worth optimizing.
+ *
+ * We are however careful to release back-to-front, so as to
+ * avoid O(N^2) behavior in ResourceOwnerForgetBuffer().
+ */
+ while (owner->nbuffers > 0)
{
- /*
- * For a top-level xact we are going to release all buffers,
- * so just do a single bufmgr call at the top of the
- * recursion.
- */
- if (owner == TopTransactionResourceOwner)
- AtEOXact_Buffers(isCommit);
- /* Mark object as owning no buffers, just for sanity */
- owner->nbuffers = 0;
- }
- else
- {
- /*
- * Release buffers retail. Note that ReleaseBuffer will
- * remove the buffer entry from my list, so I just have to
- * iterate till there are none.
- *
- * XXX this is fairly inefficient due to multiple BufMgrLock
- * grabs if there are lots of buffers to be released, but we
- * don't expect many (indeed none in the success case) so it's
- * probably not worth optimizing.
- *
- * We are however careful to release back-to-front, so as to
- * avoid O(N^2) behavior in ResourceOwnerForgetBuffer().
- */
- while (owner->nbuffers > 0)
- ReleaseBuffer(owner->buffers[owner->nbuffers - 1]);
+ if (isCommit)
+ PrintBufferLeakWarning(owner->buffers[owner->nbuffers - 1]);
+ ReleaseBuffer(owner->buffers[owner->nbuffers - 1]);
}
+
/* Release relcache references */
if (isTopLevel)
{
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 04bc09e22c..5064a1857f 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.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/storage/bufmgr.h,v 1.87 2004/10/15 22:40:25 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/storage/bufmgr.h,v 1.88 2004/10/16 18:57:26 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -122,6 +122,8 @@ extern void InitBufferPoolAccess(void);
extern char *ShowBufferUsage(void);
extern void ResetBufferUsage(void);
extern void AtEOXact_Buffers(bool isCommit);
+extern void AtProcExit_Buffers(void);
+extern void PrintBufferLeakWarning(Buffer buffer);
extern void FlushBufferPool(void);
extern BlockNumber BufferGetBlockNumber(Buffer buffer);
extern BlockNumber RelationGetNumberOfBlocks(Relation relation);