[torikoshia <torikoshia@oss.nttdata.com>]

On 2023-11-13 13:15, torikoshia wrote:
> On 2023-11-12 16:46, Michael Paquier wrote:
>> On Fri, Nov 10, 2023 at 01:15:50PM +0900, Michael Paquier wrote:
>>> The comments added could be better grammatically, but basically LGTM.
>>> I'll take care of that if there are no objections.
>> 
>> The documentation also needed a few tweaks (for DEFAULT and the
>> argument name), so I have fixed the whole and adapted the new part of
>> the docs to that, with few little tweaks.
> 
> Thanks!
> 
> I assume you have already taken this into account, but I think we
> should add the same documentation to the below patch for
> pg_stat_reset_slru():
> 
> 
> https://www.postgresql.org/message-id/CALj2ACW4Fqc_m%2BOaavrOMEivZ5aBa24pVKvoXRTmuFECsNBfAg%40mail.g...
> 
> On 2023-11-12 16:54, Michael Paquier wrote:
>> On Fri, Nov 10, 2023 at 08:32:34PM +0900, torikoshia wrote:
>>> On 2023-11-10 13:18, Andres Freund wrote:
>>>> I see no reason to not include slrus. We should never have added the
>>>> ability to reset them individually, particularly not without a use
>>>> case - I couldn't find one skimming some discussion. And what's the
>>>> point in not allowing to reset them via pg_stat_reset_shared()?
>>> 
>>> When including SLRUs, do you think it's better to add 'slrus' 
>>> argument which
>>> enables pg_stat_reset_shared() to reset all SLRUs?
>> 
>> I understand that Andres says that he'd be OK with a addition of a
>> 'slru' option in pg_stat_reset_shared(), as well as including SLRUs in
>> the resets if everything should be wiped.
> 
> Thanks, I'll make the patch.

Attached patch.

BTW currently the documentation explains all the arguments of 
pg_stat_reset_shared() in one line and I feel it's a bit hard to read.
Attached patch uses <itemizedlist>.

What do you think?


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

Attachments:

v1-0001-Add-ability-to-reset-pg_stat_slru-in-pg_stat_rese.patch (text/x-diff, 7.9K) download
From 31086c6bdbab496ca6ced88c9069213f07f1ca5e Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Tue, 14 Nov 2023 21:17:53 +0900
Subject: [PATCH v1] Add ability to reset pg_stat_slru in
 pg_stat_reset_shared()

Currently, pg_stat_reset_shared() can not reset pg_stat_slru
nevertheless this is one of shared statistics view.

This patch adds a new argment 'slru' to reset all the rows
in pg_stat_slru.
When the target is NULL or not specified, this patch also
reset pg_stat_slru.

Considering pg_stat_reset_slru() was introduced in
28cac71bd368(PostgreSQL 13) and it could impact existing
applications and it can not only reset all the entry but
can specify which entry to reset, this function is left
as it is.
---
 doc/src/sgml/monitoring.sgml        | 67 ++++++++++++++++++++++-------
 src/backend/utils/adt/pgstatfuncs.c |  3 ++
 src/test/regress/expected/stats.out | 35 +++++++++++++++
 src/test/regress/sql/stats.sql      |  7 +++
 4 files changed, 96 insertions(+), 16 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index b8495b6498..c089adb170 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -4717,22 +4717,57 @@ description | Waiting for a newly initialized WAL file to reach durable storage
        </para>
        <para>
         Resets some cluster-wide statistics counters to zero, depending on the
-        argument.  The argument can be <literal>bgwriter</literal> to reset
-        all the counters shown in
-        the <structname>pg_stat_bgwriter</structname> view,
-        <literal>checkpointer</literal> to reset all the counters shown in
-        the <structname>pg_stat_checkpointer</structname> view,
-        <literal>archiver</literal> to reset all the counters shown in
-        the <structname>pg_stat_archiver</structname> view,
-        <literal>io</literal> to reset all the counters shown in the
-        <structname>pg_stat_io</structname> view,
-        <literal>wal</literal> to reset all the counters shown in the
-        <structname>pg_stat_wal</structname> view or
-        <literal>recovery_prefetch</literal> to reset all the counters shown
-        in the <structname>pg_stat_recovery_prefetch</structname> view.
-        If <parameter>target</parameter> is <literal>NULL</literal> or
-        is not specified, all the counters from the views listed above are
-        reset.
+        argument.  <parameter>target</parameter> can be:
+       <itemizedlist>
+        <listitem>
+         <para>
+          <literal>archiver</literal>: Reset all the counters shown in the
+          <structname>pg_stat_archiver</structname> view.
+         </para>
+        </listitem>
+        <listitem>
+         <para>
+           <literal>bgwriter</literal>: Reset all the counters shown in the
+           <structname>pg_stat_bgwriter</structname> view.
+         </para>
+        </listitem>
+        <listitem>
+         <para>
+          <literal>checkpointer</literal>: Reset all the counters shown in the
+          <structname>pg_stat_checkpointer</structname> view.
+         </para>
+        </listitem>
+        <listitem>
+         <para>
+          <literal>io</literal>: Reset all the counters shown in the
+          <structname>pg_stat_io</structname> view.
+         </para>
+        </listitem>
+        <listitem>
+         <para>
+          <literal>recovery_prefetch</literal>: Reset all the counters shown in
+          the <structname>pg_stat_recovery_prefetch</structname> view.
+         </para>
+        </listitem>
+        <listitem>
+         <para>
+          <literal>slru</literal>: Reset all the counters shown in the
+          <structname>pg_stat_slru</structname> view.
+         </para>
+        </listitem>
+        <listitem>
+         <para>
+          <literal>wal</literal>: Reset all the counters shown in the
+          <structname>pg_stat_wal</structname> view.
+         </para>
+        </listitem>
+        <listitem>
+         <para>
+          <literal>NULL</literal> or not specified: All the counters from the
+          views listed above are reset.
+         </para>
+        </listitem>
+       </itemizedlist>
        </para>
        <para>
         This function is restricted to superusers by default, but other users
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 3a9f9bc4fe..9cb8210960 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1695,6 +1695,7 @@ pg_stat_reset_shared(PG_FUNCTION_ARGS)
 		pgstat_reset_of_kind(PGSTAT_KIND_CHECKPOINTER);
 		pgstat_reset_of_kind(PGSTAT_KIND_IO);
 		XLogPrefetchResetStats();
