Hi, On Fri, 15 Sept 2023 at 16:30, Melanie Plageman <melanieplage...@gmail.com> wrote: > > Yes, good catch. This is a bug. I will note that at least in 15 and > likely before, pgBufferUsage.local_blks_written is incremented for > local buffers but pgBufferUsage.blk_write_time is only added to for > shared buffers (in FlushBuffer()). I think it makes sense to propose a > bug fix to stable branches counting blk_write_time for local buffers > as well.
I attached the PG16+ (after pg_stat_io) and PG15- (before pg_stat_io) versions of the same patch. Regards, Nazir Bilal Yavuz Microsoft
From b4ea504497708b1017e8cfda5f1ac366a9e34386 Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <byavuz81@gmail.com> Date: Fri, 15 Sep 2023 11:55:43 +0300 Subject: [PATCH v2 2/2] [PG16+] Add pgBufferUsage.blk_write_time to for IOOp IOOP_EXTENDs --- src/backend/utils/activity/pgstat_io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c index c8058b57962..56051fc6072 100644 --- a/src/backend/utils/activity/pgstat_io.c +++ b/src/backend/utils/activity/pgstat_io.c @@ -119,7 +119,7 @@ pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op, INSTR_TIME_SET_CURRENT(io_time); INSTR_TIME_SUBTRACT(io_time, start_time); - if (io_op == IOOP_WRITE) + if (io_op == IOOP_WRITE || io_op == IOOP_EXTEND) { pgstat_count_buffer_write_time(INSTR_TIME_GET_MICROSEC(io_time)); if (io_object == IOOBJECT_RELATION -- 2.42.0
From 46aaa1f76f02f7697f520e6509a3aeb970a862ae Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <byavuz81@gmail.com> Date: Fri, 15 Sep 2023 12:35:01 +0300 Subject: [PATCH v2 1/2] [PG16+] Increase pgBufferUsage.blk_{read|write}_time when IOObject is IOOBJECT_TEMP_RELATION --- src/backend/utils/activity/pgstat_io.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c index eb7d35d4225..c8058b57962 100644 --- a/src/backend/utils/activity/pgstat_io.c +++ b/src/backend/utils/activity/pgstat_io.c @@ -122,13 +122,15 @@ pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op, if (io_op == IOOP_WRITE) { pgstat_count_buffer_write_time(INSTR_TIME_GET_MICROSEC(io_time)); - if (io_object == IOOBJECT_RELATION) + if (io_object == IOOBJECT_RELATION + || io_object == IOOBJECT_TEMP_RELATION) INSTR_TIME_ADD(pgBufferUsage.blk_write_time, io_time); } else if (io_op == IOOP_READ) { pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time)); - if (io_object == IOOBJECT_RELATION) + if (io_object == IOOBJECT_RELATION + || io_object == IOOBJECT_TEMP_RELATION) INSTR_TIME_ADD(pgBufferUsage.blk_read_time, io_time); } -- 2.42.0
From 1e3af6e9466924713488b374b5d7bfb2c3bd5983 Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <byavu...@gmail.com> Date: Mon, 2 Oct 2023 16:31:46 +0300 Subject: [PATCH v2 2/2] [PG15-] Add pgBufferUsage.blk_write_time to for extends --- src/backend/storage/buffer/bufmgr.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 9fcb3d6e194..a4e9fd3317b 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -825,6 +825,8 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, bool found; bool isExtend; bool isLocalBuf = SmgrIsTemp(smgr); + instr_time io_start, + io_time; *hit = false; @@ -992,9 +994,21 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, { /* new buffers are zero-filled */ MemSet((char *) bufBlock, 0, BLCKSZ); + + if (track_io_timing) + INSTR_TIME_SET_CURRENT(io_start); + /* don't set checksum for all-zero page */ smgrextend(smgr, forkNum, blockNum, (char *) bufBlock, false); + if (track_io_timing) + { + INSTR_TIME_SET_CURRENT(io_time); + INSTR_TIME_SUBTRACT(io_time, io_start); + pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time)); + INSTR_TIME_ADD(pgBufferUsage.blk_write_time, io_time); + } + /* * NB: we're *not* doing a ScheduleBufferTagForWriteback here; * although we're essentially performing a write. At least on linux @@ -1012,9 +1026,6 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, MemSet((char *) bufBlock, 0, BLCKSZ); else { - instr_time io_start, - io_time; - if (track_io_timing) INSTR_TIME_SET_CURRENT(io_start); -- 2.42.0
From c7fdf1fa2a1f878a96a71b04e9fc65b1f2a0a402 Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz <byavu...@gmail.com> Date: Mon, 2 Oct 2023 16:29:16 +0300 Subject: [PATCH v2 1/2] [PG15-] Add pgBufferUsage.blk_write_time to for local buffers --- src/backend/storage/buffer/localbuf.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c index e71f95ac1ff..b86c0a8f64f 100644 --- a/src/backend/storage/buffer/localbuf.c +++ b/src/backend/storage/buffer/localbuf.c @@ -18,6 +18,7 @@ #include "access/parallel.h" #include "catalog/catalog.h" #include "executor/instrument.h" +#include "pgstat.h" #include "storage/buf_internals.h" #include "storage/bufmgr.h" #include "utils/guc.h" @@ -116,6 +117,8 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum, int trycounter; bool found; uint32 buf_state; + instr_time io_start, + io_time; INIT_BUFFERTAG(newTag, smgr->smgr_rnode.node, forkNum, blockNum); @@ -219,6 +222,9 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum, PageSetChecksumInplace(localpage, bufHdr->tag.blockNum); + if (track_io_timing) + INSTR_TIME_SET_CURRENT(io_start); + /* And write... */ smgrwrite(oreln, bufHdr->tag.forkNum, @@ -226,6 +232,14 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum, localpage, false); + if (track_io_timing) + { + INSTR_TIME_SET_CURRENT(io_time); + INSTR_TIME_SUBTRACT(io_time, io_start); + pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time)); + INSTR_TIME_ADD(pgBufferUsage.blk_write_time, io_time); + } + /* Mark not-dirty now in case we error out below */ buf_state &= ~BM_DIRTY; pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state); -- 2.42.0