From: torikoshia <torikoshia@oss.nttdata.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>
Cc: bharath.rupireddyforpostgres@gmail.com, boekewurm+postgres@gmail.com, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Add new option 'all' to pg_stat_reset_shared()
Date: Fri, 10 Nov 2023 12:33:50 +0900
Message-ID: <31053c5b2110a5fb1871b3068444d1a0@oss.nttdata.com> (raw)
In-Reply-To: <ZUyKCE2l3mEf3d2Q@paquier.xyz>
References: <20231104014905.oiropxau5can5fdu@awork3.anarazel.de>
<e784c0d54c849e86d69138883d12138f@oss.nttdata.com>
<CALj2ACVjpHfkCUWSMfpX-oyuqDCenbbduxNfQWPUBXZ0TChMrg@mail.gmail.com>
<20231108041331.mmxtbr3cmufy4f3v@awork3.anarazel.de>
<CALj2ACW4Fqc_m+OaavrOMEivZ5aBa24pVKvoXRTmuFECsNBfAg@mail.gmail.com>
<ZUwguPB1rTXBK5CT@paquier.xyz>
<62d8244b05557a91e25914c02607b493@oss.nttdata.com>
<ZUw0_tA7g0dkTow8@paquier.xyz>
<20231109012952.fft2i356ofljugt6@awork3.anarazel.de>
<37bce6feca33e0c0441cd33f9eaa51d6@oss.nttdata.com>
<ZUyKCE2l3mEf3d2Q@paquier.xyz>
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
Attachments:v2-0001-Add-ability-to-reset-all-statistics-to-pg_stat_re.patch (text/x-diff, 7.7K) download
From f01ab1071706bdd472fe6063160379a721763a48 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@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.sgmlindex 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.sqlindex 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.cindex 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.datindex 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.outindex 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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: torikoshia@oss.nttdata.com, michael@paquier.xyz, andres@anarazel.de, horikyota.ntt@gmail.com
Subject: Re: Add new option 'all' to pg_stat_reset_shared()
In-Reply-To: <31053c5b2110a5fb1871b3068444d1a0@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox