bufmgr code cleanup

Neil Conway wrote:

This patch cleans up some of the bufmgr code:

- replace uses of

LockBuffer(buf, BUFFER_LOCK_UNLOCK);
ReleaseBuffer(buf);

with the equivalent, but more concise:

UnlockAndReleaseBuffer(buf);

- analogous changes were made by replacing LockBuffer() + WriteBuffer()
with UnlockAndWriteBuffer()

- remove a bunch of #ifdef BMTRACE code, since it was ugly and broken
anyway

- remove an unused buffer descriptor bit flag (BM_PRIVATE)

- move the definition of INVALID_DESCRIPTOR to out of bufmgr.h and into
freelist.c, since it is the only file that uses it

- remove another unused function, and fix a few comments

Please apply to the CVS HEAD.

Can this be held off a little while we're experimenting with
improvements to the buffer algorithms?

Jan

-Neil

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

Index: src/backend/access/hash/hashpage.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/access/hash/hashpage.c,v
retrieving revision 1.42
diff -c -r1.42 hashpage.c
*** src/backend/access/hash/hashpage.c	4 Sep 2003 22:06:27 -0000	1.42
--- src/backend/access/hash/hashpage.c	31 Oct 2003 22:55:59 -0000
***************
*** 135,142 ****
void
_hash_relbuf(Relation rel, Buffer buf)
{
! 	LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! 	ReleaseBuffer(buf);
}

/*
--- 135,141 ----
void
_hash_relbuf(Relation rel, Buffer buf)
{
! 	UnlockAndReleaseBuffer(buf);
}

/*
***************
*** 166,173 ****
void
_hash_wrtbuf(Relation rel, Buffer buf)
{
! LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! WriteBuffer(buf);
}

/*
--- 165,171 ----
void
_hash_wrtbuf(Relation rel, Buffer buf)
{
! 	UnlockAndWriteBuffer(buf);
}

/*
Index: src/backend/access/heap/heapam.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/access/heap/heapam.c,v
retrieving revision 1.157
diff -c -r1.157 heapam.c
*** src/backend/access/heap/heapam.c	1 Oct 2003 21:30:52 -0000	1.157
--- src/backend/access/heap/heapam.c	31 Oct 2003 22:59:14 -0000
***************
*** 899,906 ****
*/
if (!ItemIdIsUsed(lp))
{
! 		LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
! 		ReleaseBuffer(buffer);
*userbuf = InvalidBuffer;
tuple->t_datamcxt = NULL;
tuple->t_data = NULL;
--- 899,905 ----
*/
if (!ItemIdIsUsed(lp))
{
! 		UnlockAndReleaseBuffer(buffer);
*userbuf = InvalidBuffer;
tuple->t_datamcxt = NULL;
tuple->t_data = NULL;
***************
*** 1006,1013 ****
}
if (invalidBlock)
{
! 		LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
! 		ReleaseBuffer(buffer);
return NULL;
}

--- 1005,1011 ----
}
if (invalidBlock)
{
! 		UnlockAndReleaseBuffer(buffer);
return NULL;
}

***************
*** 1033,1040 ****
!ItemPointerEquals(tid, &ctid))
linkend = false;

! LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
! ReleaseBuffer(buffer);

if (!valid)
{
--- 1031,1037 ----
!ItemPointerEquals(tid, &ctid))
linkend = false;

! UnlockAndReleaseBuffer(buffer);

