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

@@ -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,

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);
 }
 
 /*

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
 }

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 */
 

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)
-		{
-			/*
-			 * 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 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)
 		{
-			/*
-			 * 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)
 		{

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