On 2023-11-09 16:28, Michael Paquier wrote:
Thanks for your review.
Attached v2 patch.
On Thu, Nov 09, 2023 at 01:50:34PM +0900, torikoshia wrote:
PGSTAT_KIND_SLRU cannot be reset by pg_stat_reset_shared(), so I feel
uncomfortable to delete it all together.
It might be better after pg_stat_reset_shared() has been modified to
take
'slru' as an argument, though.
Not sure how to feel about that, TBH, but I would not include SLRUs
here if we have already a separate function.
IMHO I agree with you.
I thought it would be better to reset statistics even when null is
specified
so that users are not confused with the behavior of
pg_stat_reset_slru().
Attached patch added pg_stat_reset_shared() in system_functions.sql
mainly
for this reason.
I'm OK with doing what your patch does, aka do the work if the value
is NULL or if there's no argument given.
- Resets some cluster-wide statistics counters to zero,
depending on the
+ Resets cluster-wide statistics counters to zero, depending on
the
This does not need to change, aka SLRUs are for example still global
and not included here.
+ If the argument is NULL or not specified, all counters shown
in all
+ of these views are reset.
Missing a <literal> markup around NULL. I know, we're not consistent
about that, either, but if we are tweaking the area let's be right at
least. Perhaps "all the counters from the views listed above are
reset"?
--
Michael
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
From f01ab1071706bdd472fe6063160379a721763a48 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikos...@oss.nttdata.com>
Date: Fri, 10 Nov 2023 11:56:40 +0900
Subject: [PATCH v2] Add ability to reset all statistics to
pg_stat_reset_shared()
Currently pg_stat_reset_shared() requires its argument to specify
statistics to be reset, and there is no way to reset all statistics
which can be specified its argument.
This patch makes it possible when no argument or NULL is specified.
In effect this API equals to call pg_sat_reset_shared() with every
argument, but it requires callers to know the set of possible values.
Considering nearly all the time the right thing to do is to reset all
shared stats, this API would be consistent with the way user reset
stats.
---
doc/src/sgml/monitoring.sgml | 2 ++
src/backend/catalog/system_functions.sql | 7 +++++++
src/backend/utils/adt/pgstatfuncs.c | 17 ++++++++++++++++-
src/include/catalog/pg_proc.dat | 5 +++--
src/test/regress/expected/stats.out | 14 +++++++-------
src/test/regress/sql/stats.sql | 14 +++++++-------
6 files changed, 42 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index e068f7e247..16a54179ae 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -4730,6 +4730,8 @@ description | Waiting for a newly initialized WAL file to reach durable storage
<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 the argument is <literal>NULL</literal> or not specified, all
+ the counters from the views listed above are reset.
</para>
<para>
This function is restricted to superusers by default, but other users
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index 35d738d576..8079f1cd7f 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -621,6 +621,13 @@ LANGUAGE internal
STRICT IMMUTABLE PARALLEL SAFE
AS 'unicode_is_normalized';
+CREATE OR REPLACE FUNCTION
+ pg_stat_reset_shared(target text DEFAULT NULL)
+RETURNS void
+LANGUAGE INTERNAL
+CALLED ON NULL INPUT VOLATILE PARALLEL SAFE
+AS 'pg_stat_reset_shared';
+
--
-- The default permissions for functions mean that anyone can execute them.
-- A number of functions shouldn't be executable by just anyone, but rather
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 1fb8b31863..a3a92c7b67 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1685,7 +1685,22 @@ pg_stat_reset(PG_FUNCTION_ARGS)
Datum
pg_stat_reset_shared(PG_FUNCTION_ARGS)
{
- char *target = text_to_cstring(PG_GETARG_TEXT_PP(0));
+ char *target = NULL;
+
+ if (PG_ARGISNULL(0))
+ {
+ /* Reset all the statistics which can be specified by the argument */
+ pgstat_reset_of_kind(PGSTAT_KIND_ARCHIVER);
+ pgstat_reset_of_kind(PGSTAT_KIND_BGWRITER);
+ pgstat_reset_of_kind(PGSTAT_KIND_CHECKPOINTER);
+ pgstat_reset_of_kind(PGSTAT_KIND_IO);
+ pgstat_reset_of_kind(PGSTAT_KIND_WAL);
+ XLogPrefetchResetStats();
+
+ PG_RETURN_VOID();
+ }
+
+ target = text_to_cstring(PG_GETARG_TEXT_PP(0));
if (strcmp(target, "archiver") == 0)
pgstat_reset_of_kind(PGSTAT_KIND_ARCHIVER);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index f14aed422a..d36144fbc6 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5879,10 +5879,11 @@
descr => 'statistics: reset collected statistics for current database',
proname => 'pg_stat_reset', proisstrict => 'f', provolatile => 'v',
prorettype => 'void', proargtypes => '', prosrc => 'pg_stat_reset' },
+
{ oid => '3775',
descr => 'statistics: reset collected statistics shared across the cluster',
- proname => 'pg_stat_reset_shared', provolatile => 'v', prorettype => 'void',
- proargtypes => 'text', prosrc => 'pg_stat_reset_shared' },
+ proname => 'pg_stat_reset_shared', proisstrict => 'f', provolatile => 'v',
+ prorettype => 'void', proargtypes => 'text', prosrc => 'pg_stat_reset_shared' },
{ oid => '3776',
descr => 'statistics: reset collected statistics for a single table or index in the current database or shared across all databases in the cluster',
proname => 'pg_stat_reset_single_table_counters', provolatile => 'v',
diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out
index 494cef07d3..dfcb451543 100644
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -975,38 +975,38 @@ SELECT stats_reset > :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
(1 row)
SELECT stats_reset AS wal_reset_ts FROM pg_stat_wal \gset
--- Test that reset_shared with no specified stats type doesn't reset anything
-SELECT pg_stat_reset_shared(NULL);
+-- Test that all shared stats are reset when no argument provided to reset function
+SELECT pg_stat_reset_shared();
pg_stat_reset_shared
----------------------
(1 row)
-SELECT stats_reset = :'archiver_reset_ts'::timestamptz FROM pg_stat_archiver;
+SELECT stats_reset > :'archiver_reset_ts'::timestamptz FROM pg_stat_archiver;
?column?
----------
t
(1 row)
-SELECT stats_reset = :'bgwriter_reset_ts'::timestamptz FROM pg_stat_bgwriter;
+SELECT stats_reset > :'bgwriter_reset_ts'::timestamptz FROM pg_stat_bgwriter;
?column?
----------
t
(1 row)
-SELECT stats_reset = :'checkpointer_reset_ts'::timestamptz FROM pg_stat_checkpointer;
+SELECT stats_reset > :'checkpointer_reset_ts'::timestamptz FROM pg_stat_checkpointer;
?column?
----------
t
(1 row)
-SELECT stats_reset = :'recovery_prefetch_reset_ts'::timestamptz FROM pg_stat_recovery_prefetch;
+SELECT stats_reset > :'recovery_prefetch_reset_ts'::timestamptz FROM pg_stat_recovery_prefetch;
?column?
----------
t
(1 row)
-SELECT stats_reset = :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
+SELECT stats_reset > :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
?column?
----------
t
diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index 7ae8b8a276..3d6cbbafb5 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -488,13 +488,13 @@ SELECT pg_stat_reset_shared('wal');
SELECT stats_reset > :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
SELECT stats_reset AS wal_reset_ts FROM pg_stat_wal \gset
--- Test that reset_shared with no specified stats type doesn't reset anything
-SELECT pg_stat_reset_shared(NULL);
-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 = :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
+-- Test that all shared stats are reset when no argument provided to reset function
+SELECT pg_stat_reset_shared();
+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 > :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
-- Test error case for reset_shared with unknown stats type
SELECT pg_stat_reset_shared('unknown');
base-commit: 5ba1ac99a8d8623604d3152be8fd9a201ba5240b
--
2.39.2
