On 18.09.2024 21:04, Fujii Masao wrote:

-                CreateCheckPoint(flags);
-                ckpt_performed = true;
+                ckpt_performed = CreateCheckPoint(flags);

This change could result in the next scheduled checkpoint being
triggered in 15 seconds if a checkpoint is skipped, which isn’t
the intended behavior.

Thanks for pointing this out! This is really bug.
Rearranged the logic a bit to save the previous behavior
in the v3 attached.

-void
+bool
  CreateCheckPoint(int flags)

It would be helpful to explain the new return value in the comment
at the top of this function.

Sure. Added an info about return value to the comment.

-{ oid => '2769',
+{ oid => '6347',

I don't think that the existing functions need to be reassigned new OIDs.

Ok. Left oids as is in the v3. Just added a new one for
pg_stat_get_checkpointer_num_performed().


With the best regards!

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

From 5832b1beb453b96a6ccbe72c404f2ad5373d0497 Mon Sep 17 00:00:00 2001
From: "Anton A. Melnikov" <a.melni...@postgrespro.ru>
Date: Thu, 19 Sep 2024 12:51:17 +0300
Subject: [PATCH] Introduce num_done counter in the pg_stat_checkpointer view
 that reflects number of really performed checkpoints.

---
 doc/src/sgml/monitoring.sgml                  | 11 ++++-
 src/backend/access/transam/xlog.c             |  9 +++-
 src/backend/catalog/system_views.sql          |  1 +
 src/backend/postmaster/checkpointer.c         | 45 ++++++++++++-------
 .../utils/activity/pgstat_checkpointer.c      |  2 +
 src/backend/utils/adt/pgstatfuncs.c           |  6 +++
 src/include/access/xlog.h                     |  2 +-
 src/include/catalog/pg_proc.dat               | 26 ++++++-----
 src/include/pgstat.h                          |  1 +
 src/test/regress/expected/rules.out           |  1 +
 10 files changed, 74 insertions(+), 30 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index a2fda4677d7..19bf0164f1c 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3063,7 +3063,16 @@ description | Waiting for a newly initialized WAL file to reach durable storage
        <structfield>num_requested</structfield> <type>bigint</type>
       </para>
       <para>
-       Number of requested checkpoints that have been performed
+       Number of backend requested checkpoints
+      </para></entry>
+     </row>
+
+      <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>num_done</structfield> <type>bigint</type>
+      </para>
+      <para>
+       Number of checkpoints that have been performed
       </para></entry>
      </row>
 
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 853ab06812b..c9d37cba453 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6878,8 +6878,11 @@ update_checkpoint_display(int flags, bool restartpoint, bool reset)
  * In this case, we only insert an XLOG_CHECKPOINT_SHUTDOWN record, and it's
  * both the record marking the completion of the checkpoint and the location
  * from which WAL replay would begin if needed.
+ *
+ * Returns true if a new checkpoint was performed or false if checkpoint
+ * was skipped because system is idle.
  */
-void
+bool
 CreateCheckPoint(int flags)
 {
 	bool		shutdown;
@@ -6971,7 +6974,7 @@ CreateCheckPoint(int flags)
 			END_CRIT_SECTION();
 			ereport(DEBUG1,
 					(errmsg_internal("checkpoint skipped because system is idle")));
-			return;
+			return false;
 		}
 	}
 
@@ -7353,6 +7356,8 @@ CreateCheckPoint(int flags)
 									 CheckpointStats.ckpt_segs_added,
 									 CheckpointStats.ckpt_segs_removed,
 									 CheckpointStats.ckpt_segs_recycled);
+
+	return true;
 }
 
 /*
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 7fd5d256a18..49109dbdc86 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1138,6 +1138,7 @@ CREATE VIEW pg_stat_checkpointer AS
     SELECT
         pg_stat_get_checkpointer_num_timed() AS num_timed,
         pg_stat_get_checkpointer_num_requested() AS num_requested,
+        pg_stat_get_checkpointer_num_performed() AS num_done,
         pg_stat_get_checkpointer_restartpoints_timed() AS restartpoints_timed,
         pg_stat_get_checkpointer_restartpoints_requested() AS restartpoints_req,
         pg_stat_get_checkpointer_restartpoints_performed() AS restartpoints_done,
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index eeb73c85726..8d610a3f1a7 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -461,8 +461,7 @@ CheckpointerMain(char *startup_data, size_t startup_data_len)
 			 */
 			if (!do_restartpoint)
 			{
-				CreateCheckPoint(flags);
-				ckpt_performed = true;
+				ckpt_performed = CreateCheckPoint(flags);
 			}
 			else
 				ckpt_performed = CreateRestartPoint(flags);
