Subject: Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica.

From: "Anton A. Melnikov" <[email-masked]>
Date: Thu, 16 Mar 2023 15:39:00 +0300

Hello!

On 15.03.2023 21:29, Gregory Stark (as CFM) wrote:

> These patches that are "Needs Review" and have received no comments at
> all since before March 1st are these. If your patch is amongst this
> list I would suggest any of:
> 
> 1) Move it yourself to the next CF (or withdraw it)
> 2) Post to the list with any pending questions asking for specific
> feedback -- it's much more likely to get feedback than just a generic
> "here's a patch plz review"...
> 3) Mark it Ready for Committer and possibly post explaining the
> resolution to any earlier questions to make it easier for a committer
> to understand the state
>

There are two different patch variants and some discussion expected.
So moved them to the next CF.

With the best wishes!

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

commit 6f3ab7b6b5562aaf68ccfdd6dde17c27c2d2869e
Author: Anton A. Melnikov <[email-masked]>
Date:   Wed Dec 7 10:10:58 2022 +0300

        Remove burst growth of the checkpoint_req on replica.
    
        with correcttions according to comment:      https://www.postgresql.org/message-id/20220907.163946.2214338139097492905.horikyota.ntt%40gmail.com

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index a31fbbff78..02d86bf370 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8935,3 +8935,20 @@ SetWalWriterSleeping(bool sleeping)
 	XLogCtl->WalWriterSleeping = sleeping;
 	SpinLockRelease(&XLogCtl->info_lck);
 }
+
+/*
+ * Check if a new checkpoint WAL record has been received since the
+ * last restartpoint. So it is possible to create a new one.
+ */
+bool RestartPointAvailable(void)
+{
+	bool result = false;
+
+	SpinLockAcquire(&XLogCtl->info_lck);
+	if (!XLogRecPtrIsInvalid(XLogCtl->lastCheckPointRecPtr)
+		&& XLogCtl->lastCheckPoint.redo > XLogCtl->RedoRecPtr)
+				result = true;
+	SpinLockRelease(&XLogCtl->info_lck);
+
+	return result;
+}
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 97b882564f..a88c716197 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -3196,7 +3196,15 @@ XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
 			{
 				(void) GetRedoRecPtr();
 				if (XLogCheckpointNeeded(readSegNo))
-					RequestCheckpoint(CHECKPOINT_CAUSE_XLOG);
+				{
+					/*
+						* If there is no new checkpoint WAL records since the
+						* last restartpoint the creation of new one
+						* will certainly fail, so skip it.
+						*/
+					if (RestartPointAvailable())
+						RequestCheckpoint(CHECKPOINT_CAUSE_XLOG);
+				}
 			}
 		}
 
diff --git a/src/include/access/xlogrecovery.h b/src/include/access/xlogrecovery.h
index f3398425d8..dcc2e64ca7 100644
--- a/src/include/access/xlogrecovery.h
+++ b/src/include/access/xlogrecovery.h
@@ -83,6 +83,7 @@ extern void InitWalRecovery(ControlFileData *ControlFile,
 							bool *wasShutdown_ptr, bool *haveBackupLabel_ptr,
 							bool *haveTblspcMap_ptr);
 extern void PerformWalRecovery(void);
+extern bool RestartPointAvailable(void);
 
 /*
  * FinishWalRecovery() returns this.  It contains information about the point
commit 9ec1f90bbe407da94f8e940084e8815b4f8e874b
Author: Anton A. Melnikov <[email-masked]>
Date:   Wed Dec 7 10:49:19 2022 +0300

    Add statistic about restartpoint into pg_stat_bgwriter

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 2d8104b090..658cafdc03 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1107,6 +1107,9 @@ CREATE VIEW pg_stat_bgwriter AS
     SELECT
         pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints_timed,
         pg_stat_get_bgwriter_requested_checkpoints() AS checkpoints_req,
+        pg_stat_get_bgwriter_timed_restartpoints() AS restartpoints_timed,
+        pg_stat_get_bgwriter_requested_restartpoints() AS restartpoints_req,
+        pg_stat_get_bgwriter_performed_restartpoints() AS restartpoints_done,
         pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
         pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
         pg_stat_get_bgwriter_buf_written_checkpoints() AS buffers_checkpoint,
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 5fc076fc14..2296701ddf 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -350,6 +350,8 @@ CheckpointerMain(void)
 		pg_time_t	now;
 		int			elapsed_secs;
 		int			cur_timeout;
+		bool		chkpt_or_rstpt_requested = false;
+		bool		chkpt_or_rstpt_timed = false;
 
 		/* Clear any already-pending wakeups */
 		ResetLatch(MyLatch);
