WriteBuffer return value

Manfred Koizar <mkoi-pg@aon.at> writes:

Fix WriteBuffer() to return STATUS_OK/STATUS_ERROR instead of
TRUE/FALSE. The return value is used by nextval() and do_setval()
in sequence.c, all other callers ignore the return value.

Given the lack of any error checks in 99% of the callers, I do not think
this is an appropriate change. I'd vote for changing WriteBuffer to
return void, and have it elog() on bad argument. No one should ever
pass it a bogus buffer ID anyway --- if you don't have a valid buffer
ID, then what the heck were you just scribbling on?

regards, tom lane

On Wed, 12 Jun 2002 10:19:17 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'd vote for changing WriteBuffer to
return void, and have it elog() on bad argument.

Whatever you want ...
BTW, to avoid rejected hunks this patch should be applied after the
EliminateUnused patch I posted 2002-06-10.

diff -ur ../orig/src/backend/commands/sequence.c src/backend/commands/sequence.c
--- ../orig/src/backend/commands/sequence.c	2002-06-10 15:40:46.000000000 +0200
+++ src/backend/commands/sequence.c	2002-06-13 09:32:25.000000000 +0200
@@ -468,8 +468,7 @@

LockBuffer(buf, BUFFER_LOCK_UNLOCK);

-	if (WriteBuffer(buf) == STATUS_ERROR)
-		elog(ERROR, "%s.nextval: WriteBuffer failed", sequence->relname);
+	WriteBuffer(buf);

relation_close(seqrel, NoLock);

@@ -581,8 +580,7 @@

LockBuffer(buf, BUFFER_LOCK_UNLOCK);

-	if (WriteBuffer(buf) == STATUS_ERROR)
-		elog(ERROR, "%s.setval: WriteBuffer failed", sequence->relname);
+	WriteBuffer(buf);

 	relation_close(seqrel, NoLock);
 }
diff -ur ../orig/src/backend/storage/buffer/bufmgr.c src/backend/storage/buffer/bufmgr.c
--- ../orig/src/backend/storage/buffer/bufmgr.c	2002-06-10 15:00:57.000000000 +0200
+++ src/backend/storage/buffer/bufmgr.c	2002-06-13 09:32:40.000000000 +0200
@@ -87,6 +87,7 @@
 static int	BufferReplace(BufferDesc *bufHdr);
 void		PrintBufferDescs(void);

+static void write_buffer(Buffer buffer, bool unpin);

/*
* ReadBuffer -- returns a buffer containing the requested
@@ -558,29 +559,22 @@
}

 /*
- * WriteBuffer
- *
- *		Marks buffer contents as dirty (actual write happens later).
- *
- * Assume that buffer is pinned.  Assume that reln is
- *		valid.
- *
- * Side Effects:
- *		Pin count is decremented.
+ * write_buffer -- common functionality for
+ *                 WriteBuffer and WriteNoReleaseBuffer
  */
-
-#undef WriteBuffer
-
-int
-WriteBuffer(Buffer buffer)
+static void
+write_buffer(Buffer buffer, bool release)
 {
 	BufferDesc *bufHdr;

 	if (BufferIsLocal(buffer))
-		return WriteLocalBuffer(buffer, TRUE);
+	{
+		WriteLocalBuffer(buffer, release);
+		return;
+	}

 	if (BAD_BUFFER_ID(buffer))
-		return FALSE;
+		elog(ERROR, "write_buffer: bad buffer %d", buffer);

bufHdr = &BufferDescriptors[buffer - 1];

@@ -589,37 +583,39 @@

bufHdr->flags |= (BM_DIRTY | BM_JUST_DIRTIED);

-	UnpinBuffer(bufHdr);
+	if (release)
+		UnpinBuffer(bufHdr);
 	LWLockRelease(BufMgrLock);
+}

-	return TRUE;
+/*
+ * WriteBuffer
+ *
+ *		Marks buffer contents as dirty (actual write happens later).
+ *
+ * Assume that buffer is pinned.  Assume that reln is
+ *		valid.
+ *
+ * Side Effects:
+ *		Pin count is decremented.
+ */
+
+#undef WriteBuffer
+
+void
+WriteBuffer(Buffer buffer)
+{
+	write_buffer(buffer, true);
 }

 /*
  * WriteNoReleaseBuffer -- like WriteBuffer, but do not unpin the buffer
  *						   when the operation is complete.
  */