@@ -484,27 +483,41 @@ CheckpointerMain(char *startup_data, size_t startup_data_len)
 
 			ConditionVariableBroadcast(&CheckpointerShmem->done_cv);
 
-			if (ckpt_performed)
+			if (!do_restartpoint)
 			{
 				/*
-				 * Note we record the checkpoint start time not end time as
-				 * last_checkpoint_time.  This is so that time-driven
-				 * checkpoints happen at a predictable spacing.
-				 */
+				* Note we record the checkpoint start time not end time as
+				* last_checkpoint_time.  This is so that time-driven
+				* checkpoints happen at a predictable spacing.
+				*/
 				last_checkpoint_time = now;
 
-				if (do_restartpoint)
-					PendingCheckpointerStats.restartpoints_performed++;
+				if(ckpt_performed)
+					PendingCheckpointerStats.num_performed++;
+
 			}
 			else
 			{
-				/*
-				 * We were not able to perform the restartpoint (checkpoints
-				 * throw an ERROR in case of error).  Most likely because we
-				 * have not received any new checkpoint WAL records since the
-				 * last restartpoint. Try again in 15 s.
-				 */
-				last_checkpoint_time = now - CheckPointTimeout + 15;
+				if (ckpt_performed)
+				{
+					/*
+					 * The same as for checkpoint.
+					 * Please see the corresponding comment.
+					 */
+					last_checkpoint_time = now;
+
+					PendingCheckpointerStats.restartpoints_performed++;
+				}
+				else
+				{
+					/*
+					* We were not able to perform the restartpoint (checkpoints
+					* throw an ERROR in case of error).  Most likely because we
+					* have not received any new checkpoint WAL records since the
+					* last restartpoint. Try again in 15 s.
+					*/
+					last_checkpoint_time = now - CheckPointTimeout + 15;
+				}
 			}
 
 			ckpt_active = false;
diff --git a/src/backend/utils/activity/pgstat_checkpointer.c b/src/backend/utils/activity/pgstat_checkpointer.c
index bbfc9c7e183..4a0a2d1493a 100644
--- a/src/backend/utils/activity/pgstat_checkpointer.c
+++ b/src/backend/utils/activity/pgstat_checkpointer.c
@@ -49,6 +49,7 @@ pgstat_report_checkpointer(void)
 #define CHECKPOINTER_ACC(fld) stats_shmem->stats.fld += PendingCheckpointerStats.fld
 	CHECKPOINTER_ACC(num_timed);
 	CHECKPOINTER_ACC(num_requested);
+	CHECKPOINTER_ACC(num_performed);
 	CHECKPOINTER_ACC(restartpoints_timed);
 	CHECKPOINTER_ACC(restartpoints_requested);
 	CHECKPOINTER_ACC(restartpoints_performed);
@@ -127,6 +128,7 @@ pgstat_checkpointer_snapshot_cb(void)
 #define CHECKPOINTER_COMP(fld) pgStatLocal.snapshot.checkpointer.fld -= reset.fld;
 	CHECKPOINTER_COMP(num_timed);
 	CHECKPOINTER_COMP(num_requested);
+	CHECKPOINTER_COMP(num_performed);
 	CHECKPOINTER_COMP(restartpoints_timed);
 	CHECKPOINTER_COMP(restartpoints_requested);
 	CHECKPOINTER_COMP(restartpoints_performed);
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 9c23ac7c8c8..17b0fc02ef0 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1191,6 +1191,12 @@ pg_stat_get_checkpointer_num_requested(PG_FUNCTION_ARGS)
 	PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->num_requested);
 }
 
+Datum
+pg_stat_get_checkpointer_num_performed(PG_FUNCTION_ARGS)
+{
+	PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->num_performed);
+}
+
 Datum
 pg_stat_get_checkpointer_restartpoints_timed(PG_FUNCTION_ARGS)
 {
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 083810f5b4c..36f6e4e4b4e 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -239,7 +239,7 @@ extern void LocalProcessControlFile(bool reset);
 extern WalLevel GetActiveWalLevelOnStandby(void);
 extern void StartupXLOG(void);
 extern void ShutdownXLOG(int code, Datum arg);
-extern void CreateCheckPoint(int flags);
+extern bool CreateCheckPoint(int flags);
 extern bool CreateRestartPoint(int flags);
 extern WALAvailability GetWALAvailability(XLogRecPtr targetLSN);
 extern void XLogPutNextOid(Oid nextOid);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 43f608d7a0a..baa6120e664 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5817,6 +5817,12 @@
   proname => 'pg_stat_get_checkpointer_num_requested', provolatile => 's',
   proparallel => 'r', prorettype => 'int8', proargtypes => '',
   prosrc => 'pg_stat_get_checkpointer_num_requested' },
