From aaeabb2896c5c12f37845ddd0179f8c6a62a8095 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Mon, 3 Jan 2022 15:01:10 -0500
Subject: [PATCH 7/8] Remove superfluous bgwriter stats code

After adding io_path_stats to PgBackendStatus, all backends keep track
of all IO done on all types of IO paths. When backends exit, they send
their IO operations stats to the stats collector to be persisted.

These statistics are available in the pg_stat_buffers view, making the
buffers_checkpoint, buffers_clean, buffers_backend,
buffers_backend_fsync, and buffers_alloc columns in the pg_stat_bgwriter
view redundant.

In order to maintain backward compatability, these columns in
pg_stat_bgwriter remain and are derived from the pg_stat_buffers view.

The structs used to track the statistics for these columns in the
pg_stat_bgwriter view and the functions querying them have been removed.

Additionally, since the "buffers" stats reset target resets both the IO
operations stats structs and the bgwriter stats structs, this member of
the bgwriter stats structs is no longer needed. Instead derive the
stats_reset column in the pg_stat_bgwriter view from pg_stat_buffers as
well.
---
 src/backend/catalog/system_views.sql  | 28 ++++++++++-----------
 src/backend/postmaster/checkpointer.c | 29 ++-------------------
 src/backend/postmaster/pgstat.c       |  7 ------
 src/backend/storage/buffer/bufmgr.c   |  6 -----
 src/backend/utils/adt/pgstatfuncs.c   | 36 ---------------------------
 src/include/catalog/pg_proc.dat       | 26 -------------------
 src/include/pgstat.h                  |  8 ------
 src/test/regress/expected/rules.out   | 24 +++++++++++++-----
 8 files changed, 34 insertions(+), 130 deletions(-)

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index b6cfe3d3f93..c6f609d9a8b 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1089,20 +1089,6 @@ CREATE VIEW pg_stat_archiver AS
         s.stats_reset
     FROM pg_stat_get_archiver() s;
 
-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_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,
-        pg_stat_get_bgwriter_buf_written_clean() AS buffers_clean,
-        pg_stat_get_bgwriter_maxwritten_clean() AS maxwritten_clean,
-        pg_stat_get_buf_written_backend() AS buffers_backend,
-        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
-        pg_stat_get_buf_alloc() AS buffers_alloc,
-        pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
-
 CREATE VIEW pg_stat_buffers AS
 SELECT
        b.backend_type,
@@ -1114,6 +1100,20 @@ SELECT
        b.stats_reset
 FROM pg_stat_get_buffers() b;
 
+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_checkpoint_write_time() AS checkpoint_write_time,
+        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
+        (SELECT write FROM pg_stat_buffers WHERE backend_type = 'checkpointer' AND io_path = 'shared') AS buffers_checkpoint,
+        (SELECT write FROM pg_stat_buffers WHERE backend_type = 'background writer' AND io_path = 'shared') AS buffers_clean,
+        pg_stat_get_bgwriter_maxwritten_clean() AS maxwritten_clean,
+        (SELECT write FROM pg_stat_buffers WHERE backend_type = 'client backend' AND io_path = 'shared') AS buffers_backend,
+        (SELECT fsync FROM pg_stat_buffers WHERE backend_type = 'client backend' AND io_path = 'shared') AS buffers_backend_fsync,
+        (SELECT alloc FROM pg_stat_buffers WHERE backend_type = 'client backend' AND io_path = 'shared') AS buffers_alloc,
+        (SELECT stats_reset FROM pg_stat_buffers LIMIT 1) AS stats_reset;
+
 CREATE VIEW pg_stat_wal AS
     SELECT
         w.wal_records,
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 061775de4ed..90020cc899e 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -91,17 +91,9 @@
  * requesting backends since the last checkpoint start.  The flags are
  * chosen so that OR'ing is the correct way to combine multiple requests.
  *
- * num_backend_writes is used to count the number of buffer writes performed
- * by user backend processes.  This counter should be wide enough that it
- * can't overflow during a single processing cycle.  num_backend_fsync
- * counts the subset of those writes that also had to do their own fsync,
- * because the checkpointer failed to absorb their request.
- *
  * The requests array holds fsync requests sent by backends and not yet
  * absorbed by the checkpointer.
  *
- * Unlike the checkpoint fields, num_backend_writes, num_backend_fsync, and
- * the requests fields are protected by CheckpointerCommLock.
  *----------
  */
 typedef struct
@@ -125,9 +117,6 @@ typedef struct
 	ConditionVariable start_cv; /* signaled when ckpt_started advances */
 	ConditionVariable done_cv;	/* signaled when ckpt_done advances */
 