-int
+void
 WriteNoReleaseBuffer(Buffer buffer)
 {
-	BufferDesc *bufHdr;
-
-	if (BufferIsLocal(buffer))
-		return WriteLocalBuffer(buffer, FALSE);
-
-	if (BAD_BUFFER_ID(buffer))
-		return STATUS_ERROR;
-
-	bufHdr = &BufferDescriptors[buffer - 1];
-
-	LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
-	Assert(bufHdr->refcount > 0);
-
-	bufHdr->flags |= (BM_DIRTY | BM_JUST_DIRTIED);
-
-	LWLockRelease(BufMgrLock);
-
-	return STATUS_OK;
+	write_buffer(buffer, false);
 }

diff -ur ../orig/src/backend/storage/buffer/localbuf.c src/backend/storage/buffer/localbuf.c
--- ../orig/src/backend/storage/buffer/localbuf.c	2002-05-03 19:42:11.000000000 +0200
+++ src/backend/storage/buffer/localbuf.c	2002-06-13 09:23:56.000000000 +0200
@@ -155,7 +155,7 @@
  * WriteLocalBuffer -
  *	  writes out a local buffer
  */
-int
+void
 WriteLocalBuffer(Buffer buffer, bool release)
 {
 	int			bufid;
@@ -174,8 +174,6 @@
 		Assert(LocalRefCount[bufid] > 0);
 		LocalRefCount[bufid]--;
 	}
-
-	return true;
 }

 /*
diff -ur ../orig/src/include/storage/buf_internals.h src/include/storage/buf_internals.h
--- ../orig/src/include/storage/buf_internals.h	2002-06-10 15:03:44.000000000 +0200
+++ src/include/storage/buf_internals.h	2002-06-13 09:23:26.000000000 +0200
@@ -176,7 +176,7 @@

 extern BufferDesc *LocalBufferAlloc(Relation reln, BlockNumber blockNum,
 				 bool *foundPtr);
-extern int	WriteLocalBuffer(Buffer buffer, bool release);
+extern void	WriteLocalBuffer(Buffer buffer, bool release);
 extern int	FlushLocalBuffer(Buffer buffer, bool sync, bool release);
 extern void LocalBufferSync(void);
 extern void ResetLocalBufferPool(void);
diff -ur ../orig/src/include/storage/bufmgr.h src/include/storage/bufmgr.h
--- ../orig/src/include/storage/bufmgr.h	2002-04-16 01:47:12.000000000 +0200
+++ src/include/storage/bufmgr.h	2002-06-13 09:22:59.000000000 +0200
@@ -148,8 +148,8 @@
  */
 extern Buffer ReadBuffer(Relation reln, BlockNumber blockNum);
 extern int	ReleaseBuffer(Buffer buffer);
-extern int	WriteBuffer(Buffer buffer);
-extern int	WriteNoReleaseBuffer(Buffer buffer);
+extern void	WriteBuffer(Buffer buffer);
+extern void	WriteNoReleaseBuffer(Buffer buffer);
 extern Buffer ReleaseAndReadBuffer(Buffer buffer, Relation relation,
 					 BlockNumber blockNum);
 extern int	FlushBuffer(Buffer buffer, bool sync, bool release);

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:

On Wed, 12 Jun 2002 10:19:17 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'd vote for changing WriteBuffer to
return void, and have it elog() on bad argument.

Whatever you want ...
BTW, to avoid rejected hunks this patch should be applied after the
EliminateUnused patch I posted 2002-06-10.

diff -ur ../orig/src/backend/commands/sequence.c src/backend/commands/sequence.c
--- ../orig/src/backend/commands/sequence.c	2002-06-10 15:40:46.000000000 +0200
+++ src/backend/commands/sequence.c	2002-06-13 09:32:25.000000000 +0200
@@ -468,8 +468,7 @@

LockBuffer(buf, BUFFER_LOCK_UNLOCK);

-	if (WriteBuffer(buf) == STATUS_ERROR)
-		elog(ERROR, "%s.nextval: WriteBuffer failed", sequence->relname);
+	WriteBuffer(buf);

relation_close(seqrel, NoLock);

@@ -581,8 +580,7 @@