if (!valid)
{
***************
*** 1174,1181 ****

END_CRIT_SECTION();

! LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
! WriteBuffer(buffer);

/*
* If tuple is cachable, mark it for invalidation from the caches in
--- 1171,1177 ----

END_CRIT_SECTION();

! UnlockAndWriteBuffer(buffer);

/*
* If tuple is cachable, mark it for invalidation from the caches in
***************
*** 1253,1260 ****

if (result == HeapTupleInvisible)
{
! 		LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
! 		ReleaseBuffer(buffer);
elog(ERROR, "attempted to delete invisible tuple");
}
else if (result == HeapTupleBeingUpdated && wait)
--- 1249,1255 ----

if (result == HeapTupleInvisible)
{
! UnlockAndReleaseBuffer(buffer);
elog(ERROR, "attempted to delete invisible tuple");
}
else if (result == HeapTupleBeingUpdated && wait)
***************
*** 1301,1308 ****
result == HeapTupleUpdated ||
result == HeapTupleBeingUpdated);
*ctid = tp.t_data->t_ctid;
! LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
! ReleaseBuffer(buffer);
return result;
}

--- 1296,1302 ----
result == HeapTupleUpdated ||
result == HeapTupleBeingUpdated);
*ctid = tp.t_data->t_ctid;
! 		UnlockAndReleaseBuffer(buffer);
return result;
}

***************
*** 1483,1490 ****

if (result == HeapTupleInvisible)
{
! 		LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
! 		ReleaseBuffer(buffer);
elog(ERROR, "attempted to update invisible tuple");
}
else if (result == HeapTupleBeingUpdated && wait)
--- 1477,1483 ----

if (result == HeapTupleInvisible)
{
! UnlockAndReleaseBuffer(buffer);
elog(ERROR, "attempted to update invisible tuple");
}
else if (result == HeapTupleBeingUpdated && wait)
***************
*** 1531,1538 ****
result == HeapTupleUpdated ||
result == HeapTupleBeingUpdated);
*ctid = oldtup.t_data->t_ctid;
! LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
! ReleaseBuffer(buffer);
return result;
}

--- 1524,1530 ----
result == HeapTupleUpdated ||
result == HeapTupleBeingUpdated);
*ctid = oldtup.t_data->t_ctid;
! 		UnlockAndReleaseBuffer(buffer);
return result;
}

***************
*** 1808,1815 ****

if (result == HeapTupleInvisible)
{
! 		LockBuffer(*buffer, BUFFER_LOCK_UNLOCK);
! 		ReleaseBuffer(*buffer);
elog(ERROR, "attempted to mark4update invisible tuple");
}
else if (result == HeapTupleBeingUpdated)
--- 1800,1806 ----

if (result == HeapTupleInvisible)
{
! 		UnlockAndReleaseBuffer(*buffer);
elog(ERROR, "attempted to mark4update invisible tuple");
}
else if (result == HeapTupleBeingUpdated)
Index: src/backend/access/index/indexam.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/access/index/indexam.c,v
retrieving revision 1.71
diff -c -r1.71 indexam.c
*** src/backend/access/index/indexam.c	25 Sep 2003 06:57:57 -0000	1.71
--- src/backend/access/index/indexam.c	31 Oct 2003 23:00:00 -0000
***************
*** 541,548 ****

if (sv_infomask != heapTuple->t_data->t_infomask)
SetBufferCommitInfoNeedsSave(scan->xs_cbuf);
! LockBuffer(scan->xs_cbuf, BUFFER_LOCK_UNLOCK);
! ReleaseBuffer(scan->xs_cbuf);
scan->xs_cbuf = InvalidBuffer;
}

--- 541,547 ----

if (sv_infomask != heapTuple->t_data->t_infomask)
SetBufferCommitInfoNeedsSave(scan->xs_cbuf);
! UnlockAndReleaseBuffer(scan->xs_cbuf);
scan->xs_cbuf = InvalidBuffer;
}

Index: src/backend/access/nbtree/nbtinsert.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/access/nbtree/nbtinsert.c,v
retrieving revision 1.106
diff -c -r1.106 nbtinsert.c
*** src/backend/access/nbtree/nbtinsert.c	25 Sep 2003 06:57:57 -0000	1.106
--- src/backend/access/nbtree/nbtinsert.c	31 Oct 2003 23:00:35 -0000
***************
*** 274,281 ****
}
if (sv_infomask != htup.t_data->t_infomask)
SetBufferCommitInfoNeedsSave(hbuffer);
! 					LockBuffer(hbuffer, BUFFER_LOCK_UNLOCK);
! 					ReleaseBuffer(hbuffer);
}
}
}
--- 274,280 ----
}
if (sv_infomask != htup.t_data->t_infomask)
SetBufferCommitInfoNeedsSave(hbuffer);
! 					UnlockAndReleaseBuffer(hbuffer);
}
}
}
Index: src/backend/access/nbtree/nbtpage.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/access/nbtree/nbtpage.c,v
retrieving revision 1.72
diff -c -r1.72 nbtpage.c
*** src/backend/access/nbtree/nbtpage.c	29 Sep 2003 23:40:26 -0000	1.72
--- src/backend/access/nbtree/nbtpage.c	31 Oct 2003 23:01:05 -0000
***************
*** 502,509 ****
void
_bt_relbuf(Relation rel, Buffer buf)
{
! 	LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! 	ReleaseBuffer(buf);
}

/*
--- 502,508 ----
void
_bt_relbuf(Relation rel, Buffer buf)
{
! 	UnlockAndReleaseBuffer(buf);
}

/*
***************
*** 521,528 ****
void
_bt_wrtbuf(Relation rel, Buffer buf)
{
! LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! WriteBuffer(buf);
}

/*
--- 520,526 ----
void
_bt_wrtbuf(Relation rel, Buffer buf)
{
! 	UnlockAndWriteBuffer(buf);
}

/*
Index: src/backend/commands/sequence.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/commands/sequence.c,v
retrieving revision 1.103
diff -c -r1.103 sequence.c
*** src/backend/commands/sequence.c	25 Sep 2003 06:57:58 -0000	1.103
--- src/backend/commands/sequence.c	31 Oct 2003 23:02:15 -0000
***************
*** 291,298 ****

END_CRIT_SECTION();

! LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! WriteBuffer(buf);
heap_close(rel, NoLock);
}

--- 291,297 ----

END_CRIT_SECTION();

! UnlockAndWriteBuffer(buf);
heap_close(rel, NoLock);
}

***************
*** 379,388 ****

END_CRIT_SECTION();

! LockBuffer(buf, BUFFER_LOCK_UNLOCK);
!
! WriteBuffer(buf);
!
relation_close(seqrel, NoLock);
}

--- 378,384 ----

END_CRIT_SECTION();

! UnlockAndWriteBuffer(buf);
relation_close(seqrel, NoLock);
}

***************
*** 583,592 ****

END_CRIT_SECTION();

! LockBuffer(buf, BUFFER_LOCK_UNLOCK);
!
! WriteBuffer(buf);
!
relation_close(seqrel, NoLock);

PG_RETURN_INT64(result);
--- 579,585 ----

END_CRIT_SECTION();

! UnlockAndWriteBuffer(buf);
relation_close(seqrel, NoLock);

PG_RETURN_INT64(result);
***************
*** 719,728 ****

END_CRIT_SECTION();

! LockBuffer(buf, BUFFER_LOCK_UNLOCK);
!
! WriteBuffer(buf);
!
relation_close(seqrel, NoLock);
}

--- 712,718 ----

END_CRIT_SECTION();

! UnlockAndWriteBuffer(buf);
relation_close(seqrel, NoLock);
}

Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/commands/vacuum.c,v
retrieving revision 1.263
diff -c -r1.263 vacuum.c
*** src/backend/commands/vacuum.c	2 Oct 2003 23:19:44 -0000	1.263
--- src/backend/commands/vacuum.c	31 Oct 2003 23:04:31 -0000
***************
*** 2299,2306 ****
page = BufferGetPage(buf);
if (!PageIsEmpty(page))
vacuum_page(onerel, buf, *curpage);
! 			LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! 			WriteBuffer(buf);
}
}

--- 2299,2305 ----
page = BufferGetPage(buf);
if (!PageIsEmpty(page))
vacuum_page(onerel, buf, *curpage);
! 			UnlockAndWriteBuffer(buf);
}
}

***************
*** 2356,2363 ****
tuple.t_data->t_infomask |= HEAP_XMIN_INVALID;
}
}
! 		LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! 		WriteBuffer(buf);
Assert((*curpage)->offsets_used == num_tuples);
checked_moved += num_tuples;
}
--- 2355,2361 ----
tuple.t_data->t_infomask |= HEAP_XMIN_INVALID;
}
}
! 		UnlockAndWriteBuffer(buf);
Assert((*curpage)->offsets_used == num_tuples);
checked_moved += num_tuples;
}
***************
*** 2467,2474 ****

END_CRIT_SECTION();

! LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! WriteBuffer(buf);
}

/* now - free new list of reaped pages */
--- 2465,2471 ----

END_CRIT_SECTION();

! UnlockAndWriteBuffer(buf);
}