-	uint32		num_backend_writes; /* counts user backend buffer writes */
-	uint32		num_backend_fsync;	/* counts user backend fsync calls */
-
 	int			num_requests;	/* current # of requests */
 	int			max_requests;	/* allocated array size */
 	CheckpointerRequest requests[FLEXIBLE_ARRAY_MEMBER];
@@ -1092,10 +1081,6 @@ ForwardSyncRequest(const FileTag *ftag, SyncRequestType type)
 
 	LWLockAcquire(CheckpointerCommLock, LW_EXCLUSIVE);
 
-	/* Count all backend writes regardless of if they fit in the queue */
-	if (!AmBackgroundWriterProcess())
-		CheckpointerShmem->num_backend_writes++;
-
 	/*
 	 * If the checkpointer isn't running or the request queue is full, the
 	 * backend will have to perform its own fsync request.  But before forcing
@@ -1105,13 +1090,12 @@ ForwardSyncRequest(const FileTag *ftag, SyncRequestType type)
 		(CheckpointerShmem->num_requests >= CheckpointerShmem->max_requests &&
 		 !CompactCheckpointerRequestQueue()))
 	{
+		LWLockRelease(CheckpointerCommLock);
+
 		/*
 		 * Count the subset of writes where backends have to do their own
 		 * fsync
 		 */
-		if (!AmBackgroundWriterProcess())
-			CheckpointerShmem->num_backend_fsync++;
-		LWLockRelease(CheckpointerCommLock);
 		pgstat_inc_ioop(IOOP_FSYNC, IOPATH_SHARED);
 		return false;
 	}
@@ -1268,15 +1252,6 @@ AbsorbSyncRequests(void)
 
 	LWLockAcquire(CheckpointerCommLock, LW_EXCLUSIVE);
 
-	/* Transfer stats counts into pending pgstats message */
-	PendingCheckpointerStats.m_buf_written_backend
-		+= CheckpointerShmem->num_backend_writes;
-	PendingCheckpointerStats.m_buf_fsync_backend
-		+= CheckpointerShmem->num_backend_fsync;
-
-	CheckpointerShmem->num_backend_writes = 0;
-	CheckpointerShmem->num_backend_fsync = 0;
-
 	/*
 	 * We try to avoid holding the lock for a long time by copying the request
 	 * array, and processing the requests after releasing the lock.
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index a48998fe944..6c2388dcf7e 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -2653,7 +2653,6 @@ pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep)
 	 * existing statsfile).
 	 */
 	ts = GetCurrentTimestamp();
-	globalStats.bgwriter.stat_reset_timestamp = ts;
 	globalStats.buffers.stat_reset_timestamp = ts;
 	archiverStats.stat_reset_timestamp = ts;
 	walStats.stat_reset_timestamp = ts;
@@ -3809,7 +3808,6 @@ pgstat_recv_resetsharedcounter(PgStat_MsgResetsharedcounter *msg, int len)
 		memset(&globalStats.bgwriter, 0, sizeof(globalStats.bgwriter));
 		memset(&globalStats.checkpointer, 0, sizeof(globalStats.checkpointer));
 		memset(&globalStats.buffers.ops, 0, sizeof(globalStats.buffers.ops));