LockBuffer(buf, BUFFER_LOCK_UNLOCK);

-	if (WriteBuffer(buf) == STATUS_ERROR)
-		elog(ERROR, "%s.setval: WriteBuffer failed", sequence->relname);
+	WriteBuffer(buf);

relation_close(seqrel, NoLock);
}
diff -ur ../orig/src/backend/storage/buffer/bufmgr.c src/backend/storage/buffer/bufmgr.c
--- ../orig/src/backend/storage/buffer/bufmgr.c	2002-06-10 15:00:57.000000000 +0200
+++ src/backend/storage/buffer/bufmgr.c	2002-06-13 09:32:40.000000000 +0200
@@ -87,6 +87,7 @@
static int	BufferReplace(BufferDesc *bufHdr);
void		PrintBufferDescs(void);

+static void write_buffer(Buffer buffer, bool unpin);

/*
* ReadBuffer -- returns a buffer containing the requested
@@ -558,29 +559,22 @@
}

/*
- * WriteBuffer
- *
- *		Marks buffer contents as dirty (actual write happens later).
- *
- * Assume that buffer is pinned.  Assume that reln is
- *		valid.
- *
- * Side Effects:
- *		Pin count is decremented.
+ * write_buffer -- common functionality for
+ *                 WriteBuffer and WriteNoReleaseBuffer
*/
-
-#undef WriteBuffer
-
-int
-WriteBuffer(Buffer buffer)
+static void
+write_buffer(Buffer buffer, bool release)
{
BufferDesc *bufHdr;

if (BufferIsLocal(buffer))
-		return WriteLocalBuffer(buffer, TRUE);
+	{
+		WriteLocalBuffer(buffer, release);
+		return;
+	}

if (BAD_BUFFER_ID(buffer))
-		return FALSE;
+		elog(ERROR, "write_buffer: bad buffer %d", buffer);

bufHdr = &BufferDescriptors[buffer - 1];

@@ -589,37 +583,39 @@

bufHdr->flags |= (BM_DIRTY | BM_JUST_DIRTIED);

-	UnpinBuffer(bufHdr);
+	if (release)
+		UnpinBuffer(bufHdr);
LWLockRelease(BufMgrLock);
+}

-	return TRUE;
+/*
+ * WriteBuffer
+ *
+ *		Marks buffer contents as dirty (actual write happens later).
+ *
+ * Assume that buffer is pinned.  Assume that reln is
+ *		valid.
+ *
+ * Side Effects:
+ *		Pin count is decremented.
+ */
+
+#undef WriteBuffer
+
+void
+WriteBuffer(Buffer buffer)
+{
+	write_buffer(buffer, true);
}

/*
* WriteNoReleaseBuffer -- like WriteBuffer, but do not unpin the buffer
*						   when the operation is complete.
*/
-int
+void
WriteNoReleaseBuffer(Buffer buffer)
{
-	BufferDesc *bufHdr;
-
-	if (BufferIsLocal(buffer))
-		return WriteLocalBuffer(buffer, FALSE);
-
-	if (BAD_BUFFER_ID(buffer))
-		return STATUS_ERROR;
-
-	bufHdr = &BufferDescriptors[buffer - 1];
-
-	LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
-	Assert(bufHdr->refcount > 0);
-
-	bufHdr->flags |= (BM_DIRTY | BM_JUST_DIRTIED);
-
-	LWLockRelease(BufMgrLock);
-
-	return STATUS_OK;
+	write_buffer(buffer, false);
}

diff -ur ../orig/src/backend/storage/buffer/localbuf.c src/backend/storage/buffer/localbuf.c
--- ../orig/src/backend/storage/buffer/localbuf.c	2002-05-03 19:42:11.000000000 +0200
+++ src/backend/storage/buffer/localbuf.c	2002-06-13 09:23:56.000000000 +0200
@@ -155,7 +155,7 @@
* WriteLocalBuffer -
*	  writes out a local buffer
*/
-int
+void
WriteLocalBuffer(Buffer buffer, bool release)
{
int			bufid;
@@ -174,8 +174,6 @@
Assert(LocalRefCount[bufid] > 0);
LocalRefCount[bufid]--;
}
-
-	return true;
}