/* now - free new list of reaped pages */
***************
*** 2535,2542 ****
buf = ReadBuffer(onerel, (*vacpage)->blkno);
LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
vacuum_page(onerel, buf, *vacpage);
! LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! WriteBuffer(buf);
}
}

--- 2532,2538 ----
buf = ReadBuffer(onerel, (*vacpage)->blkno);
LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
vacuum_page(onerel, buf, *vacpage);
! 			UnlockAndWriteBuffer(buf);
}
}

Index: src/backend/commands/vacuumlazy.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/commands/vacuumlazy.c,v
retrieving revision 1.32
diff -c -r1.32 vacuumlazy.c
*** src/backend/commands/vacuumlazy.c	25 Sep 2003 06:57:59 -0000	1.32
--- src/backend/commands/vacuumlazy.c	31 Oct 2003 23:03:10 -0000
***************
*** 267,274 ****
lazy_record_free_space(vacrelstats, blkno,
PageGetFreeSpace(page));
}
! 			LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! 			WriteBuffer(buf);
continue;
}

--- 267,273 ----
lazy_record_free_space(vacrelstats, blkno,
PageGetFreeSpace(page));
}
! 			UnlockAndWriteBuffer(buf);
continue;
}

***************
*** 277,284 ****
empty_pages++;
lazy_record_free_space(vacrelstats, blkno,
PageGetFreeSpace(page));
! LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! ReleaseBuffer(buf);
continue;
}

