BufMgr cleanup

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

Manfred Koizar wrote:

Minor code cleanup in bufmgr.c and bufmgr.h, mainly by moving repeated
lines of code into internal routines (drop_relfilenode_buffers,
release_buffer) and by hiding unused routines (PrintBufferDescs,
PrintPinnedBufs) behind #ifdef NOT_USED.
Remove AbortBufferIO() declaration from bufmgr.c (already declared
in bufmgr.h)

diff -ru ../orig/src/backend/storage/buffer/bufmgr.c src/backend/storage/buffer/bufmgr.c
--- ../orig/src/backend/storage/buffer/bufmgr.c	2002-06-13 09:32:40.000000000 +0200
+++ src/backend/storage/buffer/bufmgr.c	2002-06-13 11:02:47.000000000 +0200
@@ -71,7 +71,6 @@
static void StartBufferIO(BufferDesc *buf, bool forInput);
static void TerminateBufferIO(BufferDesc *buf);
static void ContinueBufferIO(BufferDesc *buf, bool forInput);
-extern void AbortBufferIO(void);

/*
* Macro : BUFFER_IS_BROKEN
@@ -85,9 +84,14 @@
bool *foundPtr);
static int ReleaseBufferWithBufferLock(Buffer buffer);
static int BufferReplace(BufferDesc *bufHdr);
+#ifdef NOT_USED
void PrintBufferDescs(void);
+#endif

static void write_buffer(Buffer buffer, bool unpin);
+static void drop_relfilenode_buffers(RelFileNode rnode,
+                                     bool do_local, bool do_both);
+static int release_buffer(Buffer buffer, bool havelock);

/*
* ReadBuffer -- returns a buffer containing the requested
@@ -1097,34 +1101,26 @@
return relation->rd_nblocks;
}

-/* ---------------------------------------------------------------------
- *		DropRelationBuffers
- *
- *		This function removes all the buffered pages for a relation
- *		from the buffer pool.  Dirty pages are simply dropped, without
- *		bothering to write them out first.	This is NOT rollback-able,
- *		and so should be used only with extreme caution!
- *
- *		We assume that the caller holds an exclusive lock on the relation,
- *		which should assure that no new buffers will be acquired for the rel
- *		meanwhile.
+/*
+ * drop_relfilenode_buffers -- common functionality for
+ *                             DropRelationBuffers and
+ *                             DropRelFileNodeBuffers
*
*		XXX currently it sequentially searches the buffer pool, should be
*		changed to more clever ways of searching.
- * --------------------------------------------------------------------
*/
-void
-DropRelationBuffers(Relation rel)
+static void
+drop_relfilenode_buffers(RelFileNode rnode, bool do_local, bool do_both)
{
int			i;
BufferDesc *bufHdr;

-	if (rel->rd_myxactonly)
+	if (do_local)
{
for (i = 0; i < NLocBuffer; i++)
{
bufHdr = &LocalBufferDescriptors[i];
-			if (RelFileNodeEquals(bufHdr->tag.rnode, rel->rd_node))
+			if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
{
bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
bufHdr->cntxDirty = false;
@@ -1132,7 +1128,8 @@
bufHdr->tag.rnode.relNode = InvalidOid;
}
}
-		return;
+		if (!do_both)
+			return;
}

LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
@@ -1141,7 +1138,7 @@
{
bufHdr = &BufferDescriptors[i - 1];
recheck:
-		if (RelFileNodeEquals(bufHdr->tag.rnode, rel->rd_node))
+		if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
{
/*
* If there is I/O in progress, better wait till it's done;
@@ -1188,6 +1185,25 @@
}

/* ---------------------------------------------------------------------
+ *		DropRelationBuffers
+ *
+ *		This function removes all the buffered pages for a relation
+ *		from the buffer pool.  Dirty pages are simply dropped, without
+ *		bothering to write them out first.	This is NOT rollback-able,
+ *		and so should be used only with extreme caution!
+ *
+ *		We assume that the caller holds an exclusive lock on the relation,
+ *		which should assure that no new buffers will be acquired for the rel
+ *		meanwhile.
+ * --------------------------------------------------------------------
+ */
+void
+DropRelationBuffers(Relation rel)
+{
+	drop_relfilenode_buffers(rel->rd_node, rel->rd_myxactonly, false);
+}
+
+/* ---------------------------------------------------------------------
*		DropRelFileNodeBuffers
*
*		This is the same as DropRelationBuffers, except that the target
@@ -1201,73 +1217,8 @@
void
DropRelFileNodeBuffers(RelFileNode rnode)
{
-	int			i;
-	BufferDesc *bufHdr;
-
/* We have to search both local and shared buffers... */
-
-	for (i = 0; i < NLocBuffer; i++)
-	{
-		bufHdr = &LocalBufferDescriptors[i];
-		if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
-		{
-			bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
-			bufHdr->cntxDirty = false;
-			LocalRefCount[i] = 0;
-			bufHdr->tag.rnode.relNode = InvalidOid;
-		}
-	}
-
-	LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
-
-	for (i = 1; i <= NBuffers; i++)
-	{
-		bufHdr = &BufferDescriptors[i - 1];
-recheck:
-		if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
-		{
-			/*
-			 * If there is I/O in progress, better wait till it's done;
-			 * don't want to delete the relation out from under someone
-			 * who's just trying to flush the buffer!
-			 */
-			if (bufHdr->flags & BM_IO_IN_PROGRESS)
-			{
-				WaitIO(bufHdr);
-
-				/*
-				 * By now, the buffer very possibly belongs to some other
-				 * rel, so check again before proceeding.
-				 */
-				goto recheck;
-			}
-			/* Now we can do what we came for */
-			bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
-			bufHdr->cntxDirty = false;
-
-			/*
-			 * Release any refcount we may have.
-			 *
-			 * This is very probably dead code, and if it isn't then it's
-			 * probably wrong.	I added the Assert to find out --- tgl
-			 * 11/99.
-			 */
-			if (!(bufHdr->flags & BM_FREE))
-			{
-				/* Assert checks that buffer will actually get freed! */
-				Assert(PrivateRefCount[i - 1] == 1 &&
-					   bufHdr->refcount == 1);
-				ReleaseBufferWithBufferLock(i);
-			}
-
-			/*
-			 * And mark the buffer as no longer occupied by this rel.
-			 */
-			BufTableDelete(bufHdr);
-		}
-	}
-
-	LWLockRelease(BufMgrLock);
+	drop_relfilenode_buffers(rnode, true, true);
}

/* ---------------------------------------------------------------------
@@ -1343,6 +1294,7 @@
* use only.
* -----------------------------------------------------------------
*/
+#ifdef NOT_USED
void
PrintBufferDescs()
{
@@ -1375,7 +1327,9 @@
}
}
}
+#endif

+#ifdef NOT_USED
void
PrintPinnedBufs()
{
@@ -1395,6 +1349,7 @@
}
LWLockRelease(BufMgrLock);
}
+#endif

/*
* BufferPoolBlowaway
@@ -1589,14 +1544,12 @@
return 0;
}

-#undef ReleaseBuffer
-
/*
- * ReleaseBuffer -- remove the pin on a buffer without
- *		marking it dirty.
+ * release_buffer -- common functionality for
+ *                   ReleaseBuffer and ReleaseBufferWithBufferLock
*/
-int
-ReleaseBuffer(Buffer buffer)
+static int
+release_buffer(Buffer buffer, bool havelock)
{
BufferDesc *bufHdr;

@@ -1617,14 +1570,30 @@
PrivateRefCount[buffer - 1]--;
else
{
-		LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
+		if (!havelock)
+			LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
+
UnpinBuffer(bufHdr);
-		LWLockRelease(BufMgrLock);
+
+		if (!havelock)
+			LWLockRelease(BufMgrLock);
}

return STATUS_OK;
}

+#undef ReleaseBuffer
+
+/*
+ * ReleaseBuffer -- remove the pin on a buffer without
+ *		marking it dirty.
+ */
+int
+ReleaseBuffer(Buffer buffer)
+{
+	return release_buffer(buffer, false);
+}
+
/*
* ReleaseBufferWithBufferLock
*		Same as ReleaseBuffer except we hold the bufmgr lock
@@ -1632,27 +1601,7 @@
static int
ReleaseBufferWithBufferLock(Buffer buffer)
{
-	BufferDesc *bufHdr;
-
-	if (BufferIsLocal(buffer))
-	{
-		Assert(LocalRefCount[-buffer - 1] > 0);
-		LocalRefCount[-buffer - 1]--;
-		return STATUS_OK;
-	}
-
-	if (BAD_BUFFER_ID(buffer))
-		return STATUS_ERROR;
-
-	bufHdr = &BufferDescriptors[buffer - 1];
-
-	Assert(PrivateRefCount[buffer - 1] > 0);
-	if (PrivateRefCount[buffer - 1] > 1)
-		PrivateRefCount[buffer - 1]--;
-	else
-		UnpinBuffer(bufHdr);
-
-	return STATUS_OK;
+	return release_buffer(buffer, true);
}

@@ -1695,7 +1644,7 @@
#endif

#ifdef NOT_USED
-int
+Buffer
ReleaseAndReadBuffer_Debug(char *file,
int line,
Buffer buffer,
@@ -2117,7 +2066,7 @@
{
Assert(buf == InProgressBuf);
LWLockRelease(buf->io_in_progress_lock);
- InProgressBuf = (BufferDesc *) 0;
+ InProgressBuf = (BufferDesc *) NULL;
}

/*
@@ -2142,7 +2091,7 @@
void
InitBufferIO(void)
{
- InProgressBuf = (BufferDesc *) 0;
+ InProgressBuf = (BufferDesc *) NULL;
}
#endif

diff -ru ../orig/src/include/storage/bufmgr.h src/include/storage/bufmgr.h
--- ../orig/src/include/storage/bufmgr.h	2002-06-13 09:22:59.000000000 +0200
+++ src/include/storage/bufmgr.h	2002-06-13 10:57:22.000000000 +0200
@@ -167,7 +167,9 @@
extern void DropRelationBuffers(Relation rel);
extern void DropRelFileNodeBuffers(RelFileNode rnode);
extern void DropBuffers(Oid dbid);
+#ifdef NOT_USED
extern void PrintPinnedBufs(void);
+#endif
extern int	BufferShmemSize(void);
extern RelFileNode BufferGetFileNode(Buffer buffer);

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Patch applied. Thanks.

---------------------------------------------------------------------------

Manfred Koizar wrote:

Minor code cleanup in bufmgr.c and bufmgr.h, mainly by moving repeated
lines of code into internal routines (drop_relfilenode_buffers,
release_buffer) and by hiding unused routines (PrintBufferDescs,
PrintPinnedBufs) behind #ifdef NOT_USED.
Remove AbortBufferIO() declaration from bufmgr.c (already declared
in bufmgr.h)

diff -ru ../orig/src/backend/storage/buffer/bufmgr.c src/backend/storage/buffer/bufmgr.c
--- ../orig/src/backend/storage/buffer/bufmgr.c	2002-06-13 09:32:40.000000000 +0200
+++ src/backend/storage/buffer/bufmgr.c	2002-06-13 11:02:47.000000000 +0200
@@ -71,7 +71,6 @@
static void StartBufferIO(BufferDesc *buf, bool forInput);
static void TerminateBufferIO(BufferDesc *buf);
static void ContinueBufferIO(BufferDesc *buf, bool forInput);
-extern void AbortBufferIO(void);

/*
* Macro : BUFFER_IS_BROKEN
@@ -85,9 +84,14 @@
bool *foundPtr);
static int ReleaseBufferWithBufferLock(Buffer buffer);
static int BufferReplace(BufferDesc *bufHdr);
+#ifdef NOT_USED
void PrintBufferDescs(void);
+#endif

static void write_buffer(Buffer buffer, bool unpin);
+static void drop_relfilenode_buffers(RelFileNode rnode,
+                                     bool do_local, bool do_both);
+static int release_buffer(Buffer buffer, bool havelock);

/*
* ReadBuffer -- returns a buffer containing the requested
@@ -1097,34 +1101,26 @@
return relation->rd_nblocks;
}

-/* ---------------------------------------------------------------------
- *		DropRelationBuffers
- *
- *		This function removes all the buffered pages for a relation
- *		from the buffer pool.  Dirty pages are simply dropped, without
- *		bothering to write them out first.	This is NOT rollback-able,
- *		and so should be used only with extreme caution!
- *
- *		We assume that the caller holds an exclusive lock on the relation,
- *		which should assure that no new buffers will be acquired for the rel
- *		meanwhile.
+/*
+ * drop_relfilenode_buffers -- common functionality for
+ *                             DropRelationBuffers and
+ *                             DropRelFileNodeBuffers
*
*		XXX currently it sequentially searches the buffer pool, should be
*		changed to more clever ways of searching.
- * --------------------------------------------------------------------
*/
-void
-DropRelationBuffers(Relation rel)
+static void
+drop_relfilenode_buffers(RelFileNode rnode, bool do_local, bool do_both)
{
int			i;
BufferDesc *bufHdr;

-	if (rel->rd_myxactonly)
+	if (do_local)
{
for (i = 0; i < NLocBuffer; i++)
{
bufHdr = &LocalBufferDescriptors[i];
-			if (RelFileNodeEquals(bufHdr->tag.rnode, rel->rd_node))
+			if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
{
bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
bufHdr->cntxDirty = false;
@@ -1132,7 +1128,8 @@
bufHdr->tag.rnode.relNode = InvalidOid;
}
}
-		return;
+		if (!do_both)
+			return;
}

LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
@@ -1141,7 +1138,7 @@
{
bufHdr = &BufferDescriptors[i - 1];
recheck:
-		if (RelFileNodeEquals(bufHdr->tag.rnode, rel->rd_node))
+		if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
{
/*
* If there is I/O in progress, better wait till it's done;
@@ -1188,6 +1185,25 @@
}

/* ---------------------------------------------------------------------
+ *		DropRelationBuffers
+ *
+ *		This function removes all the buffered pages for a relation
+ *		from the buffer pool.  Dirty pages are simply dropped, without
+ *		bothering to write them out first.	This is NOT rollback-able,
+ *		and so should be used only with extreme caution!
+ *
+ *		We assume that the caller holds an exclusive lock on the relation,
+ *		which should assure that no new buffers will be acquired for the rel
+ *		meanwhile.
+ * --------------------------------------------------------------------
+ */
+void
+DropRelationBuffers(Relation rel)
+{
+	drop_relfilenode_buffers(rel->rd_node, rel->rd_myxactonly, false);
+}
+
+/* ---------------------------------------------------------------------
*		DropRelFileNodeBuffers
*
*		This is the same as DropRelationBuffers, except that the target
@@ -1201,73 +1217,8 @@
void
DropRelFileNodeBuffers(RelFileNode rnode)
{
-	int			i;
-	BufferDesc *bufHdr;
-
/* We have to search both local and shared buffers... */
-
-	for (i = 0; i < NLocBuffer; i++)
-	{
-		bufHdr = &LocalBufferDescriptors[i];
-		if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
-		{
-			bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
-			bufHdr->cntxDirty = false;
-			LocalRefCount[i] = 0;
-			bufHdr->tag.rnode.relNode = InvalidOid;
-		}
-	}
-
-	LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
-
-	for (i = 1; i <= NBuffers; i++)
-	{
-		bufHdr = &BufferDescriptors[i - 1];
-recheck:
-		if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
-		{
-			/*
-			 * If there is I/O in progress, better wait till it's done;
-			 * don't want to delete the relation out from under someone
-			 * who's just trying to flush the buffer!
-			 */
-			if (bufHdr->flags & BM_IO_IN_PROGRESS)
-			{
-				WaitIO(bufHdr);
-
-				/*
-				 * By now, the buffer very possibly belongs to some other
-				 * rel, so check again before proceeding.
-				 */
-				goto recheck;
-			}
-			/* Now we can do what we came for */
-			bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
-			bufHdr->cntxDirty = false;
-
-			/*
-			 * Release any refcount we may have.
-			 *
-			 * This is very probably dead code, and if it isn't then it's
-			 * probably wrong.	I added the Assert to find out --- tgl
-			 * 11/99.
-			 */
-			if (!(bufHdr->flags & BM_FREE))
-			{
-				/* Assert checks that buffer will actually get freed! */
-				Assert(PrivateRefCount[i - 1] == 1 &&
-					   bufHdr->refcount == 1);
-				ReleaseBufferWithBufferLock(i);
-			}
-
-			/*
-			 * And mark the buffer as no longer occupied by this rel.
-			 */
-			BufTableDelete(bufHdr);
-		}
-	}
-
-	LWLockRelease(BufMgrLock);
+	drop_relfilenode_buffers(rnode, true, true);
}

/* ---------------------------------------------------------------------
@@ -1343,6 +1294,7 @@
* use only.
* -----------------------------------------------------------------
*/
+#ifdef NOT_USED
void
PrintBufferDescs()
{
@@ -1375,7 +1327,9 @@
}
}
}
+#endif

+#ifdef NOT_USED
void
PrintPinnedBufs()
{
@@ -1395,6 +1349,7 @@
}
LWLockRelease(BufMgrLock);
}
+#endif

/*
* BufferPoolBlowaway
@@ -1589,14 +1544,12 @@
return 0;
}

-#undef ReleaseBuffer
-
/*
- * ReleaseBuffer -- remove the pin on a buffer without
- *		marking it dirty.
+ * release_buffer -- common functionality for
+ *                   ReleaseBuffer and ReleaseBufferWithBufferLock
*/
-int
-ReleaseBuffer(Buffer buffer)
+static int
+release_buffer(Buffer buffer, bool havelock)
{
BufferDesc *bufHdr;

@@ -1617,14 +1570,30 @@
PrivateRefCount[buffer - 1]--;
else
{
-		LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
+		if (!havelock)
+			LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
+
UnpinBuffer(bufHdr);
-		LWLockRelease(BufMgrLock);
+
+		if (!havelock)
+			LWLockRelease(BufMgrLock);
}

return STATUS_OK;
}

+#undef ReleaseBuffer
+
+/*
+ * ReleaseBuffer -- remove the pin on a buffer without
+ *		marking it dirty.
+ */
+int
+ReleaseBuffer(Buffer buffer)
+{
+	return release_buffer(buffer, false);
+}
+
/*
* ReleaseBufferWithBufferLock
*		Same as ReleaseBuffer except we hold the bufmgr lock
@@ -1632,27 +1601,7 @@
static int
ReleaseBufferWithBufferLock(Buffer buffer)
{
-	BufferDesc *bufHdr;
-
-	if (BufferIsLocal(buffer))
-	{
-		Assert(LocalRefCount[-buffer - 1] > 0);
-		LocalRefCount[-buffer - 1]--;
-		return STATUS_OK;
-	}
-
-	if (BAD_BUFFER_ID(buffer))
-		return STATUS_ERROR;
-
-	bufHdr = &BufferDescriptors[buffer - 1];
-
-	Assert(PrivateRefCount[buffer - 1] > 0);
-	if (PrivateRefCount[buffer - 1] > 1)
-		PrivateRefCount[buffer - 1]--;
-	else
-		UnpinBuffer(bufHdr);
-
-	return STATUS_OK;
+	return release_buffer(buffer, true);
}

@@ -1695,7 +1644,7 @@
#endif

#ifdef NOT_USED
-int
+Buffer
ReleaseAndReadBuffer_Debug(char *file,
int line,
Buffer buffer,
@@ -2117,7 +2066,7 @@
{
Assert(buf == InProgressBuf);
LWLockRelease(buf->io_in_progress_lock);
- InProgressBuf = (BufferDesc *) 0;
+ InProgressBuf = (BufferDesc *) NULL;
}

/*
@@ -2142,7 +2091,7 @@
void
InitBufferIO(void)
{
- InProgressBuf = (BufferDesc *) 0;
+ InProgressBuf = (BufferDesc *) NULL;
}
#endif

diff -ru ../orig/src/include/storage/bufmgr.h src/include/storage/bufmgr.h
--- ../orig/src/include/storage/bufmgr.h	2002-06-13 09:22:59.000000000 +0200
+++ src/include/storage/bufmgr.h	2002-06-13 10:57:22.000000000 +0200
@@ -167,7 +167,9 @@
extern void DropRelationBuffers(Relation rel);
extern void DropRelFileNodeBuffers(RelFileNode rnode);
extern void DropBuffers(Oid dbid);
+#ifdef NOT_USED
extern void PrintPinnedBufs(void);
+#endif
extern int	BufferShmemSize(void);
extern RelFileNode BufferGetFileNode(Buffer buffer);

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026