Thread: WriteBuffer return value
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.
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-11 17:42:26.000000000 +0200
@@ -580,7 +580,7 @@
return WriteLocalBuffer(buffer, TRUE);
if (BAD_BUFFER_ID(buffer))
- return FALSE;
+ return STATUS_ERROR;
bufHdr = &BufferDescriptors[buffer - 1];
@@ -592,7 +592,7 @@
UnpinBuffer(bufHdr);
LWLockRelease(BufMgrLock);
- return TRUE;
+ return STATUS_OK;
}
/*
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