+		pgstat_reset_of_kind(PGSTAT_KIND_SLRU);
 		pgstat_reset_of_kind(PGSTAT_KIND_WAL);
 
 		PG_RETURN_VOID();
@@ -1712,6 +1713,8 @@ pg_stat_reset_shared(PG_FUNCTION_ARGS)
 		pgstat_reset_of_kind(PGSTAT_KIND_IO);
 	else if (strcmp(target, "recovery_prefetch") == 0)
 		XLogPrefetchResetStats();
+	else if (strcmp(target, "slru") == 0)
+		pgstat_reset_of_kind(PGSTAT_KIND_SLRU);
 	else if (strcmp(target, "wal") == 0)
 		pgstat_reset_of_kind(PGSTAT_KIND_WAL);
 	else
diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out
index 0694de736a..91e6662a41 100644
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -960,6 +960,28 @@ SELECT stats_reset > :'recovery_prefetch_reset_ts'::timestamptz FROM pg_stat_rec
 (1 row)
 
 SELECT stats_reset AS recovery_prefetch_reset_ts FROM pg_stat_recovery_prefetch \gset
+-- Test that reset_shared with slru specified as the stats type works
+SELECT MAX(stats_reset) AS slru_reset_ts FROM pg_stat_slru \gset
+SELECT pg_stat_reset_shared('slru');
+ pg_stat_reset_shared 
+----------------------
+ 
+(1 row)
+
+SELECT stats_reset > :'slru_reset_ts'::timestamptz FROM pg_stat_slru;
+ ?column? 
+----------
+ t
+ t
+ t
+ t
+ t
+ t
+ t
+ t
+(8 rows)
+
+SELECT MAX(stats_reset) AS slru_reset_ts FROM pg_stat_slru \gset
 -- Test that reset_shared with wal specified as the stats type works
 SELECT stats_reset AS wal_reset_ts FROM pg_stat_wal \gset
 SELECT pg_stat_reset_shared('wal');
@@ -1007,6 +1029,19 @@ SELECT stats_reset > :'recovery_prefetch_reset_ts'::timestamptz FROM pg_stat_rec
  t
 (1 row)
 
+SELECT stats_reset > :'slru_reset_ts'::timestamptz FROM pg_stat_slru;
+ ?column? 
+----------
+ t
+ t
+ t
+ t
+ t
+ t
+ t
+ t
+(8 rows)
+
 SELECT stats_reset > :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
  ?column? 
 ----------
diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index 13db45d4dc..eb3ed74cdb 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -482,6 +482,12 @@ SELECT pg_stat_reset_shared('recovery_prefetch');
 SELECT stats_reset > :'recovery_prefetch_reset_ts'::timestamptz FROM pg_stat_recovery_prefetch;
 SELECT stats_reset AS recovery_prefetch_reset_ts FROM pg_stat_recovery_prefetch \gset
 
+-- Test that reset_shared with slru specified as the stats type works
+SELECT MAX(stats_reset) AS slru_reset_ts FROM pg_stat_slru \gset
+SELECT pg_stat_reset_shared('slru');
+SELECT stats_reset > :'slru_reset_ts'::timestamptz FROM pg_stat_slru;
+SELECT MAX(stats_reset) AS slru_reset_ts FROM pg_stat_slru \gset
+
 -- Test that reset_shared with wal specified as the stats type works
 SELECT stats_reset AS wal_reset_ts FROM pg_stat_wal \gset
 SELECT pg_stat_reset_shared('wal');
@@ -495,6 +501,7 @@ SELECT stats_reset > :'archiver_reset_ts'::timestamptz FROM pg_stat_archiver;
 SELECT stats_reset > :'bgwriter_reset_ts'::timestamptz FROM pg_stat_bgwriter;
 SELECT stats_reset > :'checkpointer_reset_ts'::timestamptz FROM pg_stat_checkpointer;
 SELECT stats_reset > :'recovery_prefetch_reset_ts'::timestamptz FROM pg_stat_recovery_prefetch;
+SELECT stats_reset > :'slru_reset_ts'::timestamptz FROM pg_stat_slru;
 SELECT stats_reset > :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
 
 -- Test error case for reset_shared with unknown stats type

base-commit: 7606175991f8ed5ae36eecf6951cb89dcfb2156e
-- 
2.39.2