@@ -368,7 +370,7 @@ CheckpointerMain(void)
 		if (((volatile CheckpointerShmemStruct *) CheckpointerShmem)->ckpt_flags)
 		{
 			do_checkpoint = true;
-			PendingCheckpointerStats.requested_checkpoints++;
+			chkpt_or_rstpt_requested = true;
 		}
 
 		/*
@@ -382,7 +384,7 @@ CheckpointerMain(void)
 		if (elapsed_secs >= CheckPointTimeout)
 		{
 			if (!do_checkpoint)
-				PendingCheckpointerStats.timed_checkpoints++;
+				chkpt_or_rstpt_timed = true;
 			do_checkpoint = true;
 			flags |= CHECKPOINT_CAUSE_TIME;
 		}
@@ -418,6 +420,24 @@ CheckpointerMain(void)
 			if (flags & CHECKPOINT_END_OF_RECOVERY)
 				do_restartpoint = false;
 
+			if (chkpt_or_rstpt_requested)
+			{
+				chkpt_or_rstpt_requested = false;
+				if (do_restartpoint)
+					PendingCheckpointerStats.requested_restartpoints++;
+				else
+					PendingCheckpointerStats.requested_checkpoints++;
+			}
+
+			if (chkpt_or_rstpt_timed)
+			{
+				chkpt_or_rstpt_timed = false;
+				if (do_restartpoint)
+					PendingCheckpointerStats.timed_restartpoints++;
+				else
+					PendingCheckpointerStats.timed_checkpoints++;
+			}
+
 			/*
 			 * We will warn if (a) too soon since last checkpoint (whatever
 			 * caused it) and (b) somebody set the CHECKPOINT_CAUSE_XLOG flag
@@ -481,6 +501,9 @@ CheckpointerMain(void)
 				 * checkpoints happen at a predictable spacing.
 				 */
 				last_checkpoint_time = now;
+
+				if (do_restartpoint)
+					PendingCheckpointerStats.performed_restartpoints++;
 			}
 			else
 			{
diff --git a/src/backend/utils/activity/pgstat_checkpointer.c b/src/backend/utils/activity/pgstat_checkpointer.c
index af8d513e7b..9de99a2cf1 100644
--- a/src/backend/utils/activity/pgstat_checkpointer.c
+++ b/src/backend/utils/activity/pgstat_checkpointer.c
@@ -49,6 +49,9 @@ pgstat_report_checkpointer(void)
 #define CHECKPOINTER_ACC(fld) stats_shmem->stats.fld += PendingCheckpointerStats.fld
 	CHECKPOINTER_ACC(timed_checkpoints);
 	CHECKPOINTER_ACC(requested_checkpoints);
+	CHECKPOINTER_ACC(timed_restartpoints);
+	CHECKPOINTER_ACC(requested_restartpoints);
+	CHECKPOINTER_ACC(performed_restartpoints);
 	CHECKPOINTER_ACC(checkpoint_write_time);
 	CHECKPOINTER_ACC(checkpoint_sync_time);
 	CHECKPOINTER_ACC(buf_written_checkpoints);
@@ -112,6 +115,9 @@ pgstat_checkpointer_snapshot_cb(void)
 #define CHECKPOINTER_COMP(fld) pgStatLocal.snapshot.checkpointer.fld -= reset.fld;
 	CHECKPOINTER_COMP(timed_checkpoints);
 	CHECKPOINTER_COMP(requested_checkpoints);
+	CHECKPOINTER_COMP(timed_restartpoints);
+	CHECKPOINTER_COMP(requested_restartpoints);
+	CHECKPOINTER_COMP(performed_restartpoints);
 	CHECKPOINTER_COMP(checkpoint_write_time);
 	CHECKPOINTER_COMP(checkpoint_sync_time);
 	CHECKPOINTER_COMP(buf_written_checkpoints);
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 25a159b5e5..4c6b98dcce 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1138,6 +1138,24 @@ pg_stat_get_bgwriter_requested_checkpoints(PG_FUNCTION_ARGS)
 	PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->requested_checkpoints);
 }
 