-		globalStats.bgwriter.stat_reset_timestamp = GetCurrentTimestamp();
 		globalStats.buffers.stat_reset_timestamp = GetCurrentTimestamp();
 		memcpy(&globalStats.buffers.resets[backend_type_get_idx(backend_type)],
 				&msg->m_backend_resets.iop.io_path_ops,
@@ -4089,9 +4087,7 @@ pgstat_recv_archiver(PgStat_MsgArchiver *msg, int len)
 static void
 pgstat_recv_bgwriter(PgStat_MsgBgWriter *msg, int len)
 {
-	globalStats.bgwriter.buf_written_clean += msg->m_buf_written_clean;
 	globalStats.bgwriter.maxwritten_clean += msg->m_maxwritten_clean;
-	globalStats.bgwriter.buf_alloc += msg->m_buf_alloc;
 }
 
 /*
@@ -4104,9 +4100,6 @@ pgstat_recv_checkpointer(PgStat_MsgCheckpointer *msg, int len)
 	globalStats.checkpointer.requested_checkpoints += msg->m_requested_checkpoints;
 	globalStats.checkpointer.checkpoint_write_time += msg->m_checkpoint_write_time;
 	globalStats.checkpointer.checkpoint_sync_time += msg->m_checkpoint_sync_time;
-	globalStats.checkpointer.buf_written_checkpoints += msg->m_buf_written_checkpoints;
-	globalStats.checkpointer.buf_written_backend += msg->m_buf_written_backend;
-	globalStats.checkpointer.buf_fsync_backend += msg->m_buf_fsync_backend;
 }
 
 static void
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index a3136a589fa..13f0c0aac10 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -2170,7 +2170,6 @@ BufferSync(int flags)
 			if (SyncOneBuffer(buf_id, false, &wb_context) & BUF_WRITTEN)
 			{
 				TRACE_POSTGRESQL_BUFFER_SYNC_WRITTEN(buf_id);
-				PendingCheckpointerStats.m_buf_written_checkpoints++;
 				num_written++;
 			}
 		}
@@ -2279,9 +2278,6 @@ BgBufferSync(WritebackContext *wb_context)
 	 */
 	strategy_buf_id = StrategySyncStart(&strategy_passes, &recent_alloc);
 
-	/* Report buffer alloc counts to pgstat */
-	PendingBgWriterStats.m_buf_alloc += recent_alloc;
-
 	/*
 	 * If we're not running the LRU scan, just stop after doing the stats
 	 * stuff.  We mark the saved state invalid so that we can recover sanely
@@ -2478,8 +2474,6 @@ BgBufferSync(WritebackContext *wb_context)
 			reusable_buffers++;
 	}
 
-	PendingBgWriterStats.m_buf_written_clean += num_written;
-
 #ifdef BGW_DEBUG
 	elog(DEBUG1, "bgwriter: recent_alloc=%u smoothed=%.2f delta=%ld ahead=%d density=%.2f reusable_est=%d upcoming_est=%d scanned=%d wrote=%d reusable=%d",
 		 recent_alloc, smoothed_alloc, strategy_delta, bufs_ahead,
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index e1213a9ad03..0a448ba4ed9 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1681,18 +1681,6 @@ pg_stat_get_bgwriter_requested_checkpoints(PG_FUNCTION_ARGS)
 	PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->requested_checkpoints);
 }
 
-Datum
-pg_stat_get_bgwriter_buf_written_checkpoints(PG_FUNCTION_ARGS)
-{
-	PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->buf_written_checkpoints);
-}
-
-Datum
-pg_stat_get_bgwriter_buf_written_clean(PG_FUNCTION_ARGS)
-{
-	PG_RETURN_INT64(pgstat_fetch_stat_bgwriter()->buf_written_clean);
-}
-
 Datum
 pg_stat_get_bgwriter_maxwritten_clean(PG_FUNCTION_ARGS)
 {
@@ -1715,30 +1703,6 @@ pg_stat_get_checkpoint_sync_time(PG_FUNCTION_ARGS)
 					 pgstat_fetch_stat_checkpointer()->checkpoint_sync_time);
 }
 
-Datum
-pg_stat_get_bgwriter_stat_reset_time(PG_FUNCTION_ARGS)
-{
-	PG_RETURN_TIMESTAMPTZ(pgstat_fetch_stat_bgwriter()->stat_reset_timestamp);
-}
-
-Datum
-pg_stat_get_buf_written_backend(PG_FUNCTION_ARGS)
-{
-	PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->buf_written_backend);
-}
-
-Datum
-pg_stat_get_buf_fsync_backend(PG_FUNCTION_ARGS)
-{
-	PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->buf_fsync_backend);
-}
-
-Datum
-pg_stat_get_buf_alloc(PG_FUNCTION_ARGS)
-{
-	PG_RETURN_INT64(pgstat_fetch_stat_bgwriter()->buf_alloc);
-}
-
 /*
 * When adding a new column to the pg_stat_buffers view, add a new enum
 * value here above BUFFERS_NUM_COLUMNS.
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 2a3f11c26e9..0af63b50d15 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5599,25 +5599,11 @@
   proname => 'pg_stat_get_bgwriter_requested_checkpoints', provolatile => 's',
   proparallel => 'r', prorettype => 'int8', proargtypes => '',
   prosrc => 'pg_stat_get_bgwriter_requested_checkpoints' },
-{ oid => '2771',
-  descr => 'statistics: number of buffers written by the bgwriter during checkpoints',
-  proname => 'pg_stat_get_bgwriter_buf_written_checkpoints', provolatile => 's',
-  proparallel => 'r', prorettype => 'int8', proargtypes => '',
-  prosrc => 'pg_stat_get_bgwriter_buf_written_checkpoints' },
-{ 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',
-  proparallel => 'r', prorettype => 'int8', proargtypes => '',
-  prosrc => 'pg_stat_get_bgwriter_buf_written_clean' },
 { oid => '2773',
   descr => 'statistics: number of times the bgwriter stopped processing when it had written too many buffers while cleaning',
   proname => 'pg_stat_get_bgwriter_maxwritten_clean', provolatile => 's',
   proparallel => 'r', prorettype => 'int8', proargtypes => '',
   prosrc => 'pg_stat_get_bgwriter_maxwritten_clean' },
-{ oid => '3075', descr => 'statistics: last reset for the bgwriter',
-  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 time spent writing buffers to disk, in milliseconds',
   proname => 'pg_stat_get_checkpoint_write_time', provolatile => 's',
@@ -5628,18 +5614,6 @@
   proname => 'pg_stat_get_checkpoint_sync_time', provolatile => 's',
   proparallel => 'r', prorettype => 'float8', proargtypes => '',
   prosrc => 'pg_stat_get_checkpoint_sync_time' },
-{ oid => '2775', descr => 'statistics: number of buffers written by backends',
-  proname => 'pg_stat_get_buf_written_backend', provolatile => 's',
-  proparallel => 'r', prorettype => 'int8', proargtypes => '',
-  prosrc => 'pg_stat_get_buf_written_backend' },
-{ oid => '3063',
-  descr => 'statistics: number of backend buffer writes that did their own fsync',
-  proname => 'pg_stat_get_buf_fsync_backend', provolatile => 's',
-  proparallel => 'r', prorettype => 'int8', proargtypes => '',
-  prosrc => 'pg_stat_get_buf_fsync_backend' },
-{ 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' },
 
 { oid => '8459', descr => 'statistics: counts of all IO operations done through all IO paths by each type of backend.',
   proname => 'pg_stat_get_buffers', provolatile => 's', proisstrict => 'f',
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index ca96f7c3e4c..63075a6b358 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -573,9 +573,7 @@ typedef struct PgStat_MsgBgWriter
 {
 	PgStat_MsgHdr m_hdr;
 
-	PgStat_Counter m_buf_written_clean;
 	PgStat_Counter m_maxwritten_clean;
-	PgStat_Counter m_buf_alloc;
 } PgStat_MsgBgWriter;
 
 /* ----------
@@ -588,9 +586,6 @@ typedef struct PgStat_MsgCheckpointer
 
 	PgStat_Counter m_timed_checkpoints;
 	PgStat_Counter m_requested_checkpoints;
-	PgStat_Counter m_buf_written_checkpoints;
-	PgStat_Counter m_buf_written_backend;
-	PgStat_Counter m_buf_fsync_backend;
 	PgStat_Counter m_checkpoint_write_time; /* times in milliseconds */
 	PgStat_Counter m_checkpoint_sync_time;
 } PgStat_MsgCheckpointer;
