Hi,
As suggested in [1], the attached patch adds shared buffer hits to
pg_stat_io.
I remember at some point having this in the view and then removing it
but I can't quite remember what the issue was -- nor do I see a
rationale mentioned in the thread [2].
It might have had something to do with the interaction between the
now-removed "rejected" buffers column.
I am looking for input as to the order of this column in the view. I
think it should go after op_bytes since it is not relevant for
non-block-oriented IO. However, I'm not sure what the order of hits,
evictions, and reuses should be (all have to do with buffers).
While adding this, I noticed that I had made all of the IOOP columns
int8 in the view, and I was wondering if this is sufficient for hits (I
imagine you could end up with quite a lot of those).
- Melanie
[1]
https://www.postgresql.org/message-id/20230209050319.chyyup4vtq4jzobq%40awork3.anarazel.de
[2]
https://www.postgresql.org/message-id/flat/20230209050319.chyyup4vtq4jzobq%40awork3.anarazel.de#63ff7a97b7a5bb7b86c1a250065be7f9
From 4055b14e3c2c610575a9bb3fe631b2094d3ea1a6 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Sat, 25 Feb 2023 14:37:25 -0500
Subject: [PATCH v1 1/2] Reorder pgstatfuncs-local enum
---
src/backend/utils/adt/pgstatfuncs.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 9d707c3521..1e55e077b7 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1276,16 +1276,16 @@ pgstat_get_io_op_index(IOOp io_op)
{
case IOOP_EVICT:
return IO_COL_EVICTIONS;
+ case IOOP_EXTEND:
+ return IO_COL_EXTENDS;
+ case IOOP_FSYNC:
+ return IO_COL_FSYNCS;
case IOOP_READ:
return IO_COL_READS;
case IOOP_REUSE:
return IO_COL_REUSES;
case IOOP_WRITE:
return IO_COL_WRITES;
- case IOOP_EXTEND:
- return IO_COL_EXTENDS;
- case IOOP_FSYNC:
- return IO_COL_FSYNCS;
}
elog(ERROR, "unrecognized IOOp value: %d", io_op);
--
2.37.2
From a3821a166b57464839ee5878598c233b5a722572 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Sat, 25 Feb 2023 14:36:06 -0500
Subject: [PATCH v1 2/2] Add IOOP_HIT to pg_stat_io
Begin tracking shared buffer hits in pg_stat_io.
---
doc/src/sgml/monitoring.sgml | 11 ++++++++
src/backend/catalog/system_views.sql | 1 +
src/backend/storage/buffer/bufmgr.c | 38 ++++++++++----------------
src/backend/storage/buffer/localbuf.c | 10 +------
src/backend/utils/activity/pgstat_io.c | 2 +-
src/backend/utils/adt/pgstatfuncs.c | 3 ++
src/include/catalog/pg_proc.dat | 6 ++--
src/include/pgstat.h | 1 +
src/include/storage/buf_internals.h | 2 +-
src/test/regress/expected/rules.out | 3 +-
10 files changed, 38 insertions(+), 39 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index b0b997f092..53e7e40a07 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3855,6 +3855,17 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
</entry>
</row>
+ <row>
+ <entry role="catalog_table_entry">
+ <para role="column_definition">
+ <structfield>hits</structfield> <type>bigint</type>
+ </para>
+ <para>
+ The number of times a desired block was found in a shared buffer.
+ </para>
+ </entry>
+ </row>
+
<row>
<entry role="catalog_table_entry">
<para role="column_definition">
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 34ca0e739f..87bbbdfcb3 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1126,6 +1126,7 @@ SELECT
b.writes,
b.extends,
b.op_bytes,
+ b.hits,
b.evictions,
b.reuses,
b.fsyncs,
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 98904a7c05..7a23088c0b 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -472,7 +472,7 @@ static BufferDesc *BufferAlloc(SMgrRelation smgr,
ForkNumber forkNum,
BlockNumber blockNum,
BufferAccessStrategy strategy,
- bool *foundPtr, IOContext *io_context);
+ bool *foundPtr, IOContext io_context);
static void FlushBuffer(BufferDesc *buf, SMgrRelation reln,
IOObject io_object, IOContext io_context);
static void FindAndDropRelationBuffers(RelFileLocator rlocator,
@@ -850,13 +850,14 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
if (isLocalBuf)
{
/*
- * LocalBufferAlloc() will set the io_context to IOCONTEXT_NORMAL. We
- * do not use a BufferAccessStrategy for I/O of temporary tables.
+ * We do not use a BufferAccessStrategy for I/O of temporary tables.
* However, in some cases, the "strategy" may not be NULL, so we can't
* rely on IOContextForStrategy() to set the right IOContext for us.
* This may happen in cases like CREATE TEMPORARY TABLE AS...
*/
- bufHdr = LocalBufferAlloc(smgr, forkNum, blockNum, &found, &io_context);
+ io_context = IOCONTEXT_NORMAL;
+ io_object = IOOBJECT_TEMP_RELATION;
+ bufHdr = LocalBufferAlloc(smgr, forkNum, blockNum, &found, io_context);
if (found)
pgBufferUsage.local_blks_hit++;
else if (isExtend)
@@ -871,8 +872,10 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
* lookup the buffer. IO_IN_PROGRESS is set if the requested block is
* not currently in memory.
*/
+ io_context = IOContextForStrategy(strategy);
+ io_object = IOOBJECT_RELATION;
bufHdr = BufferAlloc(smgr, relpersistence, forkNum, blockNum,
- strategy, &found, &io_context);
+ strategy, &found, io_context);
if (found)
pgBufferUsage.shared_blks_hit++;
else if (isExtend)
@@ -892,6 +895,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
/* Just need to update stats before we exit */
*hit = true;
VacuumPageHit++;
+ pgstat_count_io_op(io_object, io_context, IOOP_HIT);
if (VacuumCostActive)
VacuumCostBalance += VacuumCostPageHit;
@@ -987,16 +991,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
*/
Assert(!(pg_atomic_read_u32(&bufHdr->state) & BM_VALID)); /* spinlock not needed */
- if (isLocalBuf)
- {
- bufBlock = LocalBufHdrGetBlock(bufHdr);
- io_object = IOOBJECT_TEMP_RELATION;
- }
- else
- {
- bufBlock = BufHdrGetBlock(bufHdr);
- io_object = IOOBJECT_RELATION;
- }
+ bufBlock = isLocalBuf ? LocalBufHdrGetBlock(bufHdr) : BufHdrGetBlock(bufHdr);
if (isExtend)
{
@@ -1139,7 +1134,7 @@ static BufferDesc *
BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
BlockNumber blockNum,
BufferAccessStrategy strategy,
- bool *foundPtr, IOContext *io_context)
+ bool *foundPtr, IOContext io_context)
{
bool from_ring;
BufferTag newTag; /* identity of requested block */
@@ -1193,11 +1188,8 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
{
/*
* If we get here, previous attempts to read the buffer must
- * have failed ... but we shall bravely try again. Set
- * io_context since we will in fact need to count an IO
- * Operation.
+ * have failed ... but we shall bravely try again.
*/
- *io_context = IOContextForStrategy(strategy);
*foundPtr = false;
}
}
@@ -1211,8 +1203,6 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
*/
LWLockRelease(newPartitionLock);
- *io_context = IOContextForStrategy(strategy);
-
/* Loop here in case we have to try another victim buffer */
for (;;)
{
@@ -1295,7 +1285,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
smgr->smgr_rlocator.locator.dbOid,
smgr->smgr_rlocator.locator.relNumber);
- FlushBuffer(buf, NULL, IOOBJECT_RELATION, *io_context);
+ FlushBuffer(buf, NULL, IOOBJECT_RELATION, io_context);
LWLockRelease(BufferDescriptorGetContentLock(buf));
ScheduleBufferTagForWriteback(&BackendWritebackContext,
@@ -1494,7 +1484,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
* we may have been forced to release the buffer due to concurrent
* pinners or erroring out.
*/
- pgstat_count_io_op(IOOBJECT_RELATION, *io_context,
+ pgstat_count_io_op(IOOBJECT_RELATION, io_context,
from_ring ? IOOP_REUSE : IOOP_EVICT);
}
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 5325ddb663..de5bb8607d 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -108,7 +108,7 @@ PrefetchLocalBuffer(SMgrRelation smgr, ForkNumber forkNum,
*/
BufferDesc *
LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
- bool *foundPtr, IOContext *io_context)
+ bool *foundPtr, IOContext io_context)
{
BufferTag newTag; /* identity of requested block */
LocalBufferLookupEnt *hresult;
@@ -128,14 +128,6 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
hresult = (LocalBufferLookupEnt *)
hash_search(LocalBufHash, &newTag, HASH_FIND, NULL);
- /*
- * IO Operations on local buffers are only done in IOCONTEXT_NORMAL. Set
- * io_context here (instead of after a buffer hit would have returned) for
- * convenience since we don't have to worry about the overhead of calling
- * IOContextForStrategy().
- */
- *io_context = IOCONTEXT_NORMAL;
-
if (hresult)
{
b = hresult->id;
diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c
index 0e07e0848d..6bc2355b63 100644
--- a/src/backend/utils/activity/pgstat_io.c
+++ b/src/backend/utils/activity/pgstat_io.c
@@ -349,7 +349,7 @@ pgstat_tracks_io_op(BackendType bktype, IOObject io_object,
* Some BackendTypes will not do certain IOOps.
*/
if ((bktype == B_BG_WRITER || bktype == B_CHECKPOINTER) &&
- (io_op == IOOP_READ || io_op == IOOP_EVICT))
+ (io_op == IOOP_READ || io_op == IOOP_EVICT || io_op == IOOP_HIT))
return false;
if ((bktype == B_AUTOVAC_LAUNCHER || bktype == B_BG_WRITER ||
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 1e55e077b7..8ee9434434 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1258,6 +1258,7 @@ typedef enum io_stat_col
IO_COL_WRITES,
IO_COL_EXTENDS,
IO_COL_CONVERSION,
+ IO_COL_HITS,
IO_COL_EVICTIONS,
IO_COL_REUSES,
IO_COL_FSYNCS,
@@ -1280,6 +1281,8 @@ pgstat_get_io_op_index(IOOp io_op)
return IO_COL_EXTENDS;
case IOOP_FSYNC:
return IO_COL_FSYNCS;
+ case IOOP_HIT:
+ return IO_COL_HITS;
case IOOP_READ:
return IO_COL_READS;
case IOOP_REUSE:
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index e2a7642a2b..0b806e4f7e 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5721,9 +5721,9 @@
proname => 'pg_stat_get_io', provolatile => 'v',
prorows => '30', proretset => 't',
proparallel => 'r', prorettype => 'record', proargtypes => '',
- proallargtypes => '{text,text,text,int8,int8,int8,int8,int8,int8,int8,timestamptz}',
- proargmodes => '{o,o,o,o,o,o,o,o,o,o,o}',
- proargnames => '{backend_type,io_object,io_context,reads,writes,extends,op_bytes,evictions,reuses,fsyncs,stats_reset}',
+ proallargtypes => '{text,text,text,int8,int8,int8,int8,int8,int8,int8,int8,timestamptz}',
+ proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o}',
+ proargnames => '{backend_type,io_object,io_context,reads,writes,extends,op_bytes,hits,evictions,reuses,fsyncs,stats_reset}',
prosrc => 'pg_stat_get_io' },
{ oid => '1136', descr => 'statistics: information about WAL activity',
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index db9675884f..fe83ba0cc8 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -306,6 +306,7 @@ typedef enum IOOp
IOOP_EVICT,
IOOP_EXTEND,
IOOP_FSYNC,
+ IOOP_HIT,
IOOP_READ,
IOOP_REUSE,
IOOP_WRITE,
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 0b44814740..409fbb44f7 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -419,7 +419,7 @@ extern PrefetchBufferResult PrefetchLocalBuffer(SMgrRelation smgr,
ForkNumber forkNum,
BlockNumber blockNum);
extern BufferDesc *LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum,
- BlockNumber blockNum, bool *foundPtr, IOContext *io_context);
+ BlockNumber blockNum, bool *foundPtr, IOContext io_context);
extern void MarkLocalBufferDirty(Buffer buffer);
extern void DropRelationLocalBuffers(RelFileLocator rlocator,
ForkNumber forkNum,
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index e953d1f515..c994da9f97 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1883,11 +1883,12 @@ pg_stat_io| SELECT backend_type,
writes,
extends,
op_bytes,
+ hits,
evictions,
reuses,
fsyncs,
stats_reset
- FROM pg_stat_get_io() b(backend_type, io_object, io_context, reads, writes, extends, op_bytes, evictions, reuses, fsyncs, stats_reset);
+ FROM pg_stat_get_io() b(backend_type, io_object, io_context, reads, writes, extends, op_bytes, hits, evictions, reuses, fsyncs, stats_reset);
pg_stat_progress_analyze| SELECT s.pid,
s.datid,
d.datname,
--
2.37.2