--- 276,282 ----
empty_pages++;
lazy_record_free_space(vacrelstats, blkno,
PageGetFreeSpace(page));
! 			UnlockAndReleaseBuffer(buf);
continue;
}

***************
*** 477,484 ****
page = BufferGetPage(buf);
lazy_record_free_space(vacrelstats, tblk,
PageGetFreeSpace(page));
! LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! WriteBuffer(buf);
npages++;
}

--- 475,481 ----
page = BufferGetPage(buf);
lazy_record_free_space(vacrelstats, tblk,
PageGetFreeSpace(page));
! 		UnlockAndWriteBuffer(buf);
npages++;
}

***************
*** 812,819 ****
if (PageIsNew(page) || PageIsEmpty(page))
{
/* PageIsNew robably shouldn't happen... */
! LockBuffer(buf, BUFFER_LOCK_UNLOCK);
! ReleaseBuffer(buf);
continue;
}

--- 809,815 ----
if (PageIsNew(page) || PageIsEmpty(page))
{
/* PageIsNew robably shouldn't happen... */
! 			UnlockAndReleaseBuffer(buf);
continue;
}

Index: src/backend/storage/buffer/buf_init.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/storage/buffer/buf_init.c,v
retrieving revision 1.54
diff -c -r1.54 buf_init.c
*** src/backend/storage/buffer/buf_init.c	4 Aug 2003 02:40:03 -0000	1.54
--- src/backend/storage/buffer/buf_init.c	31 Oct 2003 22:38:16 -0000
***************
*** 34,50 ****
#include "utils/hsearch.h"
#include "utils/memutils.h"

-
- /*
- * if BMTRACE is defined, we trace the last 200 buffer allocations and
- * deallocations in a circular buffer in shared memory.
- */
- #ifdef BMTRACE
- bmtrace *TraceBuf;
- long *CurTraceBuf;
-
- #define BMT_LIMIT 200
- #endif /* BMTRACE */
int ShowPinTrace = 0;

int			Data_Descriptors;
--- 34,39 ----
***************
*** 144,159 ****
*/
LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);

- #ifdef BMTRACE
- 	CurTraceBuf = (long *) ShmemInitStruct("Buffer trace",
- 							(BMT_LIMIT * sizeof(bmtrace)) + sizeof(long),
- 										   &foundDescs);
- 	if (!foundDescs)
- 		MemSet(CurTraceBuf, 0, (BMT_LIMIT * sizeof(bmtrace)) + sizeof(long));
- 
- 	TraceBuf = (bmtrace *) & (CurTraceBuf[1]);
- #endif
- 
BufferDescriptors = (BufferDesc *)
ShmemInitStruct("Buffer Descriptors",
Num_Descriptors * sizeof(BufferDesc), &foundDescs);
--- 133,138 ----
***************
*** 266,274 ****
/* size of buffer hash table */
size += hash_estimate_size(NBuffers, sizeof(BufferLookupEnt));