/*
diff -ur ../orig/src/include/storage/buf_internals.h src/include/storage/buf_internals.h
--- ../orig/src/include/storage/buf_internals.h	2002-06-10 15:03:44.000000000 +0200
+++ src/include/storage/buf_internals.h	2002-06-13 09:23:26.000000000 +0200
@@ -176,7 +176,7 @@

extern BufferDesc *LocalBufferAlloc(Relation reln, BlockNumber blockNum,
bool *foundPtr);
-extern int	WriteLocalBuffer(Buffer buffer, bool release);
+extern void	WriteLocalBuffer(Buffer buffer, bool release);
extern int	FlushLocalBuffer(Buffer buffer, bool sync, bool release);
extern void LocalBufferSync(void);
extern void ResetLocalBufferPool(void);
diff -ur ../orig/src/include/storage/bufmgr.h src/include/storage/bufmgr.h
--- ../orig/src/include/storage/bufmgr.h	2002-04-16 01:47:12.000000000 +0200
+++ src/include/storage/bufmgr.h	2002-06-13 09:22:59.000000000 +0200
@@ -148,8 +148,8 @@
*/
extern Buffer ReadBuffer(Relation reln, BlockNumber blockNum);
extern int	ReleaseBuffer(Buffer buffer);
-extern int	WriteBuffer(Buffer buffer);
-extern int	WriteNoReleaseBuffer(Buffer buffer);
+extern void	WriteBuffer(Buffer buffer);
+extern void	WriteNoReleaseBuffer(Buffer buffer);
extern Buffer ReleaseAndReadBuffer(Buffer buffer, Relation relation,
BlockNumber blockNum);
extern int	FlushBuffer(Buffer buffer, bool sync, bool release);

---------------------------(end of broadcast)---------------------------
TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org

-- 
  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:

On Wed, 12 Jun 2002 10:19:17 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'd vote for changing WriteBuffer to
return void, and have it elog() on bad argument.

Whatever you want ...
BTW, to avoid rejected hunks this patch should be applied after the
EliminateUnused patch I posted 2002-06-10.

diff -ur ../orig/src/backend/commands/sequence.c src/backend/commands/sequence.c
--- ../orig/src/backend/commands/sequence.c	2002-06-10 15:40:46.000000000 +0200
+++ src/backend/commands/sequence.c	2002-06-13 09:32:25.000000000 +0200
@@ -468,8 +468,7 @@

LockBuffer(buf, BUFFER_LOCK_UNLOCK);

-	if (WriteBuffer(buf) == STATUS_ERROR)
-		elog(ERROR, "%s.nextval: WriteBuffer failed", sequence->relname);
+	WriteBuffer(buf);

relation_close(seqrel, NoLock);

@@ -581,8 +580,7 @@

LockBuffer(buf, BUFFER_LOCK_UNLOCK);

-	if (WriteBuffer(buf) == STATUS_ERROR)
-		elog(ERROR, "%s.setval: WriteBuffer failed", sequence->relname);
+	WriteBuffer(buf);

relation_close(seqrel, NoLock);
}
diff -ur ../orig/src/backend/storage/buffer/bufmgr.c src/backend/storage/buffer/bufmgr.c
--- ../orig/src/backend/storage/buffer/bufmgr.c	2002-06-10 15:00:57.000000000 +0200
+++ src/backend/storage/buffer/bufmgr.c	2002-06-13 09:32:40.000000000 +0200
@@ -87,6 +87,7 @@
static int	BufferReplace(BufferDesc *bufHdr);
void		PrintBufferDescs(void);

+static void write_buffer(Buffer buffer, bool unpin);

/*
* ReadBuffer -- returns a buffer containing the requested
@@ -558,29 +559,22 @@
}

/*
- * WriteBuffer
- *
- *		Marks buffer contents as dirty (actual write happens later).
- *
- * Assume that buffer is pinned.  Assume that reln is
- *		valid.
- *
- * Side Effects:
- *		Pin count is decremented.
+ * write_buffer -- common functionality for
+ *                 WriteBuffer and WriteNoReleaseBuffer
*/
-
-#undef WriteBuffer
-
-int
-WriteBuffer(Buffer buffer)
+static void
+write_buffer(Buffer buffer, bool release)
{
BufferDesc *bufHdr;

if (BufferIsLocal(buffer))
-		return WriteLocalBuffer(buffer, TRUE);
+	{
+		WriteLocalBuffer(buffer, release);
+		return;
+	}

if (BAD_BUFFER_ID(buffer))
-		return FALSE;
+		elog(ERROR, "write_buffer: bad buffer %d", buffer);

bufHdr = &BufferDescriptors[buffer - 1];

@@ -589,37 +583,39 @@

bufHdr->flags |= (BM_DIRTY | BM_JUST_DIRTIED);

-	UnpinBuffer(bufHdr);
+	if (release)
+		UnpinBuffer(bufHdr);
LWLockRelease(BufMgrLock);
+}

