On 2024/10/01 3:33, Fujii Masao wrote:
On 2024/09/22 20:44, Nitin Jadhav wrote:
+
CheckpointStats.ckpt_slru_written,
+ (double)
CheckpointStats.ckpt_slru_written * 100 / NBuffers,
I don't think NBuffers represents the maximum number of SLRU buffers.
We might need to calculate this based on specific GUC settings,
like transaction_buffers.
Great observation. Since the SLRU buffers written during a checkpoint
can include transaction_buffers, commit_timestamp_buffers,
subtransaction_buffers, multixact_member_buffers,
multixact_offset_buffers, and serializable_buffers, the total count of
SLRU buffers should be the sum of all these types. We might need to
introduce a global variable, such as total_slru_count, in the
globals.c file to hold this sum. The num_slots variable in the
SlruSharedData structure needs to be accessed from all types of SLRU
and stored in total_slru_count. This can then be used during logging
to calculate the percentage of SLRU buffers written. However, I’m
unsure if this effort is justified. While it makes sense for normal
buffers to display the percentage, the additional code required might
not provide significant value to users. Therefore, I have removed this
in the attached v6 patch.
+1
Thanks for updating the patch! It looks good to me.
Barring any objections, I will commit this patch.
Before committing the patch, I revised it with the following changes:
- I added "shared" to the description of pg_stat_checkpointer.buffers_written
to clarify that it tracks shared buffers.
- In the checkpoint log message, I replaced "slru" with "SLRU" for consistency,
as uppercase is typically used for SLRU.
- I updated the commit message.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
From 150a915995115a882b18ad94c104b58dd1673cb1 Mon Sep 17 00:00:00 2001
From: Nitin Jadhav <nitinjad...@microsoft.com>
Date: Sun, 22 Sep 2024 17:04:23 +0530
Subject: [PATCH v7] Fix inconsistent reporting of checkpointer stats
Previously, the pg_stat_checkpointer view and the checkpoint completion
log message could show different numbers for buffers written
during checkpoints. The view only counted shared buffers,
while the log message included both shared and SLRU buffers,
causing inconsistencies.
This commit resolves the issue by updating both the view and the log message
to separately report shared and SLRU buffers written during checkpoints.
A new slru_written column is added to the pg_stat_checkpointer view
to track SLRU buffers, while the existing buffers_written column now
tracks only shared buffers. This change would help users distinguish
between the two types of buffers, in the pg_stat_checkpointer view and
the checkpoint complete log message, respectively.
Bump catalog version.
Author: Nitin Jadhav
Reviewed-by: Bharath Rupireddy, Michael Paquier, Kyotaro Horiguchi, Robert Haas
Reviewed-by: Andres Freund, vignesh C
Discussion:
https://postgr.es/m/camm1awb18ept0whjrjg+-nyhnouxet6zuw0pnyyae+nezpv...@mail.gmail.com
---
doc/src/sgml/monitoring.sgml | 11 +++++++-
src/backend/access/transam/slru.c | 7 +++--
src/backend/access/transam/xlog.c | 26 ++++++++++---------
src/backend/catalog/system_views.sql | 1 +
.../utils/activity/pgstat_checkpointer.c | 2 ++
src/backend/utils/adt/pgstatfuncs.c | 6 +++++
src/include/access/xlog.h | 1 +
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_proc.dat | 5 ++++
src/include/pgstat.h | 1 +
src/test/regress/expected/rules.out | 1 +
11 files changed, 47 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 48ffe87241..331315f8d3 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3127,7 +3127,16 @@ description | Waiting for a newly initialized WAL file
to reach durable storage
<structfield>buffers_written</structfield> <type>bigint</type>
</para>
<para>
- Number of buffers written during checkpoints and restartpoints
+ Number of shared buffers written during checkpoints and restartpoints
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>slru_written</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of SLRU buffers written during checkpoints and restartpoints
</para></entry>
</row>
diff --git a/src/backend/access/transam/slru.c
b/src/backend/access/transam/slru.c
index e7f73bf427..889eff1815 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -716,9 +716,12 @@ SlruInternalWritePage(SlruCtl ctl, int slotno,
SlruWriteAll fdata)
if (!ok)
SlruReportIOError(ctl, pageno, InvalidTransactionId);
- /* If part of a checkpoint, count this as a buffer written. */
+ /* If part of a checkpoint, count this as a SLRU buffer written. */
if (fdata)
- CheckpointStats.ckpt_bufs_written++;
+ {
+ CheckpointStats.ckpt_slru_written++;
+ PendingCheckpointerStats.slru_written++;
+ }
}
/*
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 64304d77d3..9102c8d772 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6727,14 +6727,15 @@ LogCheckpointEnd(bool restartpoint)
*/
if (restartpoint)
ereport(LOG,
- (errmsg("restartpoint complete: wrote %d
buffers (%.1f%%); "
- "%d WAL file(s) added, %d
removed, %d recycled; "
- "write=%ld.%03d s,
sync=%ld.%03d s, total=%ld.%03d s; "
- "sync files=%d,
longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d
kB; "
- "lsn=%X/%X, redo lsn=%X/%X",
+ (errmsg("restartpoint complete: wrote %d
buffers (%.1f%%), "
+ "wrote %d SLRU buffers; %d WAL
file(s) added, "
+ "%d removed, %d recycled;
write=%ld.%03d s, "
+ "sync=%ld.%03d s,
total=%ld.%03d s; sync files=%d, "
+ "longest=%ld.%03d s,
average=%ld.%03d s; distance=%d kB, "
+ "estimate=%d kB; lsn=%X/%X,
redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double)
CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
+
CheckpointStats.ckpt_slru_written,
CheckpointStats.ckpt_segs_added,
CheckpointStats.ckpt_segs_removed,
CheckpointStats.ckpt_segs_recycled,
@@ -6750,14 +6751,15 @@ LogCheckpointEnd(bool restartpoint)
LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
else
ereport(LOG,
- (errmsg("checkpoint complete: wrote %d buffers
(%.1f%%); "
- "%d WAL file(s) added, %d
removed, %d recycled; "
- "write=%ld.%03d s,
sync=%ld.%03d s, total=%ld.%03d s; "
- "sync files=%d,
longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d
kB; "
- "lsn=%X/%X, redo lsn=%X/%X",
+ (errmsg("checkpoint complete: wrote %d buffers
(%.1f%%), "
+ "wrote %d SLRU buffers; %d WAL
file(s) added, "
+ "%d removed, %d recycled;
write=%ld.%03d s, "
+ "sync=%ld.%03d s,
total=%ld.%03d s; sync files=%d, "
+ "longest=%ld.%03d s,
average=%ld.%03d s; distance=%d kB, "
+ "estimate=%d kB; lsn=%X/%X,
redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double)
CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
+
CheckpointStats.ckpt_slru_written,
CheckpointStats.ckpt_segs_added,
CheckpointStats.ckpt_segs_removed,
CheckpointStats.ckpt_segs_recycled,
diff --git a/src/backend/catalog/system_views.sql
b/src/backend/catalog/system_views.sql
index 49109dbdc8..3456b821bc 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1145,6 +1145,7 @@ CREATE VIEW pg_stat_checkpointer AS
pg_stat_get_checkpointer_write_time() AS write_time,
pg_stat_get_checkpointer_sync_time() AS sync_time,
pg_stat_get_checkpointer_buffers_written() AS buffers_written,
+ pg_stat_get_checkpointer_slru_written() AS slru_written,
pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
CREATE VIEW pg_stat_io AS
diff --git a/src/backend/utils/activity/pgstat_checkpointer.c
b/src/backend/utils/activity/pgstat_checkpointer.c
index 4a0a2d1493..5a3fb4a9e0 100644
--- a/src/backend/utils/activity/pgstat_checkpointer.c
+++ b/src/backend/utils/activity/pgstat_checkpointer.c
@@ -56,6 +56,7 @@ pgstat_report_checkpointer(void)
CHECKPOINTER_ACC(write_time);
CHECKPOINTER_ACC(sync_time);
CHECKPOINTER_ACC(buffers_written);
+ CHECKPOINTER_ACC(slru_written);
#undef CHECKPOINTER_ACC
pgstat_end_changecount_write(&stats_shmem->changecount);
@@ -135,5 +136,6 @@ pgstat_checkpointer_snapshot_cb(void)
CHECKPOINTER_COMP(write_time);
CHECKPOINTER_COMP(sync_time);
CHECKPOINTER_COMP(buffers_written);
+ CHECKPOINTER_COMP(slru_written);
#undef CHECKPOINTER_COMP
}
diff --git a/src/backend/utils/adt/pgstatfuncs.c
b/src/backend/utils/adt/pgstatfuncs.c
index 17b0fc02ef..f7b50e0b5a 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1221,6 +1221,12 @@
pg_stat_get_checkpointer_buffers_written(PG_FUNCTION_ARGS)
PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->buffers_written);
}
+Datum
+pg_stat_get_checkpointer_slru_written(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->slru_written);
+}
+
Datum
pg_stat_get_bgwriter_buf_written_clean(PG_FUNCTION_ARGS)
{
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 36f6e4e4b4..34ad46c067 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -167,6 +167,7 @@ typedef struct CheckpointStatsData
TimestampTz ckpt_end_t; /* end of checkpoint */
int ckpt_bufs_written; /* # of buffers written
*/
+ int ckpt_slru_written; /* # of SLRU buffers
written */
int ckpt_segs_added; /* # of new xlog
segments created */
int ckpt_segs_removed; /* # of xlog segments
deleted */
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 3c89b70f9e..946ee29af2 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -57,6 +57,6 @@
*/
/* yyyymmddN */
-#define CATALOG_VERSION_NO 202409302
+#define CATALOG_VERSION_NO 202410011
#endif
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 05fcbf7515..77f54a79e6 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5847,6 +5847,11 @@
proname => 'pg_stat_get_checkpointer_buffers_written', provolatile => 's',
proparallel => 'r', prorettype => 'int8', proargtypes => '',
prosrc => 'pg_stat_get_checkpointer_buffers_written' },
+{ oid => '8573',
+ descr => 'statistics: number of SLRU buffers written during checkpoints and
restartpoints',
+ proname => 'pg_stat_get_checkpointer_slru_written', provolatile => 's',
+ proparallel => 'r', prorettype => 'int8', proargtypes => '',
+ prosrc => 'pg_stat_get_checkpointer_slru_written' },
{ oid => '6314', descr => 'statistics: last reset for the checkpointer',
proname => 'pg_stat_get_checkpointer_stat_reset_time', provolatile => 's',
proparallel => 'r', prorettype => 'timestamptz', proargtypes => '',
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 476acd680c..df53fa2d4f 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -301,6 +301,7 @@ typedef struct PgStat_CheckpointerStats
PgStat_Counter write_time; /* times in milliseconds */
PgStat_Counter sync_time;
PgStat_Counter buffers_written;
+ PgStat_Counter slru_written;
TimestampTz stat_reset_timestamp;
} PgStat_CheckpointerStats;
diff --git a/src/test/regress/expected/rules.out
b/src/test/regress/expected/rules.out
index f5434d8365..2b47013f11 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1831,6 +1831,7 @@ pg_stat_checkpointer| SELECT
pg_stat_get_checkpointer_num_timed() AS num_timed,
pg_stat_get_checkpointer_write_time() AS write_time,
pg_stat_get_checkpointer_sync_time() AS sync_time,
pg_stat_get_checkpointer_buffers_written() AS buffers_written,
+ pg_stat_get_checkpointer_slru_written() AS slru_written,
pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
pg_stat_database| SELECT oid AS datid,
datname,
--
2.45.2