- #ifdef BMTRACE
- 	size += (BMT_LIMIT * sizeof(bmtrace)) + sizeof(long);
- #endif
- 
return size;
}
--- 245,249 ----
Index: src/backend/storage/buffer/buf_table.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/storage/buffer/buf_table.c,v
retrieving revision 1.29
diff -c -r1.29 buf_table.c
*** src/backend/storage/buffer/buf_table.c	4 Aug 2003 02:40:03 -0000	1.29
--- src/backend/storage/buffer/buf_table.c	31 Oct 2003 22:30:34 -0000
***************
*** 133,147 ****
result->id = buf->buf_id;
return TRUE;
}
- 
- /* prints out collision stats for the buf table */
- #ifdef NOT_USED
- void
- DBG_LookupListCheck(int nlookup)
- {
- 	nlookup = 10;
- 
- 	hash_stats("Shared", SharedBufHash);
- }
- 
- #endif
--- 133,135 ----
Index: src/backend/storage/buffer/bufmgr.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/storage/buffer/bufmgr.c,v
retrieving revision 1.141
diff -c -r1.141 bufmgr.c
*** src/backend/storage/buffer/bufmgr.c	25 Sep 2003 06:58:01 -0000	1.141
--- src/backend/storage/buffer/bufmgr.c	31 Oct 2003 22:46:35 -0000
***************
*** 105,117 ****
*
* Note: a side effect of a P_NEW call is to update reln->rd_nblocks.
*/
- 
- #undef ReadBuffer				/* conflicts with macro when BUFMGR_DEBUG
- 								 * defined */
- 
- /*
-  * ReadBuffer
-  */
Buffer
ReadBuffer(Relation reln, BlockNumber blockNum)
{
--- 105,110 ----
***************
*** 358,366 ****
*/
*foundPtr = FALSE;
}
- #ifdef BMTRACE
- 		_bm_trace((reln->rd_rel->relisshared ? 0 : MyDatabaseId), RelationGetRelid(reln), blockNum, BufferDescriptorGetBuffer(buf), BMT_ALLOCFND);
- #endif   /* BMTRACE */

if (!(*foundPtr))
StartBufferIO(buf, true);
--- 351,356 ----
***************
*** 569,578 ****
else
ContinueBufferIO(buf, true);

- #ifdef BMTRACE
- _bm_trace((reln->rd_rel->relisshared ? 0 : MyDatabaseId), RelationGetRelid(reln), blockNum, BufferDescriptorGetBuffer(buf), BMT_ALLOCNOTFND);
- #endif /* BMTRACE */
-
LWLockRelease(BufMgrLock);

return buf;
--- 559,564 ----
***************
*** 619,627 ****
* Side Effects:
*		Pin count is decremented.
*/
- 
- #undef WriteBuffer
- 
void
WriteBuffer(Buffer buffer)
{
--- 605,610 ----
***************
*** 638,645 ****
write_buffer(buffer, false);
}

- 
- #undef ReleaseAndReadBuffer
/*
* ReleaseAndReadBuffer -- combine ReleaseBuffer() and ReadBuffer()
*		to save a lock release/acquire.
--- 621,626 ----
***************
*** 1099,1105 ****
* new or temp, because no one else should be modifying it.  Otherwise
* we need to ask the smgr for the current physical file length.
*
! 	 * Don't call smgr on a view, either.
*/
if (relation->rd_rel->relkind == RELKIND_VIEW)
relation->rd_nblocks = 0;
--- 1080,1086 ----
* new or temp, because no one else should be modifying it.  Otherwise
* we need to ask the smgr for the current physical file length.
*
! 	 * Don't call smgr on a view or a composite type, either.
*/
if (relation->rd_rel->relkind == RELKIND_VIEW)
relation->rd_nblocks = 0;
***************
*** 1107,1112 ****
--- 1088,1094 ----
relation->rd_nblocks = 0;
else if (!relation->rd_isnew && !relation->rd_istemp)
relation->rd_nblocks = smgrnblocks(DEFAULT_SMGR, relation);
+ 
return relation->rd_nblocks;
}

***************
*** 1555,1562 ****
return 0;
}