-	return TRUE;
+/*
+ * WriteBuffer
+ *
+ *		Marks buffer contents as dirty (actual write happens later).
+ *
+ * Assume that buffer is pinned.  Assume that reln is
+ *		valid.
+ *
+ * Side Effects:
+ *		Pin count is decremented.
+ */
+
+#undef WriteBuffer
+
+void
+WriteBuffer(Buffer buffer)
+{
+	write_buffer(buffer, true);
}

/*
* WriteNoReleaseBuffer -- like WriteBuffer, but do not unpin the buffer
*						   when the operation is complete.
*/
-int
+void
WriteNoReleaseBuffer(Buffer buffer)
{
-	BufferDesc *bufHdr;
-
-	if (BufferIsLocal(buffer))
-		return WriteLocalBuffer(buffer, FALSE);
-
-	if (BAD_BUFFER_ID(buffer))
-		return STATUS_ERROR;
-
-	bufHdr = &BufferDescriptors[buffer - 1];
-
-	LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
-	Assert(bufHdr->refcount > 0);
-
-	bufHdr->flags |= (BM_DIRTY | BM_JUST_DIRTIED);
-
-	LWLockRelease(BufMgrLock);
-
-	return STATUS_OK;
+	write_buffer(buffer, false);
}

diff -ur ../orig/src/backend/storage/buffer/localbuf.c src/backend/storage/buffer/localbuf.c
--- ../orig/src/backend/storage/buffer/localbuf.c	2002-05-03 19:42:11.000000000 +0200
+++ src/backend/storage/buffer/localbuf.c	2002-06-13 09:23:56.000000000 +0200
@@ -155,7 +155,7 @@
* WriteLocalBuffer -
*	  writes out a local buffer
*/
-int
+void
WriteLocalBuffer(Buffer buffer, bool release)
{
int			bufid;
@@ -174,8 +174,6 @@
Assert(LocalRefCount[bufid] > 0);
LocalRefCount[bufid]--;
}
-
-	return true;
}

/*
diff -ur ../orig/src/include/storage/buf_internals.h src/include/storage/buf_internals.h
--- ../orig/src/include/storage/buf_internals.h	2002-06-10 15:03:44.000000000 +0200
+++ src/include/storage/buf_internals.h	2002-06-13 09:23:26.000000000 +0200
@@ -176,7 +176,7 @@

extern BufferDesc *LocalBufferAlloc(Relation reln, BlockNumber blockNum,
bool *foundPtr);
-extern int	WriteLocalBuffer(Buffer buffer, bool release);
+extern void	WriteLocalBuffer(Buffer buffer, bool release);
extern int	FlushLocalBuffer(Buffer buffer, bool sync, bool release);
extern void LocalBufferSync(void);
extern void ResetLocalBufferPool(void);
diff -ur ../orig/src/include/storage/bufmgr.h src/include/storage/bufmgr.h
--- ../orig/src/include/storage/bufmgr.h	2002-04-16 01:47:12.000000000 +0200
+++ src/include/storage/bufmgr.h	2002-06-13 09:22:59.000000000 +0200
@@ -148,8 +148,8 @@
*/
extern Buffer ReadBuffer(Relation reln, BlockNumber blockNum);
extern int	ReleaseBuffer(Buffer buffer);
-extern int	WriteBuffer(Buffer buffer);
-extern int	WriteNoReleaseBuffer(Buffer buffer);
+extern void	WriteBuffer(Buffer buffer);
+extern void	WriteNoReleaseBuffer(Buffer buffer);
extern Buffer ReleaseAndReadBuffer(Buffer buffer, Relation relation,
BlockNumber blockNum);
extern int	FlushBuffer(Buffer buffer, bool sync, bool release);

---------------------------(end of broadcast)---------------------------
TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org

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