@@ -858,10 +853,7 @@ typedef struct PgStat_ArchiverStats
 
 typedef struct PgStat_BgWriterStats
 {
-	PgStat_Counter buf_written_clean;
 	PgStat_Counter maxwritten_clean;
-	PgStat_Counter buf_alloc;
-	TimestampTz stat_reset_timestamp;
 } PgStat_BgWriterStats;
 
 typedef struct PgStat_CheckpointerStats
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 39d0ed45642..47f54d4a929 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1813,13 +1813,25 @@ pg_stat_bgwriter| SELECT pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints
     pg_stat_get_bgwriter_requested_checkpoints() AS checkpoints_req,
     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,
-    pg_stat_get_bgwriter_buf_written_clean() AS buffers_clean,
+    ( SELECT pg_stat_buffers.write
+           FROM pg_stat_buffers
+          WHERE ((pg_stat_buffers.backend_type = 'checkpointer'::text) AND (pg_stat_buffers.io_path = 'shared'::text))) AS buffers_checkpoint,
+    ( SELECT pg_stat_buffers.write
+           FROM pg_stat_buffers
+          WHERE ((pg_stat_buffers.backend_type = 'background writer'::text) AND (pg_stat_buffers.io_path = 'shared'::text))) AS buffers_clean,
     pg_stat_get_bgwriter_maxwritten_clean() AS maxwritten_clean,
-    pg_stat_get_buf_written_backend() AS buffers_backend,
-    pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
-    pg_stat_get_buf_alloc() AS buffers_alloc,
-    pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
+    ( SELECT pg_stat_buffers.write
+           FROM pg_stat_buffers
+          WHERE ((pg_stat_buffers.backend_type = 'client backend'::text) AND (pg_stat_buffers.io_path = 'shared'::text))) AS buffers_backend,
+    ( SELECT pg_stat_buffers.fsync
+           FROM pg_stat_buffers
+          WHERE ((pg_stat_buffers.backend_type = 'client backend'::text) AND (pg_stat_buffers.io_path = 'shared'::text))) AS buffers_backend_fsync,
+    ( SELECT pg_stat_buffers.alloc
+           FROM pg_stat_buffers
+          WHERE ((pg_stat_buffers.backend_type = 'client backend'::text) AND (pg_stat_buffers.io_path = 'shared'::text))) AS buffers_alloc,
+    ( SELECT pg_stat_buffers.stats_reset
+           FROM pg_stat_buffers
+         LIMIT 1) AS stats_reset;
 pg_stat_buffers| SELECT b.backend_type,
     b.io_path,
     b.alloc,
-- 
2.17.1

