Re: Add new option 'all' to pg_stat_reset_shared()
On 2023-11-16 16:48, Michael Paquier wrote: On Wed, Nov 15, 2023 at 04:25:14PM +0900, Michael Paquier wrote: Other than that, it looks OK. Tweaked the queries of this one slightly, and applied. So I think that we are now good for this thread. Thanks, all! Thanks for the modification and apply! -- Michael -- Regards, -- Atsushi Torikoshi NTT DATA Group Corporation
Re: Add new option 'all' to pg_stat_reset_shared()
On Wed, Nov 15, 2023 at 04:25:14PM +0900, Michael Paquier wrote: > Other than that, it looks OK. Tweaked the queries of this one slightly, and applied. So I think that we are now good for this thread. Thanks, all! -- Michael signature.asc Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
On Wed, Nov 15, 2023 at 11:58:38AM +0900, torikoshia wrote: > On 2023-11-15 09:47, Michael Paquier wrote: >> You have forgotten to update the errhint at the end of >> pg_stat_reset_shared(), where "slru" needs to be listed :) > > Oops, attached v2 patch. +SELECT stats_reset > :'slru_reset_ts'::timestamptz FROM pg_stat_slru; A problem with these two queries is that they depend on the number of SLRUs set in the system while only returning a single 't' without the cache names each tuple is linked to. To keep things simple, you could just LIMIT 1 or aggregate through the whole set. Other than that, it looks OK. -- Michael signature.asc Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
On 2023-11-15 09:47, Michael Paquier wrote:
On Tue, Nov 14, 2023 at 10:02:32PM +0900, torikoshia wrote:
Attached patch.
You have forgotten to update the errhint at the end of
pg_stat_reset_shared(), where "slru" needs to be listed :)
Oops, attached v2 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 .
Yes, this one is a good idea because each target works on a different
system view so it becomes easier to understand what a target affects,
so I've applied this bit, without the slru addition.
Thanks!
--
Michael
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group CorporationFrom 9283d25cb96c9c4253d7f39c464b61576bd52a52 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi
Date: Wed, 15 Nov 2023 11:47:13 +0900
Subject: [PATCH v2] 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.
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| 6 +
src/backend/utils/adt/pgstatfuncs.c | 5 +++-
src/test/regress/expected/stats.out | 37 -
src/test/regress/sql/stats.sql | 7 ++
4 files changed, 53 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 1e9913c8bc..42509042ad 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -4749,6 +4749,12 @@ description | Waiting for a newly initialized WAL file to reach durable storage
the pg_stat_recovery_prefetch view.
+
+
+ slru: Reset all the counters shown in the
+ pg_stat_slru view.
+
+
wal: Reset all the counters shown in the
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 3a9f9bc4fe..0cea320c00 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,13 +1713,15 @@ 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
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("unrecognized reset target: \"%s\"", target),
- errhint("Target must be \"archiver\", \"bgwriter\", \"checkpointer\", \"io\", \"recovery_prefetch\", or \"wal\".")));
+ errhint("Target must be \"archiver\", \"bgwriter\", \"checkpointer\", \"io\", \"recovery_prefetch\", \"slru\", or \"wal\".")));
PG_RETURN_VOID();
}
diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out
index 6ed3584a76..103ea453df 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?
--
@@ -1016,7 +1051,7 @@ 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');
ERROR:
Re: Add new option 'all' to pg_stat_reset_shared()
On Tue, Nov 14, 2023 at 10:02:32PM +0900, torikoshia wrote: > Attached patch. You have forgotten to update the errhint at the end of pg_stat_reset_shared(), where "slru" needs to be listed :) > 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 . Yes, this one is a good idea because each target works on a different system view so it becomes easier to understand what a target affects, so I've applied this bit, without the slru addition. -- Michael signature.asc Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
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.gmail.com 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 . What do you think? -- Regards, -- Atsushi Torikoshi NTT DATA Group CorporationFrom 31086c6bdbab496ca6ced88c9069213f07f1ca5e Mon Sep 17 00:00:00 2001 From: Atsushi Torikoshi 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 Resets some cluster-wide statistics counters to zero, depending on the -argument. The argument can be bgwriter to reset -all the counters shown in -the pg_stat_bgwriter view, -checkpointer to reset all the counters shown in -the pg_stat_checkpointer view, -archiver to reset all the counters shown in -the pg_stat_archiver view, -io to reset all the counters shown in the -pg_stat_io view, -wal to reset all the counters shown in the -pg_stat_wal view or -recovery_prefetch to reset all the counters shown -in the pg_stat_recovery_prefetch view. -If target is NULL or -is not specified, all the counters from the views listed above are -reset. +argument. target can be: + + + + archiver: Reset all the counters shown in the + pg_stat_archiver view. + + + + + bgwriter: Reset all the counters shown in the + pg_stat_bgwriter view. + + + + + checkpointer: Reset all the counters shown in the + pg_stat_checkpointer view. + + + + + io: Reset all the counters shown in the + pg_stat_io view. + + + + + recovery_prefetch: Reset all the counters shown in + the pg_stat_recovery_prefetch view. + + + + + slru: Reset all the counters shown in the + pg_stat_slru view. + + + + + wal: Reset all the counters shown in the + pg_stat_wal view. + + + + + NULL or not specified: All the counters from the + views listed above are reset. + + + This function is restricted to
Re: Add new option 'all' to pg_stat_reset_shared()
On Mon, Nov 13, 2023 at 07:31:41PM +0900, Michael Paquier wrote: > On Mon, Nov 13, 2023 at 02:07:21PM +0530, Bharath Rupireddy wrote: >> Modified the docs for pg_stat_reset_slru to match with that of >> pg_stat_reset_shared. PSA v2 patch. > > That feels consistent. Thanks. And applied this one. -- Michael signature.asc Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
On Mon, Nov 13, 2023 at 02:07:21PM +0530, Bharath Rupireddy wrote:
> Modified the docs for pg_stat_reset_slru to match with that of
> pg_stat_reset_shared. PSA v2 patch.
That feels consistent. Thanks.
> I noticed that the commit 23c8c0c8 missed to add proargnames =>
> '{target}' in .dat file for pg_stat_reset_shared, is it intentional?
> Naming the argument in system_funtion.sql is enough to be able to pass
> in named arguments like SELECT pg_stat_reset_shared(target := 'io');,
> but is it needed in .dat file as well to keep it consistent?
I don't see a need to do that because, as you say, the functions are
redefined for their default values, meaning that they'll also have
argument names consistent with the docs. There are quite a few like
that in pg_proc.dat like pg_promote, pg_backup_start,
json_populate_record, etc.
--
Michael
signature.asc
Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
On Mon, Nov 13, 2023 at 9:45 AM 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.gmail.com
Modified the docs for pg_stat_reset_slru to match with that of
pg_stat_reset_shared. PSA v2 patch.
I noticed that the commit 23c8c0c8 missed to add proargnames =>
'{target}' in .dat file for pg_stat_reset_shared, is it intentional?
Naming the argument in system_funtion.sql is enough to be able to pass
in named arguments like SELECT pg_stat_reset_shared(target := 'io');,
but is it needed in .dat file as well to keep it consistent?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
v2-0001-Add-no-argument-support-for-pg_stat_reset_slru.patch
Description: Binary data
Re: Add new option 'all' to pg_stat_reset_shared()
On Mon, Nov 13, 2023 at 01:15:14PM +0900, torikoshia wrote: > 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.gmail.com Yep, the DEFAULT value and the argument name should be documented in monitoring.sgml. -- Michael signature.asc Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
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.gmail.com 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. 28cac71bd368 is around since 13~, so changing pg_stat_reset_slru() or removing it could impact existing applications, so there's little benefit in changing it at this stage. Let it be itself. +1. As described above, since SLRUs cannot be reset by pg_stat_reset_shared(), I feel a bit uncomfortable to delete it all together. That would be only effective if NULL is given to the function to reset everything, which is OK IMO, because this is a shared stats. -- Michael -- Regards, -- Atsushi Torikoshi NTT DATA Group Corporation
Re: Add new option 'all' to pg_stat_reset_shared()
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. 28cac71bd368 is around since 13~, so changing pg_stat_reset_slru() or removing it could impact existing applications, so there's little benefit in changing it at this stage. Let it be itself. > As described above, since SLRUs cannot be reset by pg_stat_reset_shared(), I > feel a bit uncomfortable to delete it all together. That would be only effective if NULL is given to the function to reset everything, which is OK IMO, because this is a shared stats. -- Michael signature.asc Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
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. -- Michael signature.asc Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
On 2023-11-10 13:18, Andres Freund wrote: Hi, On November 8, 2023 11:28:08 PM PST, Michael Paquier wrote: 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. 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 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"? 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? As described above, since SLRUs cannot be reset by pg_stat_reset_shared(), I feel a bit uncomfortable to delete it all together. -- Regards, -- Atsushi Torikoshi NTT DATA Group Corporation
Re: Add new option 'all' to pg_stat_reset_shared()
Hi, On November 8, 2023 11:28:08 PM PST, Michael Paquier wrote: >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. > >> 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 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"? 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()? Greetings, Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: Add new option 'all' to pg_stat_reset_shared()
On Fri, Nov 10, 2023 at 12:33:50PM +0900, torikoshia wrote: > On 2023-11-09 16:28, Michael Paquier wrote: >> 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. The comments added could be better grammatically, but basically LGTM. I'll take care of that if there are no objections. -- Michael signature.asc Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
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 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 CorporationFrom f01ab1071706bdd472fe6063160379a721763a48 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi
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
pg_stat_wal view or
recovery_prefetch to reset all the counters shown
in the pg_stat_recovery_prefetch view.
+If the argument is NULL or not specified, all
+the counters from the views listed above are reset.
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
Re: Add new option 'all' to pg_stat_reset_shared()
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. > 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 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 signature.asc Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
On Thu, Nov 09, 2023 at 10:10:39AM +0900, torikoshia wrote:
I'll attach the patch.
Attached.
On Mon, Nov 6, 2023 at 5:30 PM Bharath Rupireddy
3. I think the new reset all stats function must also consider
resetting all SLRU stats, no?
/* stats for fixed-numbered objects */
PGSTAT_KIND_ARCHIVER,
PGSTAT_KIND_BGWRITER,
PGSTAT_KIND_CHECKPOINTER,
PGSTAT_KIND_IO,
PGSTAT_KIND_SLRU,
PGSTAT_KIND_WAL,
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.
On Wed, Nov 8, 2023 at 1:13 PM Andres Freund wrote:
It's not like oids are a precious resource. It's a more confusing API
to have
to have to specify a NULL as an argument than not having to do so. If
we
really want to avoid a separate oid, a more sensible path would be to
add a
default argument to pg_stat_reset_slru() (by doing a CREATE OR REPLACE
in
system_functions.sql).
Currently proisstrict is true and pg_stat_reset_shared() returns null
without doing any work.
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.
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group CorporationFrom 931bdac6e70b681e3416bbf464cbc6f42f971c6c Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi
Date: Thu, 9 Nov 2023 13:41:51 +0900
Subject: [PATCH v1] 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
at the same time.
This patch makes it possible when no argument or NULL is specified.
---
doc/src/sgml/monitoring.sgml | 4 +++-
src/backend/catalog/system_functions.sql | 7 +++
src/backend/utils/adt/pgstatfuncs.c | 19 +--
src/include/catalog/pg_proc.dat | 5 +++--
src/test/regress/expected/stats.out | 14 +++---
src/test/regress/sql/stats.sql | 14 +++---
6 files changed, 44 insertions(+), 19 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index e068f7e247..1cddc7d238 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -4716,7 +4716,7 @@ description | Waiting for a newly initialized WAL file to reach durable storage
void
-Resets some cluster-wide statistics counters to zero, depending on the
+Resets cluster-wide statistics counters to zero, depending on the
argument. The argument can be bgwriter to reset
all the counters shown in
the pg_stat_bgwriter view,
@@ -4730,6 +4730,8 @@ description | Waiting for a newly initialized WAL file to reach durable storage
pg_stat_wal view or
recovery_prefetch to reset all the counters shown
in the pg_stat_recovery_prefetch view.
+If the argument is NULL or not specified, all counters shown in all
+of these views are reset.
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..e3d78b9665 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1677,7 +1677,7 @@ pg_stat_reset(PG_FUNCTION_ARGS)
}
/*
- * Reset some shared cluster-wide counters
+ * Reset shared cluster-wide counters
*
* When adding a new reset target, ideally the name should match that in
* pgstat_kind_infos, if relevant.
@@ -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);
+
Re: Add new option 'all' to pg_stat_reset_shared()
Hi, On 2023-11-09 10:25:18 +0900, Michael Paquier wrote: > On Thu, Nov 09, 2023 at 10:10:39AM +0900, torikoshia wrote: > > I am a little concerned about that the reset time is not the same and that > > GetCurrentTimestamp() is called multiple times, but I think it would be > > acceptable because the function is probably not used that often and the > > reset time is not atomic in practice. > > Arf, right. I misremembered that this is just a clock_timestamp() so > that's not transaction-resilient. Anyway, my take is that this is not > a big deal in practice compared to the usability of the wrapper. It seems inconsequential cost-wise. Resetting stats is way more expensive that a few timestamp determinations. Correctness wise it actually seems *better* to record the timestamps more granularly, after all, that moves them closer to the time the individual kind of stats is reset. Greetings, Andres Freund
Re: Add new option 'all' to pg_stat_reset_shared()
On Thu, Nov 09, 2023 at 10:10:39AM +0900, torikoshia wrote: > I am a little concerned about that the reset time is not the same and that > GetCurrentTimestamp() is called multiple times, but I think it would be > acceptable because the function is probably not used that often and the > reset time is not atomic in practice. Arf, right. I misremembered that this is just a clock_timestamp() so that's not transaction-resilient. Anyway, my take is that this is not a big deal in practice compared to the usability of the wrapper. -- Michael signature.asc Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
On 2023-11-09 08:58, Michael Paquier wrote: On Wed, Nov 08, 2023 at 02:15:24PM +0530, Bharath Rupireddy wrote: On Wed, Nov 8, 2023 at 9:43 AM Andres Freund wrote: It's not like oids are a precious resource. It's a more confusing API to have to have to specify a NULL as an argument than not having to do so. If we really want to avoid a separate oid, a more sensible path would be to add a default argument to pg_stat_reset_slru() (by doing a CREATE OR REPLACE in system_functions.sql). +1. Attached the patch. -- Test that multiple SLRUs are reset when no specific SLRU provided to reset function -SELECT pg_stat_reset_slru(NULL); +SELECT pg_stat_reset_slru(); For the SLRU part, why not. Hmm. What's the final plan for pg_stat_reset_shared(), then? An equivalent that calls a series of pgstat_reset_of_kind()? Sorry for late reply and thanks for the feedbacks everyone. As your 1st suggestion, I think "calls a series of pgstat_reset_of_kind()" would be enough. I am a little concerned about that the reset time is not the same and that GetCurrentTimestamp() is called multiple times, but I think it would be acceptable because the function is probably not used that often and the reset time is not atomic in practice. I'll attach the patch. -- Regards, -- Atsushi Torikoshi NTT DATA Group Corporation
Re: Add new option 'all' to pg_stat_reset_shared()
On Wed, Nov 08, 2023 at 02:15:24PM +0530, Bharath Rupireddy wrote: > On Wed, Nov 8, 2023 at 9:43 AM Andres Freund wrote: >> It's not like oids are a precious resource. It's a more confusing API to have >> to have to specify a NULL as an argument than not having to do so. If we >> really want to avoid a separate oid, a more sensible path would be to add a >> default argument to pg_stat_reset_slru() (by doing a CREATE OR REPLACE in >> system_functions.sql). > > +1. Attached the patch. > > -- Test that multiple SLRUs are reset when no specific SLRU provided to > reset function > -SELECT pg_stat_reset_slru(NULL); > +SELECT pg_stat_reset_slru(); For the SLRU part, why not. Hmm. What's the final plan for pg_stat_reset_shared(), then? An equivalent that calls a series of pgstat_reset_of_kind()? -- Michael signature.asc Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
On Wed, 8 Nov 2023 at 05:13, Andres Freund wrote: > > Hi, > > On 2023-11-06 14:00:13 +0530, Bharath Rupireddy wrote: > > Well, that's a total of ~17 LWLocks this new function takes to make > > the stats reset atomic. I'm not sure if this atomicity is worth the > > effort which can easily be misused - what if someone runs something > > like SELECT pg_stat_reset_shared() FROM generate_series(1, > > 10n); to cause heavy lock acquisition and release cycles? > > Yea, this seems like an *extremely* bad idea to me. Without careful analysis > it could very well cause deadlocks. I didn't realize that it'd take 17 LwLocks to reset those stats; I thought it was one shared system using the same lock, or a very limited set of locks. Aquiring 17 locks is quite likely not worth the chance of having to wait for some stats lock or another and thus generating 'bubbles' in other stats gathering pipelines. > > IMV, atomicity is not something that applies for the stats reset > > operation because stats are approximate numbers by nature after all. > > If the pg_stat_reset_shared() resets stats for only a bunch of stats > > types and fails, it's the basic application programming style that > > when a query fails it's the application that needs to have a retry > > mechanism. FWIW, the atomicity doesn't apply today if someone wants to > > reset stats in a loop for all stats types. > > Yea. Additionally it's not really atomic regardless of the lwlocks, due to > various processes all accumulating in local counters first, and only > occasionally updating the shared data. So even after holding all the locks at > the same time, the shared stats would still not actually represent a truly > atomic state. Good points that I hadn't thought much about yet. I agree that atomic reset isn't worth implementing in this stats system - it's too costly and complex to do it correctly. Kind regards, Matthias van de Meent Neon (https://neon.tech)
Re: Add new option 'all' to pg_stat_reset_shared()
On Wed, Nov 8, 2023 at 9:43 AM Andres Freund wrote:
>
> > 2.
> > +{ oid => '8000',
> > + descr => 'statistics: reset collected statistics shared across the
> > cluster',
> > + proname => 'pg_stat_reset_shared', provolatile => 'v', prorettype =>
> > 'void',
> > + proargtypes => '', prosrc => 'pg_stat_reset_shared_all' },
> >
> > Why a new function consuming the oid? Why can't we just do the trick
> > of proisstrict => 'f' and if (PG_ARGISNULL(0)) { reset all stats} else
> > {reset specified stats kind} like the pg_stat_reset_slru()?
>
> It's not like oids are a precious resource. It's a more confusing API to have
> to have to specify a NULL as an argument than not having to do so. If we
> really want to avoid a separate oid, a more sensible path would be to add a
> default argument to pg_stat_reset_slru() (by doing a CREATE OR REPLACE in
> system_functions.sql).
+1. Attached the patch.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 8b70989fcf88e92cd005852dd75a770324f3de3e Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Wed, 8 Nov 2023 08:33:19 +
Subject: [PATCH v1] Add no argument support for pg_stat_reset_slru
Presently, one needs to specify pg_stat_reset_slru input argument
as NULL to reset all SLRU stats which looks a bit odd confusing to
the user. This commit changes the API by adding a DEFAULT NULL to
input parameter in system_functions.sql allowing users to not pass
anything as input to reset all SLRU stats.
---
doc/src/sgml/monitoring.sgml | 6 +++---
src/backend/catalog/system_functions.sql | 7 +++
src/include/catalog/pg_proc.dat | 3 ++-
src/test/regress/expected/stats.out | 2 +-
src/test/regress/sql/stats.sql | 2 +-
5 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index a0257fea0c..87873a3c29 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -4844,9 +4844,9 @@ description | Waiting for a newly initialized WAL file to
reach durable storage
Resets statistics to zero for a single SLRU cache, or for all SLRUs in
-the cluster. If the argument is NULL, all counters shown in
-the pg_stat_slru view for all SLRU caches are
-reset. The argument can be one of
+the cluster. If the argument is NULL or not specified, all counters
+shown in the pg_stat_slru view for all SLRU
+caches are reset. The argument can be one of
CommitTs,
MultiXactMember,
MultiXactOffset,
diff --git a/src/backend/catalog/system_functions.sql
b/src/backend/catalog/system_functions.sql
index 35d738d576..2546912436 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_slru(target text DEFAULT NULL)
+RETURNS void
+LANGUAGE INTERNAL
+CALLED ON NULL INPUT VOLATILE PARALLEL SAFE
+AS 'pg_stat_reset_slru';
+
--
-- 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/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 4af16a0f81..7999da77f4 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5896,7 +5896,8 @@
{ oid => '2307',
descr => 'statistics: reset collected statistics for a single SLRU',
proname => 'pg_stat_reset_slru', proisstrict => 'f', provolatile => 'v',
- prorettype => 'void', proargtypes => 'text', prosrc => 'pg_stat_reset_slru'
},
+ prorettype => 'void', proargtypes => 'text', proargnames => '{target}',
+ prosrc => 'pg_stat_reset_slru' },
{ oid => '6170',
descr => 'statistics: reset collected statistics for a single replication
slot',
proname => 'pg_stat_reset_replication_slot', proisstrict => 'f',
diff --git a/src/test/regress/expected/stats.out
b/src/test/regress/expected/stats.out
index 494cef07d3..3355adb956 100644
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -882,7 +882,7 @@ SELECT stats_reset >
:'slru_commit_ts_reset_ts'::timestamptz FROM pg_stat_slru W
SELECT stats_reset AS slru_commit_ts_reset_ts FROM pg_stat_slru WHERE name =
'CommitTs' \gset
-- Test that multiple SLRUs are reset when no specific SLRU provided to reset
function
-SELECT pg_stat_reset_slru(NULL);
+SELECT pg_stat_reset_slru();
pg_stat_reset_slru
diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index 7ae8b8a276..f8fc38eea7 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -454,7 +454,7 @@ SELECT stats_reset >
:'slru_commit_ts_reset_ts'::timestamptz FROM pg_stat_slru W
SELECT stats_reset AS
Re: Add new option 'all' to pg_stat_reset_shared()
Hi, On 2023-11-08 10:08:42 +0900, Kyotaro Horiguchi wrote: > At Mon, 6 Nov 2023 14:00:13 +0530, Bharath Rupireddy > wrote in > > On Mon, Nov 6, 2023 at 11:39 AM torikoshia > > wrote: > > > Since each stats, except wal_prefetch was reset acquiring LWLock, > > > attached PoC patch makes the call atomic by using these LWlocks. > > > > > > If this is the right direction, I'll try to make wal_prefetch also take > > > LWLock. > > I must say, I have reservations about this approach. The main concern > is the duplication of reset code, which has been efficiently > encapsulated for individual targets, into this location. This practice > degrades the maintainability and clarity of the code. Yes, as-is this seems to evolve the code in precisely the wrong direction. We want less central awareness of different types of stats, not more. The proposed new code is far longer than the current pg_stat_reset(), despite doing something conceptually simpler. Greetings, Andres Freund
Re: Add new option 'all' to pg_stat_reset_shared()
Hi,
On 2023-11-06 14:00:13 +0530, Bharath Rupireddy wrote:
> On Mon, Nov 6, 2023 at 11:39 AM torikoshia wrote:
> >
> > Thanks all for the comments!
> >
> > On Fri, Nov 3, 2023 at 5:17 AM Matthias van de Meent
> > wrote:
> > > Knowing that your metrics have a shared starting point can be quite
> > > valuable, as it allows you to do some math that would otherwise be
> > > much less accurate when working with stats over a short amount of
> > > time. I've not used these stats systems much myself, but skew between
> > > metrics caused by different reset points can be difficult to detect
> > > and debug, so I think an atomic call to reset all these stats could be
> > > worth implementing.
> >
> > Since each stats, except wal_prefetch was reset acquiring LWLock,
> > attached PoC patch makes the call atomic by using these LWlocks.
> >
> > If this is the right direction, I'll try to make wal_prefetch also take
> > LWLock.
>
> +// Acquire LWLocks
> +LWLock *locks[] = {_archiver->lock, _bgwriter->lock,
> + _checkpointer->lock, _wal->lock};
> +
> +for (int i = 0; i < STATS_SHARED_NUM_LWLOCK; i++)
> +LWLockAcquire(locks[i], LW_EXCLUSIVE);
> +
> +for (int i = 0; i < BACKEND_NUM_TYPES; i++)
> +{
> +LWLock*bktype_lock = _io->locks[i];
> +LWLockAcquire(bktype_lock, LW_EXCLUSIVE);
> +}
>
> Well, that's a total of ~17 LWLocks this new function takes to make
> the stats reset atomic. I'm not sure if this atomicity is worth the
> effort which can easily be misused - what if someone runs something
> like SELECT pg_stat_reset_shared() FROM generate_series(1,
> 10n); to cause heavy lock acquisition and release cycles?
Yea, this seems like an *extremely* bad idea to me. Without careful analysis
it could very well cause deadlocks.
> IMV, atomicity is not something that applies for the stats reset
> operation because stats are approximate numbers by nature after all.
> If the pg_stat_reset_shared() resets stats for only a bunch of stats
> types and fails, it's the basic application programming style that
> when a query fails it's the application that needs to have a retry
> mechanism. FWIW, the atomicity doesn't apply today if someone wants to
> reset stats in a loop for all stats types.
Yea. Additionally it's not really atomic regardless of the lwlocks, due to
various processes all accumulating in local counters first, and only
occasionally updating the shared data. So even after holding all the locks at
the same time, the shared stats would still not actually represent a truly
atomic state.
> 2.
> +{ oid => '8000',
> + descr => 'statistics: reset collected statistics shared across the
> cluster',
> + proname => 'pg_stat_reset_shared', provolatile => 'v', prorettype =>
> 'void',
> + proargtypes => '', prosrc => 'pg_stat_reset_shared_all' },
>
> Why a new function consuming the oid? Why can't we just do the trick
> of proisstrict => 'f' and if (PG_ARGISNULL(0)) { reset all stats} else
> {reset specified stats kind} like the pg_stat_reset_slru()?
It's not like oids are a precious resource. It's a more confusing API to have
to have to specify a NULL as an argument than not having to do so. If we
really want to avoid a separate oid, a more sensible path would be to add a
default argument to pg_stat_reset_slru() (by doing a CREATE OR REPLACE in
system_functions.sql).
Greetings,
Andres Freund
Re: Add new option 'all' to pg_stat_reset_shared()
On Wed, Nov 08, 2023 at 10:08:42AM +0900, Kyotaro Horiguchi wrote:
> At Mon, 6 Nov 2023 14:00:13 +0530, Bharath Rupireddy
> wrote in
> I must say, I have reservations about this approach. The main concern
> is the duplication of reset code, which has been efficiently
> encapsulated for individual targets, into this location. This practice
> degrades the maintainability and clarity of the code.
+{ oid => '8000',
This OID pick had me smile.
>> IMV, atomicity is not something that applies for the stats reset
>> operation because stats are approximate numbers by nature after all.
>> If the pg_stat_reset_shared() resets stats for only a bunch of stats
>> types and fails, it's the basic application programming style that
>> when a query fails it's the application that needs to have a retry
>> mechanism. FWIW, the atomicity doesn't apply today if someone wants to
>> reset stats in a loop for all stats types.
>
> The infrequent use of this feature, coupled with the fact that there
> is no inherent need for these counters to be reset simultaneoulsy,
> leads me to think that there is little point in centralizing the
> locks.
Each stat listed with fixed_amount has meaning taken in isolation, so
I don't see why this patch has to be that complicated. I'd expect one
code path that just calls a series of pgstat_reset_of_kind(). There
could be an argument for a new routine in pgstat.c that loops over the
pgstat_kind_infos and triggers the callbacks where .fixed_amount is
set, but that's less transparent than the other approach. The reset
time should be consistent across all the calls as we rely on
GetCurrentTimestamp().
--
Michael
signature.asc
Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
At Mon, 6 Nov 2023 14:00:13 +0530, Bharath Rupireddy wrote in > On Mon, Nov 6, 2023 at 11:39 AM torikoshia wrote: > > Since each stats, except wal_prefetch was reset acquiring LWLock, > > attached PoC patch makes the call atomic by using these LWlocks. > > > > If this is the right direction, I'll try to make wal_prefetch also take > > LWLock. I must say, I have reservations about this approach. The main concern is the duplication of reset code, which has been efficiently encapsulated for individual targets, into this location. This practice degrades the maintainability and clarity of the code. > Well, that's a total of ~17 LWLocks this new function takes to make > the stats reset atomic. I'm not sure if this atomicity is worth the > effort which can easily be misused - what if someone runs something > like SELECT pg_stat_reset_shared() FROM generate_series(1, > 10n); to cause heavy lock acquisition and release cycles? ... > IMV, atomicity is not something that applies for the stats reset > operation because stats are approximate numbers by nature after all. > If the pg_stat_reset_shared() resets stats for only a bunch of stats > types and fails, it's the basic application programming style that > when a query fails it's the application that needs to have a retry > mechanism. FWIW, the atomicity doesn't apply today if someone wants to > reset stats in a loop for all stats types. The infrequent use of this feature, coupled with the fact that there is no inherent need for these counters to be reset simultaneoulsy, leads me to think that there is little point in cnetralizing the locks. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Add new option 'all' to pg_stat_reset_shared()
On Mon, Nov 6, 2023 at 11:39 AM torikoshia wrote:
>
> Thanks all for the comments!
>
> On Fri, Nov 3, 2023 at 5:17 AM Matthias van de Meent
> wrote:
> > Knowing that your metrics have a shared starting point can be quite
> > valuable, as it allows you to do some math that would otherwise be
> > much less accurate when working with stats over a short amount of
> > time. I've not used these stats systems much myself, but skew between
> > metrics caused by different reset points can be difficult to detect
> > and debug, so I think an atomic call to reset all these stats could be
> > worth implementing.
>
> Since each stats, except wal_prefetch was reset acquiring LWLock,
> attached PoC patch makes the call atomic by using these LWlocks.
>
> If this is the right direction, I'll try to make wal_prefetch also take
> LWLock.
+// Acquire LWLocks
+LWLock *locks[] = {_archiver->lock, _bgwriter->lock,
+ _checkpointer->lock, _wal->lock};
+
+for (int i = 0; i < STATS_SHARED_NUM_LWLOCK; i++)
+LWLockAcquire(locks[i], LW_EXCLUSIVE);
+
+for (int i = 0; i < BACKEND_NUM_TYPES; i++)
+{
+LWLock*bktype_lock = _io->locks[i];
+LWLockAcquire(bktype_lock, LW_EXCLUSIVE);
+}
Well, that's a total of ~17 LWLocks this new function takes to make
the stats reset atomic. I'm not sure if this atomicity is worth the
effort which can easily be misused - what if someone runs something
like SELECT pg_stat_reset_shared() FROM generate_series(1,
10n); to cause heavy lock acquisition and release cycles?
IMV, atomicity is not something that applies for the stats reset
operation because stats are approximate numbers by nature after all.
If the pg_stat_reset_shared() resets stats for only a bunch of stats
types and fails, it's the basic application programming style that
when a query fails it's the application that needs to have a retry
mechanism. FWIW, the atomicity doesn't apply today if someone wants to
reset stats in a loop for all stats types.
> On 2023-11-04 10:49, Andres Freund wrote:
>
> > Yes, agreed. However, I'd suggest adding pg_stat_reset_shared(),
> > instead of
> > pg_stat_reset_shared('all') for this purpose.
>
> In the attached PoC patch the shared statistics are reset by calling
> pg_stat_reset_shared() not pg_stat_reset_shared('all').
Some quick comments:
1.
+/*
+pg_stat_reset_shared_all(PG_FUNCTION_ARGS)
+{
+pgstat_reset_shared_all();
+PG_RETURN_VOID();
+}
IMO, simpler way is to move the existing code in
pg_stat_reset_shared() to a common internal function like
pgstat_reset_shared(char *target) and the pg_stat_reset_shared_all()
can just loop over all the stats targets.
2.
+{ oid => '8000',
+ descr => 'statistics: reset collected statistics shared across the cluster',
+ proname => 'pg_stat_reset_shared', provolatile => 'v', prorettype => 'void',
+ proargtypes => '', prosrc => 'pg_stat_reset_shared_all' },
Why a new function consuming the oid? Why can't we just do the trick
of proisstrict => 'f' and if (PG_ARGISNULL(0)) { reset all stats} else
{reset specified stats kind} like the pg_stat_reset_slru()?
3. I think the new reset all stats function must also consider
resetting all SLRU stats, no?
/* stats for fixed-numbered objects */
PGSTAT_KIND_ARCHIVER,
PGSTAT_KIND_BGWRITER,
PGSTAT_KIND_CHECKPOINTER,
PGSTAT_KIND_IO,
PGSTAT_KIND_SLRU,
PGSTAT_KIND_WAL,
4. I think the new reset all stats function must also consider
resetting recovery_prefetch.
5.
+If no argument is specified, reset all these views at once.
+Note current patch is WIP and pg_stat_recovery_prefetch is not reset.
How about "If the argument is NULL, all counters shown in all of these
views are reset."?
6. Add a test case to cover the code in stats.sql.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Re: Add new option 'all' to pg_stat_reset_shared()
On Sat, Nov 4, 2023 at 7:19 AM Andres Freund wrote:
>
> On 2023-11-03 00:55:00 +0530, Bharath Rupireddy wrote:
> > On Wed, Nov 1, 2023 at 4:24 AM Michael Paquier wrote:
> > >
> > > On Tue, Oct 31, 2023 at 04:26:18PM +0900, torikoshia wrote:
> > > > Yes, calling pg_stat_reset_shared() for all stats types can do what I
> > > > wanted
> > > > to do.
> > > > But calling it with 6 different parameters seems tiresome and I thought
> > > > it
> > > > would be convenient to have a parameter to delete all cluster-wide
> > > > statistics at once.
> > > > I may be wrong, but I imagine that it's more common to want to delete
> > > > all of
> > > > the statistics for an entire cluster rather than just a portion of it.
>
> Yes, agreed. However, I'd suggest adding pg_stat_reset_shared(), instead of
> pg_stat_reset_shared('all') for this purpose.
An overloaded function seems a better choice than another target
'all'. I'm all +1 for it as others seem to concur with the idea of
having something to reset all shared stats.
> > > If more flexibility is wanted in this function, could it be an option
> > > to consider a flavor like pg_stat_reset_shared(text[]), where it is
> > > possible to specify a list of shared stats types to reset? Perhaps
> > > there are no real use cases for it, just wanted to mention it anyway
> > > regarding the fact that it could have benefits to refactor this code
> > > to use a bitwise operator for its internals with bit flags for each
> > > type.
>
> I don't think there is much point in such an API - if the caller actually
> wants to delete individual stats, it's not too hard to loop.
-1 for text[] version.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Re: Add new option 'all' to pg_stat_reset_shared()
Thanks all for the comments!
On Fri, Nov 3, 2023 at 5:17 AM Matthias van de Meent
wrote:
Knowing that your metrics have a shared starting point can be quite
valuable, as it allows you to do some math that would otherwise be
much less accurate when working with stats over a short amount of
time. I've not used these stats systems much myself, but skew between
metrics caused by different reset points can be difficult to detect
and debug, so I think an atomic call to reset all these stats could be
worth implementing.
Since each stats, except wal_prefetch was reset acquiring LWLock,
attached PoC patch makes the call atomic by using these LWlocks.
If this is the right direction, I'll try to make wal_prefetch also take
LWLock.
On 2023-11-04 10:49, Andres Freund wrote:
Yes, agreed. However, I'd suggest adding pg_stat_reset_shared(),
instead of
pg_stat_reset_shared('all') for this purpose.
In the attached PoC patch the shared statistics are reset by calling
pg_stat_reset_shared() not pg_stat_reset_shared('all').
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group CorporationFrom f0a5cc6ad6cc351adff9302c3e9b2a227a5d9cb4 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi
Date: Mon, 6 Nov 2023 14:53:19 +0900
Subject: [PATCH v1] [WIP]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
at the same time.
This patch makes it possible when no argument is specified.
Note this patch is WIP and pg_stat_recovery_prefetch is not reset.
---
doc/src/sgml/monitoring.sgml| 4 +-
src/backend/utils/activity/pgstat.c | 75 +
src/backend/utils/adt/pgstatfuncs.c | 10
src/include/catalog/pg_proc.dat | 4 ++
src/include/pgstat.h| 1 +
5 files changed, 93 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index e068f7e247..9d1e3bf4db 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -4716,7 +4716,7 @@ description | Waiting for a newly initialized WAL file to reach durable storage
void
-Resets some cluster-wide statistics counters to zero, depending on the
+Resets cluster-wide statistics counters to zero, depending on the
argument. The argument can be bgwriter to reset
all the counters shown in
the pg_stat_bgwriter view,
@@ -4730,6 +4730,8 @@ description | Waiting for a newly initialized WAL file to reach durable storage
pg_stat_wal view or
recovery_prefetch to reset all the counters shown
in the pg_stat_recovery_prefetch view.
+If no argument is specified, reset all these views at once.
+Note current patch is WIP and pg_stat_recovery_prefetch is not reset.
This function is restricted to superusers by default, but other users
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index d743fc0b28..9b9398439f 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -767,6 +767,81 @@ pgstat_reset_of_kind(PgStat_Kind kind)
pgstat_reset_entries_of_kind(kind, ts);
}
+/* Reset below cluster-wide statistics:
+ * - pg_stat_bgwriter
+ * - pg_stat_checkpointer
+ * - pg_stat_archiver
+ * - pg_stat_io
+ * - pg_stat_wal
+ *
+ * Note recovery_prefetch is not implemented(WIP).
+ */
+void
+pgstat_reset_shared_all(void)
+{
+#define STATS_SHARED_NUM_LWLOCK 4
+ TimestampTz ts = GetCurrentTimestamp();
+
+ PgStatShared_Archiver *stats_archiver = >archiver;
+ PgStatShared_BgWriter *stats_bgwriter = >bgwriter;
+ PgStatShared_Checkpointer *stats_checkpointer = >checkpointer;
+ PgStatShared_Wal *stats_wal = >wal;
+ PgStatShared_IO *stats_io = >io;
+
+ // Acquire LWLocks
+ LWLock *locks[] = {_archiver->lock, _bgwriter->lock,
+ _checkpointer->lock, _wal->lock};
+
+ for (int i = 0; i < STATS_SHARED_NUM_LWLOCK; i++)
+ LWLockAcquire(locks[i], LW_EXCLUSIVE);
+
+ for (int i = 0; i < BACKEND_NUM_TYPES; i++)
+ {
+ LWLock *bktype_lock = _io->locks[i];
+ LWLockAcquire(bktype_lock, LW_EXCLUSIVE);
+ }
+
+ // Reset stats
+ pgstat_copy_changecounted_stats(_archiver->reset_offset,
+ _archiver->stats,
+ sizeof(stats_archiver->stats),
+ _archiver->changecount);
+ stats_archiver->stats.stat_reset_timestamp = ts;
+
+ pgstat_copy_changecounted_stats(_bgwriter->reset_offset,
+ _bgwriter->stats,
+ sizeof(stats_bgwriter->stats),
+ _bgwriter->changecount);
+ stats_bgwriter->stats.stat_reset_timestamp = ts;
+
+ pgstat_copy_changecounted_stats(_checkpointer->reset_offset,
+ _checkpointer->stats,
+ sizeof(stats_checkpointer->stats),
+ _checkpointer->changecount);
+ stats_checkpointer->stats.stat_reset_timestamp = ts;
+
+ memset(_wal->stats, 0, sizeof(stats_wal->stats));
+
Re: Add new option 'all' to pg_stat_reset_shared()
Hi,
On 2023-11-03 00:55:00 +0530, Bharath Rupireddy wrote:
> On Wed, Nov 1, 2023 at 4:24 AM Michael Paquier wrote:
> >
> > On Tue, Oct 31, 2023 at 04:26:18PM +0900, torikoshia wrote:
> > > Yes, calling pg_stat_reset_shared() for all stats types can do what I
> > > wanted
> > > to do.
> > > But calling it with 6 different parameters seems tiresome and I thought it
> > > would be convenient to have a parameter to delete all cluster-wide
> > > statistics at once.
> > > I may be wrong, but I imagine that it's more common to want to delete all
> > > of
> > > the statistics for an entire cluster rather than just a portion of it.
Yes, agreed. However, I'd suggest adding pg_stat_reset_shared(), instead of
pg_stat_reset_shared('all') for this purpose.
> > If more flexibility is wanted in this function, could it be an option
> > to consider a flavor like pg_stat_reset_shared(text[]), where it is
> > possible to specify a list of shared stats types to reset? Perhaps
> > there are no real use cases for it, just wanted to mention it anyway
> > regarding the fact that it could have benefits to refactor this code
> > to use a bitwise operator for its internals with bit flags for each
> > type.
I don't think there is much point in such an API - if the caller actually
wants to delete individual stats, it's not too hard to loop.
But most of the time resetting individual stats doesn't make sense. IMO it was
a mistake to ever add the ability. But that ship has sailed.
> I don't see a strong reason to introduce yet-another API when someone
> can just call things in a loop.
I don't agree at all. That requires callers to know the set of possible values
that stats need to be reset for - which has grown over time. But nearly all
the time the right thing to do is to reset *all* shared stats, not just some.
> I could recollect a recent analogy - a
> proposal to have a way to define multiple custom wait events with a
> single function call instead of callers defining in a loop didn't draw
> much interest.
That's not analoguous - in your example the caller by definition knows the set
of wait events it wants to create. Introducing a batch API wouldn't change
that. But in case of resetting all stats the caller does *not* inherently
know the set of stats types.
Greetings,
Andres Freund
Re: Add new option 'all' to pg_stat_reset_shared()
On Thu, 2 Nov 2023 at 20:26, Bharath Rupireddy wrote: > > On Wed, Nov 1, 2023 at 4:24 AM Michael Paquier wrote: > > > > On Tue, Oct 31, 2023 at 04:26:18PM +0900, torikoshia wrote: > > > Yes, calling pg_stat_reset_shared() for all stats types can do what I > > > wanted > > > to do. > > > But calling it with 6 different parameters seems tiresome and I thought it > > > would be convenient to have a parameter to delete all cluster-wide > > > statistics at once. > > > > > > I may be wrong, but I imagine that it's more common to want to delete all > > > of > > > the statistics for an entire cluster rather than just a portion of it. > > > > If more flexibility is wanted in this function, could it be an option > > to consider a flavor like pg_stat_reset_shared(text[]), where it is > > possible to specify a list of shared stats types to reset? Perhaps > > there are no real use cases for it, just wanted to mention it anyway > > regarding the fact that it could have benefits to refactor this code > > to use a bitwise operator for its internals with bit flags for each > > type. > > I don't see a strong reason to introduce yet-another API when someone > can just call things in a loop. I could recollect a recent analogy - a > proposal to have a way to define multiple custom wait events with a > single function call instead of callers defining in a loop didn't draw > much interest. Knowing that your metrics have a shared starting point can be quite valuable, as it allows you to do some math that would otherwise be much less accurate when working with stats over a short amount of time. I've not used these stats systems much myself, but skew between metrics caused by different reset points can be difficult to detect and debug, so I think an atomic call to reset all these stats could be worth implementing. Kind regards, Matthias van de Meent Neon (https://neon.tech)
Re: Add new option 'all' to pg_stat_reset_shared()
On Wed, Nov 1, 2023 at 4:24 AM Michael Paquier wrote: > > On Tue, Oct 31, 2023 at 04:26:18PM +0900, torikoshia wrote: > > Yes, calling pg_stat_reset_shared() for all stats types can do what I wanted > > to do. > > But calling it with 6 different parameters seems tiresome and I thought it > > would be convenient to have a parameter to delete all cluster-wide > > statistics at once. > > > > I may be wrong, but I imagine that it's more common to want to delete all of > > the statistics for an entire cluster rather than just a portion of it. > > If more flexibility is wanted in this function, could it be an option > to consider a flavor like pg_stat_reset_shared(text[]), where it is > possible to specify a list of shared stats types to reset? Perhaps > there are no real use cases for it, just wanted to mention it anyway > regarding the fact that it could have benefits to refactor this code > to use a bitwise operator for its internals with bit flags for each > type. I don't see a strong reason to introduce yet-another API when someone can just call things in a loop. I could recollect a recent analogy - a proposal to have a way to define multiple custom wait events with a single function call instead of callers defining in a loop didn't draw much interest. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Add new option 'all' to pg_stat_reset_shared()
On Tue, Oct 31, 2023 at 04:26:18PM +0900, torikoshia wrote: > Yes, calling pg_stat_reset_shared() for all stats types can do what I wanted > to do. > But calling it with 6 different parameters seems tiresome and I thought it > would be convenient to have a parameter to delete all cluster-wide > statistics at once. > > I may be wrong, but I imagine that it's more common to want to delete all of > the statistics for an entire cluster rather than just a portion of it. If more flexibility is wanted in this function, could it be an option to consider a flavor like pg_stat_reset_shared(text[]), where it is possible to specify a list of shared stats types to reset? Perhaps there are no real use cases for it, just wanted to mention it anyway regarding the fact that it could have benefits to refactor this code to use a bitwise operator for its internals with bit flags for each type. -- Michael signature.asc Description: PGP signature
Re: Add new option 'all' to pg_stat_reset_shared()
On Mon, Oct 30, 2023 at 5:46 PM Bharath Rupireddy
wrote:
Thanks for the comments!
Isn't calling pg_stat_reset_shared() for all stats types helping you
out? Is there any problem with it? Can you be more specific about the
use-case?
Yes, calling pg_stat_reset_shared() for all stats types can do what I
wanted to do.
But calling it with 6 different parameters seems tiresome and I thought
it would be convenient to have a parameter to delete all cluster-wide
statistics at once.
I may be wrong, but I imagine that it's more common to want to delete
all of the statistics for an entire cluster rather than just a portion
of it.
IMV, I don't see any point for adding another pseudo (rather
non-existent) shared stats target which might confuse users - it's
easy to specify pg_stat_reset_shared('all'); to clear things out when
someone actually doesn't want to reset all - an accidental usage of
the 'all' option will reset all shared memory stats.
I once considered changing the pg_stat_reset_shared() to delete all
stats when called without parameters like pg_stat_statements_reset(),
but gave it up since it can confuse users as you described.
I was hoping that the need to specify 'all' would remind users that the
target can be specified individually.
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
Re: Add new option 'all' to pg_stat_reset_shared()
At Mon, 30 Oct 2023 14:15:53 +0530, Bharath Rupireddy wrote in > > > I imagine there are cases where people want to initialize all of them > > > at the same time in addition to initializing one at a time. > > > > FWIW, I fairly often wanted it, but forgot about that:p > > Isn't calling pg_stat_reset_shared() for all stats types helping you > out? Is there any problem with it? Can you be more specific about the > use-case? Oh. Sorry, it's my bad. pg_stat_reset_shared() is sufficient. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Add new option 'all' to pg_stat_reset_shared()
On Mon, Oct 30, 2023 at 1:47 PM Kyotaro Horiguchi
wrote:
>
> At Mon, 30 Oct 2023 16:35:19 +0900, torikoshia
> wrote in
> > Hi,
> >
> > After 96f052613f3, we have below 6 types of parameter for
> > pg_stat_reset_shared().
> >
> > "archiver", "bgwriter", "checkpointer", "io", "recovery_prefetch",
> > "wal"
> >
> > How about adding a new option 'all' to delete all targets above?
> >
> > I imagine there are cases where people want to initialize all of them
> > at the same time in addition to initializing one at a time.
>
> FWIW, I fairly often wanted it, but forgot about that:p
Isn't calling pg_stat_reset_shared() for all stats types helping you
out? Is there any problem with it? Can you be more specific about the
use-case?
IMV, I don't see any point for adding another pseudo (rather
non-existent) shared stats target which might confuse users - it's
easy to specify pg_stat_reset_shared('all'); to clear things out when
someone actually doesn't want to reset all - an accidental usage of
the 'all' option will reset all shared memory stats.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Re: Add new option 'all' to pg_stat_reset_shared()
At Mon, 30 Oct 2023 16:35:19 +0900, torikoshia wrote in > Hi, > > After 96f052613f3, we have below 6 types of parameter for > pg_stat_reset_shared(). > > "archiver", "bgwriter", "checkpointer", "io", "recovery_prefetch", > "wal" > > How about adding a new option 'all' to delete all targets above? > > I imagine there are cases where people want to initialize all of them > at the same time in addition to initializing one at a time. FWIW, I fairly often wanted it, but forgot about that:p regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Add new option 'all' to pg_stat_reset_shared()
Hi, After 96f052613f3, we have below 6 types of parameter for pg_stat_reset_shared(). "archiver", "bgwriter", "checkpointer", "io", "recovery_prefetch", "wal" How about adding a new option 'all' to delete all targets above? I imagine there are cases where people want to initialize all of them at the same time in addition to initializing one at a time. -- Regards, -- Atsushi Torikoshi NTT DATA Group Corporation