+{ oid => '2775',
+  descr => 'statistics: number of checkpoints performed by the checkpointer',
+  proname => 'pg_stat_get_checkpointer_num_performed',
+  provolatile => 's', proparallel => 'r', prorettype => 'int8',
+  proargtypes => '',
+  prosrc => 'pg_stat_get_checkpointer_num_performed' },
 { oid => '6327',
   descr => 'statistics: number of timed restartpoints started by the checkpointer',
   proname => 'pg_stat_get_checkpointer_restartpoints_timed', provolatile => 's',
@@ -5843,6 +5849,16 @@
   proname => 'pg_stat_get_checkpointer_stat_reset_time', provolatile => 's',
   proparallel => 'r', prorettype => 'timestamptz', proargtypes => '',
   prosrc => 'pg_stat_get_checkpointer_stat_reset_time' },
+  { oid => '3160',
+  descr => 'statistics: checkpoint/restartpoint time spent writing buffers to disk, in milliseconds',
+  proname => 'pg_stat_get_checkpointer_write_time', provolatile => 's',
+  proparallel => 'r', prorettype => 'float8', proargtypes => '',
+  prosrc => 'pg_stat_get_checkpointer_write_time' },
+{ oid => '3161',
+  descr => 'statistics: checkpoint/restartpoint time spent synchronizing buffers to disk, in milliseconds',
+  proname => 'pg_stat_get_checkpointer_sync_time', provolatile => 's',
+  proparallel => 'r', prorettype => 'float8', proargtypes => '',
+  prosrc => 'pg_stat_get_checkpointer_sync_time' },
 { oid => '2772',
   descr => 'statistics: number of buffers written by the bgwriter for cleaning dirty buffers',
   proname => 'pg_stat_get_bgwriter_buf_written_clean', provolatile => 's',
@@ -5857,16 +5873,6 @@
   proname => 'pg_stat_get_bgwriter_stat_reset_time', provolatile => 's',
   proparallel => 'r', prorettype => 'timestamptz', proargtypes => '',
   prosrc => 'pg_stat_get_bgwriter_stat_reset_time' },
-{ oid => '3160',
-  descr => 'statistics: checkpoint/restartpoint time spent writing buffers to disk, in milliseconds',
-  proname => 'pg_stat_get_checkpointer_write_time', provolatile => 's',
-  proparallel => 'r', prorettype => 'float8', proargtypes => '',
-  prosrc => 'pg_stat_get_checkpointer_write_time' },
-{ oid => '3161',
-  descr => 'statistics: checkpoint/restartpoint time spent synchronizing buffers to disk, in milliseconds',
-  proname => 'pg_stat_get_checkpointer_sync_time', provolatile => 's',
-  proparallel => 'r', prorettype => 'float8', proargtypes => '',
-  prosrc => 'pg_stat_get_checkpointer_sync_time' },
 { oid => '2859', descr => 'statistics: number of buffer allocations',
   proname => 'pg_stat_get_buf_alloc', provolatile => 's', proparallel => 'r',
   prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_buf_alloc' },
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 4752dfe7197..476acd680c0 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -294,6 +294,7 @@ typedef struct PgStat_CheckpointerStats
 {
 	PgStat_Counter num_timed;
 	PgStat_Counter num_requested;
+	PgStat_Counter num_performed;
 	PgStat_Counter restartpoints_timed;
 	PgStat_Counter restartpoints_requested;
 	PgStat_Counter restartpoints_performed;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index a1626f3fae9..f5434d8365c 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1824,6 +1824,7 @@ pg_stat_bgwriter| SELECT pg_stat_get_bgwriter_buf_written_clean() AS buffers_cle
     pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
 pg_stat_checkpointer| SELECT pg_stat_get_checkpointer_num_timed() AS num_timed,
     pg_stat_get_checkpointer_num_requested() AS num_requested,
+    pg_stat_get_checkpointer_num_performed() AS num_done,
     pg_stat_get_checkpointer_restartpoints_timed() AS restartpoints_timed,
     pg_stat_get_checkpointer_restartpoints_requested() AS restartpoints_req,
     pg_stat_get_checkpointer_restartpoints_performed() AS restartpoints_done,
-- 
2.46.0

Reply via email to