Old BufferDesc refcount in PrintBufferDescs and PrintPinnedBufs
Hi,
In bufmgr.c we have the debugging functions PrintBufferDescs
and PrintPinnedBufs, which are typically hidden behind the flag
#ifdef NOT_USED. These functions reference the old buf->refcount and
buf->flags fields, and so they no longer compile. I attached a patch to
use BUF_STATE_GET_REFCOUNT instead and removed the reference to buf->flags.
Regards,
Jacob Brazeal
Attachments:
trial.patchapplication/octet-stream; name=trial.patchDownload
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 739daa1153..0892839b43 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4424,21 +4424,22 @@ void
PrintBufferDescs(void)
{
int i;
+ uint32 buf_state;
for (i = 0; i < NBuffers; ++i)
{
BufferDesc *buf = GetBufferDescriptor(i);
Buffer b = BufferDescriptorGetBuffer(buf);
- /* theoretically we should lock the bufhdr here */
+ buf_state = LockBufHdr(buf);
elog(LOG,
"[%02d] (freeNext=%d, rel=%s, "
- "blockNum=%u, flags=0x%x, refcount=%u %d)",
+ "blockNum=%u, refcount=%u %d)",
i, buf->freeNext,
relpathbackend(BufTagGetRelFileLocator(&buf->tag),
INVALID_PROC_NUMBER, BufTagGetForkNum(&buf->tag)),
- buf->tag.blockNum, buf->flags,
- buf->refcount, GetPrivateRefCount(b));
+ buf->tag.blockNum,
+ BUF_STATE_GET_REFCOUNT(buf_state), GetPrivateRefCount(b));
}
}
#endif
@@ -4448,6 +4449,7 @@ void
PrintPinnedBufs(void)
{
int i;
+ uint32 buf_state;
for (i = 0; i < NBuffers; ++i)
{
@@ -4456,15 +4458,15 @@ PrintPinnedBufs(void)
if (GetPrivateRefCount(b) > 0)
{
- /* theoretically we should lock the bufhdr here */
+ buf_state = LockBufHdr(buf);
elog(LOG,
"[%02d] (freeNext=%d, rel=%s, "
- "blockNum=%u, flags=0x%x, refcount=%u %d)",
+ "blockNum=%u, refcount=%u %d)",
i, buf->freeNext,
relpathperm(BufTagGetRelFileLocator(&buf->tag),
BufTagGetForkNum(&buf->tag)),
- buf->tag.blockNum, buf->flags,
- buf->refcount, GetPrivateRefCount(b));
+ buf->tag.blockNum,
+ BUF_STATE_GET_REFCOUNT(buf_state), GetPrivateRefCount(b));
}
}
}
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 3fdd29bd0b..9e1ae5682e 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -278,6 +278,7 @@ extern XLogRecPtr BufferGetLSNAtomic(Buffer buffer);
#ifdef NOT_USED
extern void PrintPinnedBufs(void);
+extern void PrintBufferDescs(void);
#endif
extern void BufferGetTag(Buffer buffer, RelFileLocator *rlocator,
ForkNumber *forknum, BlockNumber *blknum);
Since this patch takes out a lock to read the BufferDesc state, I also need
to release the lock, which I've added to this patch.
On Fri, Jan 17, 2025 at 2:11 PM Jacob Brazeal <jacob.brazeal@gmail.com>
wrote:
Show quoted text
Hi,
In bufmgr.c we have the debugging functions PrintBufferDescs
and PrintPinnedBufs, which are typically hidden behind the flag
#ifdef NOT_USED. These functions reference the old buf->refcount and
buf->flags fields, and so they no longer compile. I attached a patch to
use BUF_STATE_GET_REFCOUNT instead and removed the reference to buf->flags.Regards,
Jacob Brazeal
Attachments:
v2-buf-helper-compile-err.patchapplication/octet-stream; name=v2-buf-helper-compile-err.patchDownload
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 739daa1153..de4c617b0e 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4424,21 +4424,24 @@ void
PrintBufferDescs(void)
{
int i;
+ uint32 buf_state;
for (i = 0; i < NBuffers; ++i)
{
BufferDesc *buf = GetBufferDescriptor(i);
Buffer b = BufferDescriptorGetBuffer(buf);
- /* theoretically we should lock the bufhdr here */
+ buf_state = LockBufHdr(buf);
elog(LOG,
"[%02d] (freeNext=%d, rel=%s, "
- "blockNum=%u, flags=0x%x, refcount=%u %d)",
+ "blockNum=%u, refcount=%u %d)",
i, buf->freeNext,
relpathbackend(BufTagGetRelFileLocator(&buf->tag),
INVALID_PROC_NUMBER, BufTagGetForkNum(&buf->tag)),
- buf->tag.blockNum, buf->flags,
- buf->refcount, GetPrivateRefCount(b));
+ buf->tag.blockNum,
+ BUF_STATE_GET_REFCOUNT(buf_state), GetPrivateRefCount(b));
+
+ UnlockBufHdr(buf, buf_state);
}
}
#endif
@@ -4448,6 +4451,7 @@ void
PrintPinnedBufs(void)
{
int i;
+ uint32 buf_state;
for (i = 0; i < NBuffers; ++i)
{
@@ -4456,15 +4460,17 @@ PrintPinnedBufs(void)
if (GetPrivateRefCount(b) > 0)
{
- /* theoretically we should lock the bufhdr here */
+ buf_state = LockBufHdr(buf);
elog(LOG,
"[%02d] (freeNext=%d, rel=%s, "
- "blockNum=%u, flags=0x%x, refcount=%u %d)",
+ "blockNum=%u, refcount=%u %d)",
i, buf->freeNext,
relpathperm(BufTagGetRelFileLocator(&buf->tag),
BufTagGetForkNum(&buf->tag)),
- buf->tag.blockNum, buf->flags,
- buf->refcount, GetPrivateRefCount(b));
+ buf->tag.blockNum,
+ BUF_STATE_GET_REFCOUNT(buf_state), GetPrivateRefCount(b));
+
+ UnlockBufHdr(buf, buf_state);
}
}
}
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 3fdd29bd0b..9e1ae5682e 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -278,6 +278,7 @@ extern XLogRecPtr BufferGetLSNAtomic(Buffer buffer);
#ifdef NOT_USED
extern void PrintPinnedBufs(void);
+extern void PrintBufferDescs(void);
#endif
extern void BufferGetTag(Buffer buffer, RelFileLocator *rlocator,
ForkNumber *forknum, BlockNumber *blknum);
Jacob Brazeal <jacob.brazeal@gmail.com> writes:
In bufmgr.c we have the debugging functions PrintBufferDescs
and PrintPinnedBufs, which are typically hidden behind the flag
#ifdef NOT_USED. These functions reference the old buf->refcount and
buf->flags fields, and so they no longer compile. I attached a patch to
use BUF_STATE_GET_REFCOUNT instead and removed the reference to buf->flags.
Hmm. So those functions have not compiled since 48354581a of
2016-04-10, and nobody's noticed. Moreover, whatever use-case
they do have would be reduced a good deal by taking a buffer
lock (which might not be available, or our own process might
hold it already).
I'd vote for just removing them...
regards, tom lane
On Fri, Jan 17, 2025 at 9:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Jacob Brazeal <jacob.brazeal@gmail.com> writes:
In bufmgr.c we have the debugging functions PrintBufferDescs
and PrintPinnedBufs, which are typically hidden behind the flag
#ifdef NOT_USED. These functions reference the old buf->refcount and
buf->flags fields, and so they no longer compile. I attached a patch to
use BUF_STATE_GET_REFCOUNT instead and removed the reference tobuf->flags.
Hmm. So those functions have not compiled since 48354581a of
2016-04-10, and nobody's noticed. Moreover, whatever use-case
they do have would be reduced a good deal by taking a buffer
lock (which might not be available, or our own process might
hold it already).I'd vote for just removing them...
regards, tom lane
Sounds good! v3 attached.
Attachments:
v3-buf-helper-compile-err.patchapplication/octet-stream; name=v3-buf-helper-compile-err.patchDownload
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 739daa1153..0d8849bf89 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4412,64 +4412,6 @@ DropDatabaseBuffers(Oid dbid)
}
}
-/* -----------------------------------------------------------------
- * PrintBufferDescs
- *
- * this function prints all the buffer descriptors, for debugging
- * use only.
- * -----------------------------------------------------------------
- */
-#ifdef NOT_USED
-void
-PrintBufferDescs(void)
-{
- int i;
-
- for (i = 0; i < NBuffers; ++i)
- {
- BufferDesc *buf = GetBufferDescriptor(i);
- Buffer b = BufferDescriptorGetBuffer(buf);
-
- /* theoretically we should lock the bufhdr here */
- elog(LOG,
- "[%02d] (freeNext=%d, rel=%s, "
- "blockNum=%u, flags=0x%x, refcount=%u %d)",
- i, buf->freeNext,
- relpathbackend(BufTagGetRelFileLocator(&buf->tag),
- INVALID_PROC_NUMBER, BufTagGetForkNum(&buf->tag)),
- buf->tag.blockNum, buf->flags,
- buf->refcount, GetPrivateRefCount(b));
- }
-}
-#endif
-
-#ifdef NOT_USED
-void
-PrintPinnedBufs(void)
-{
- int i;
-
- for (i = 0; i < NBuffers; ++i)
- {
- BufferDesc *buf = GetBufferDescriptor(i);
- Buffer b = BufferDescriptorGetBuffer(buf);
-
- if (GetPrivateRefCount(b) > 0)
- {
- /* theoretically we should lock the bufhdr here */
- elog(LOG,
- "[%02d] (freeNext=%d, rel=%s, "
- "blockNum=%u, flags=0x%x, refcount=%u %d)",
- i, buf->freeNext,
- relpathperm(BufTagGetRelFileLocator(&buf->tag),
- BufTagGetForkNum(&buf->tag)),
- buf->tag.blockNum, buf->flags,
- buf->refcount, GetPrivateRefCount(b));
- }
- }
-}
-#endif
-
/* ---------------------------------------------------------------------
* FlushRelationBuffers
*
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 3fdd29bd0b..4b2c8905c6 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -276,9 +276,6 @@ extern void DropDatabaseBuffers(Oid dbid);
extern bool BufferIsPermanent(Buffer buffer);
extern XLogRecPtr BufferGetLSNAtomic(Buffer buffer);
-#ifdef NOT_USED
-extern void PrintPinnedBufs(void);
-#endif
extern void BufferGetTag(Buffer buffer, RelFileLocator *rlocator,
ForkNumber *forknum, BlockNumber *blknum);
On Fri, Jan 17, 2025 at 11:14:02PM -0800, Jacob Brazeal wrote:
Sounds good! v3 attached.
Removal sounds good to me. Any objections from anybody?
Andres, perhaps you have some experience using that and would prefer
keep them and make them work?
--
Michael
Hi,
On 2025-01-19 09:37:54 +0900, Michael Paquier wrote:
On Fri, Jan 17, 2025 at 11:14:02PM -0800, Jacob Brazeal wrote:
Sounds good! v3 attached.
Removal sounds good to me. Any objections from anybody?
Andres, perhaps you have some experience using that and would prefer
keep them and make them work?
I think they're pretty useless, tbh. There's too many buffers that just
printing them out is helpful - pg_buffercache is going to be a better
bet. When debugging issues where pg_buffercache isn't an option (e.g. because
it's a hang that doesn't allow running pg_buffercache), using
DebugPrintBufferRefcount() is much more targeted.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2025-01-19 09:37:54 +0900, Michael Paquier wrote:
Removal sounds good to me. Any objections from anybody?
Andres, perhaps you have some experience using that and would prefer
keep them and make them work?
I think they're pretty useless, tbh. There's too many buffers that just
printing them out is helpful - pg_buffercache is going to be a better
bet. When debugging issues where pg_buffercache isn't an option (e.g. because
it's a hang that doesn't allow running pg_buffercache), using
DebugPrintBufferRefcount() is much more targeted.
Sounds like we're in agreement. I'll push Jacob's second patch.
regards, tom lane