- #undef ReleaseBuffer
- 
/*
* ReleaseBuffer -- remove the pin on a buffer without
*		marking it dirty.
--- 1537,1542 ----
***************
*** 1669,1819 ****
}
#endif

- #ifdef BMTRACE
- 
- /*
-  *	trace allocations and deallocations in a circular buffer in
-  *	shared memory.	check the buffer before doing the allocation,
-  *	and die if there's anything fishy.
-  */
- 
- void
- _bm_trace(Oid dbId, Oid relId, int blkNo, int bufNo, int allocType)
- {
- 	long		start,
- 				cur;
- 	bmtrace    *tb;
- 
- 	start = *CurTraceBuf;
- 
- 	if (start > 0)
- 		cur = start - 1;
- 	else
- 		cur = BMT_LIMIT - 1;
- 
- 	for (;;)
- 	{
- 		tb = &TraceBuf[cur];
- 		if (tb->bmt_op != BMT_NOTUSED)
- 		{
- 			if (tb->bmt_buf == bufNo)
- 			{
- 				if ((tb->bmt_op == BMT_DEALLOC)
- 					|| (tb->bmt_dbid == dbId && tb->bmt_relid == relId
- 						&& tb->bmt_blkno == blkNo))
- 					goto okay;
- 
- 				/* die holding the buffer lock */
- 				_bm_die(dbId, relId, blkNo, bufNo, allocType, start, cur);
- 			}
- 		}
- 
- 		if (cur == start)
- 			goto okay;
- 
- 		if (cur == 0)
- 			cur = BMT_LIMIT - 1;
- 		else
- 			cur--;
- 	}
- 
- okay:
- 	tb = &TraceBuf[start];
- 	tb->bmt_pid = MyProcPid;
- 	tb->bmt_buf = bufNo;
- 	tb->bmt_dbid = dbId;
- 	tb->bmt_relid = relId;
- 	tb->bmt_blkno = blkNo;
- 	tb->bmt_op = allocType;
- 
- 	*CurTraceBuf = (start + 1) % BMT_LIMIT;
- }
- 
- void
- _bm_die(Oid dbId, Oid relId, int blkNo, int bufNo,
- 		int allocType, long start, long cur)
- {
- 	FILE	   *fp;
- 	bmtrace    *tb;
- 	int			i;
- 
- 	tb = &TraceBuf[cur];
- 
- 	if ((fp = AllocateFile("/tmp/death_notice", "w")) == NULL)
- 		elog(FATAL, "buffer alloc trace error and can't open log file");
- 
- 	fprintf(fp, "buffer alloc trace detected the following error:\n\n");
- 	fprintf(fp, "    buffer %d being %s inconsistently with a previous %s\n\n",
- 		 bufNo, (allocType == BMT_DEALLOC ? "deallocated" : "allocated"),
- 			(tb->bmt_op == BMT_DEALLOC ? "deallocation" : "allocation"));
- 
- 	fprintf(fp, "the trace buffer contains:\n");
- 
- 	i = start;
- 	for (;;)
- 	{
- 		tb = &TraceBuf[i];
- 		if (tb->bmt_op != BMT_NOTUSED)
- 		{
- 			fprintf(fp, "     [%3d]%spid %d buf %2d for <%u,%u,%u> ",
- 					i, (i == cur ? " ---> " : "\t"),
- 					tb->bmt_pid, tb->bmt_buf,
- 					tb->bmt_dbid, tb->bmt_relid, tb->bmt_blkno);
- 
- 			switch (tb->bmt_op)
- 			{
- 				case BMT_ALLOCFND:
- 					fprintf(fp, "allocate (found)\n");
- 					break;
- 
- 				case BMT_ALLOCNOTFND:
- 					fprintf(fp, "allocate (not found)\n");
- 					break;
- 
- 				case BMT_DEALLOC:
- 					fprintf(fp, "deallocate\n");
- 					break;
- 
- 				default:
- 					fprintf(fp, "unknown op type %d\n", tb->bmt_op);
- 					break;
- 			}
- 		}
- 
- 		i = (i + 1) % BMT_LIMIT;
- 		if (i == start)
- 			break;
- 	}
- 
- 	fprintf(fp, "\noperation causing error:\n");
- 	fprintf(fp, "\tpid %d buf %d for <%d,%u,%d> ",
- 			getpid(), bufNo, dbId, relId, blkNo);
- 
- 	switch (allocType)
- 	{
- 		case BMT_ALLOCFND:
- 			fprintf(fp, "allocate (found)\n");
- 			break;
- 
- 		case BMT_ALLOCNOTFND:
- 			fprintf(fp, "allocate (not found)\n");
- 			break;
- 
- 		case BMT_DEALLOC:
- 			fprintf(fp, "deallocate\n");
- 			break;
- 
- 		default:
- 			fprintf(fp, "unknown op type %d\n", allocType);
- 			break;
- 	}
- 
- 	FreeFile(fp);
- 
- 	kill(getpid(), SIGILL);
- }
- #endif   /* BMTRACE */
- 
/*
* SetBufferCommitInfoNeedsSave
*
--- 1649,1654 ----
Index: src/backend/storage/buffer/freelist.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/storage/buffer/freelist.c,v
retrieving revision 1.31
diff -c -r1.31 freelist.c
*** src/backend/storage/buffer/freelist.c	4 Aug 2003 02:40:03 -0000	1.31
--- src/backend/storage/buffer/freelist.c	31 Oct 2003 22:50:11 -0000
***************
*** 32,40 ****
#include "storage/ipc.h"
#include "storage/proc.h"

-
static BufferDesc *SharedFreeList;

/*
* State-checking macros
*/
--- 32,41 ----
#include "storage/ipc.h"
#include "storage/proc.h"