+Datum
+pg_stat_get_bgwriter_timed_restartpoints(PG_FUNCTION_ARGS)
+{
+	PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->timed_restartpoints);
+}
+
+Datum
+pg_stat_get_bgwriter_requested_restartpoints(PG_FUNCTION_ARGS)
+{
+	PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->requested_restartpoints);
+}
+
+Datum
+pg_stat_get_bgwriter_performed_restartpoints(PG_FUNCTION_ARGS)
+{
+	PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->performed_restartpoints);
+}
+
 Datum
 pg_stat_get_bgwriter_buf_written_checkpoints(PG_FUNCTION_ARGS)
 {
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index f9301b2627..5d31ce40f7 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5637,6 +5637,21 @@
   proname => 'pg_stat_get_bgwriter_requested_checkpoints', provolatile => 's',
   proparallel => 'r', prorettype => 'int8', proargtypes => '',
   prosrc => 'pg_stat_get_bgwriter_requested_checkpoints' },
+{ oid => '9938',
+  descr => 'statistics: number of timed restartpoints started by the bgwriter',
+  proname => 'pg_stat_get_bgwriter_timed_restartpoints', provolatile => 's',
+  proparallel => 'r', prorettype => 'int8', proargtypes => '',
+  prosrc => 'pg_stat_get_bgwriter_timed_restartpoints' },
+{ oid => '9939',
+  descr => 'statistics: number of backend requested restartpoints started by the bgwriter',
+  proname => 'pg_stat_get_bgwriter_requested_restartpoints', provolatile => 's',
+  proparallel => 'r', prorettype => 'int8', proargtypes => '',
+  prosrc => 'pg_stat_get_bgwriter_requested_restartpoints' },
+{ oid => '9940',
+  descr => 'statistics: number of backend performed restartpoints started by the bgwriter',
+  proname => 'pg_stat_get_bgwriter_performed_restartpoints', provolatile => 's',
+  proparallel => 'r', prorettype => 'int8', proargtypes => '',
+  prosrc => 'pg_stat_get_bgwriter_performed_restartpoints' },
 { oid => '2771',
   descr => 'statistics: number of buffers written by the bgwriter during checkpoints',
   proname => 'pg_stat_get_bgwriter_buf_written_checkpoints', provolatile => 's',
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index a3df8d27c3..cfc384ec5b 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -269,6 +269,9 @@ typedef struct PgStat_CheckpointerStats
 {
 	PgStat_Counter timed_checkpoints;
 	PgStat_Counter requested_checkpoints;
+	PgStat_Counter timed_restartpoints;
+	PgStat_Counter requested_restartpoints;
+	PgStat_Counter performed_restartpoints;
 	PgStat_Counter checkpoint_write_time;	/* times in milliseconds */
 	PgStat_Counter checkpoint_sync_time;
 	PgStat_Counter buf_written_checkpoints;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index fb9f936d43..0809264ad5 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1816,6 +1816,9 @@ pg_stat_archiver| SELECT s.archived_count,
    FROM pg_stat_get_archiver() s(archived_count, last_archived_wal, last_archived_time, failed_count, last_failed_wal, last_failed_time, stats_reset);
 pg_stat_bgwriter| SELECT pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints_timed,
     pg_stat_get_bgwriter_requested_checkpoints() AS checkpoints_req,
+    pg_stat_get_bgwriter_timed_restartpoints() AS restartpoints_timed,
+    pg_stat_get_bgwriter_requested_restartpoints() AS restartpoints_req,
+    pg_stat_get_bgwriter_performed_restartpoints() AS restartpoints_done,
     pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
     pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
     pg_stat_get_bgwriter_buf_written_checkpoints() AS buffers_checkpoint,