static BufferDesc *SharedFreeList;

+ #define INVALID_DESCRIPTOR (-3)
+ 
/*
* State-checking macros
*/
***************
*** 65,74 ****
static void
AddBufferToFreelist(BufferDesc *bf)
{
- #ifdef BMTRACE
- 	_bm_trace(bf->tag.relId.dbId, bf->tag.relId.relId, bf->tag.blockNum,
- 			  BufferDescriptorGetBuffer(bf), BMT_DEALLOC);
- #endif   /* BMTRACE */
IsNotInQueue(bf);

/* change bf so it points to inFrontOfNew and its successor */
--- 66,71 ----
Index: src/include/storage/buf_internals.h
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/include/storage/buf_internals.h,v
retrieving revision 1.61
diff -c -r1.61 buf_internals.h
*** src/include/storage/buf_internals.h	4 Aug 2003 02:40:14 -0000	1.61
--- src/include/storage/buf_internals.h	31 Oct 2003 22:37:19 -0000
***************
*** 33,46 ****
* Flags for buffer descriptors
*/
#define BM_DIRTY				(1 << 0)
! #define BM_PRIVATE				(1 << 1)
! #define BM_VALID				(1 << 2)
! #define BM_DELETED				(1 << 3)
! #define BM_FREE					(1 << 4)
! #define BM_IO_IN_PROGRESS		(1 << 5)
! #define BM_IO_ERROR				(1 << 6)
! #define BM_JUST_DIRTIED			(1 << 7)
! #define BM_PIN_COUNT_WAITER		(1 << 8)

typedef bits16 BufFlags;

--- 33,45 ----
* Flags for buffer descriptors
*/
#define BM_DIRTY				(1 << 0)
! #define BM_VALID				(1 << 1)
! #define BM_DELETED				(1 << 2)
! #define BM_FREE					(1 << 3)
! #define BM_IO_IN_PROGRESS		(1 << 4)
! #define BM_IO_ERROR				(1 << 5)
! #define BM_JUST_DIRTIED			(1 << 6)
! #define BM_PIN_COUNT_WAITER		(1 << 7)

typedef bits16 BufFlags;

***************
*** 123,154 ****
Buffer id;
} BufferLookupEnt;

- /*
-  *	mao tracing buffer allocation
-  */
- 
- /*#define BMTRACE*/
- 
- #ifdef BMTRACE
- 
- typedef struct _bmtrace
- {
- 	int			bmt_pid;
- 	int			bmt_buf;
- 	Oid			bmt_dbid;
- 	Oid			bmt_relid;
- 	BlockNumber bmt_blkno;
- 	int			bmt_op;
- 
- #define BMT_NOTUSED		0
- #define BMT_ALLOCFND	1
- #define BMT_ALLOCNOTFND 2
- #define BMT_DEALLOC		3
- 
- }	bmtrace;
- #endif   /* BMTRACE */
- 
- 
/* counters in buf_init.c */
extern long int ReadBufferCount;
extern long int ReadLocalBufferCount;
--- 122,127 ----
Index: src/include/storage/bufmgr.h
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/include/storage/bufmgr.h,v
retrieving revision 1.70
diff -c -r1.70 bufmgr.h
*** src/include/storage/bufmgr.h	10 Aug 2003 19:48:08 -0000	1.70
--- src/include/storage/bufmgr.h	31 Oct 2003 22:50:16 -0000
***************
*** 52,58 ****
*/

#define BAD_BUFFER_ID(bid) ((bid) < 1 || (bid) > NBuffers)
- #define INVALID_DESCRIPTOR (-3)

#define UnlockAndReleaseBuffer(buffer)	\
( \
--- 52,57 ----

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

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #

Jan Wieck <JanWieck@Yahoo.com> writes:

Neil Conway wrote:

This patch cleans up some of the bufmgr code:

Can this be held off a little while we're experimenting with
improvements to the buffer algorithms?

I do not actually agree with the "UnlockAndReleaseBuffer" changes
anyway. I think this obscures the code by making resource grabbing
and resource releasing code unsymmetrical, not to mention incompatible
with code branches where the unlock and the buffer release can't be
merged because other things are done in between.

As for removing the BM_TRACE code, what's broken about it? Shouldn't we
be more interested in fixing it than removing it?

regards, tom lane

On Mon, 2003-11-03 at 10:00, Tom Lane wrote:

I do not actually agree with the "UnlockAndReleaseBuffer" changes
anyway. I think this obscures the code by making resource grabbing
and resource releasing code unsymmetrical

Hmm... fair enough, I see your point. In that case, should I remove the
UnlockAndReleaseBuffer macro and change all the places that use it to
just do a LockBuffer() followed by ReleaseBuffer()?

(Similarly for UnlockAndWriteBuffer())

not to mention incompatible
with code branches where the unlock and the buffer release can't be
merged because other things are done in between.

This makes no sense to me: the macro is inapplicable in this case, but I
don't see how this makes anything "incompatible". Can you elaborate?

As for removing the BM_TRACE code, what's broken about it? Shouldn't we
be more interested in fixing it than removing it?

Well, it doesn't compile, and probably hasn't for years, so I took that
as a sign that there wasn't much interest in it. We need proper buffer
tracing, but if anyone wants to implement that, the first thing they'd
need to do is rip out the existing, broken BMTRACE code. In the unlikely
event that someone would like to resurrect it or use it for reference,
that's what CVS is for...

-Neil

Neil Conway <neilc@samurai.com> writes:

On Mon, 2003-11-03 at 10:00, Tom Lane wrote:

I do not actually agree with the "UnlockAndReleaseBuffer" changes
anyway. I think this obscures the code by making resource grabbing
and resource releasing code unsymmetrical

Hmm... fair enough, I see your point. In that case, should I remove the
UnlockAndReleaseBuffer macro and change all the places that use it to
just do a LockBuffer() followed by ReleaseBuffer()?

Didn't realize we had one, actually. Wonder when it was put in? I
agree with removing it --- seems like it obscures the code to little
purpose.

not to mention incompatible
with code branches where the unlock and the buffer release can't be
merged because other things are done in between.

This makes no sense to me: the macro is inapplicable in this case, but I
don't see how this makes anything "incompatible". Can you elaborate?

Incompatible in the sense that it looks different. If you see in one
place

foo = ReadBuffer(...);
do some stuff;
LockBuffer(foo, LOCK);
do some stuff;
LockBuffer(foo, UNLOCK);
do some stuff;
ReleaseBuffer(foo);

then it's reasonably apparent that the LockBuffers pair together and so
do ReadBuffer/ReleaseBuffer. If other places replace this pattern by

foo = ReadBuffer(...);
do some stuff;
LockBuffer(foo, LOCK);
do some stuff;
UnLockAndReleaseBuffer(foo);

it looks visually different and the pairing of operations is lost.
ISTM this adds potential for confusion without really buying anything.

One could argue that LockBuffer(foo, UNLOCK) ought to be written
UnLockBuffer(foo) for symmetry with ReleaseBuffer. I'm not sure if
there are any places that really depend on the way it's coded now...

As for removing the BM_TRACE code, what's broken about it?

Well, it doesn't compile, and probably hasn't for years, so I took that
as a sign that there wasn't much interest in it.

Ah. Fair point I guess ...

regards, tom lane