Add new option 'all' to pg_stat_reset_shared()

At Mon, 30 Oct 2023 16:35:19 +0900, torikoshia <torikoshia@oss.nttdata.com> 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

On Mon, Oct 30, 2023 at 1:47 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Mon, 30 Oct 2023 16:35:19 +0900, torikoshia <torikoshia@oss.nttdata.com> 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

At Mon, 30 Oct 2023 14:15:53 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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

On Mon, Oct 30, 2023 at 5:46 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> 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

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

On Wed, Nov 1, 2023 at 4:24 AM Michael Paquier <michael@paquier.xyz> 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

On Thu, 2 Nov 2023 at 20:26, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Nov 1, 2023 at 4:24 AM Michael Paquier <michael@paquier.xyz> 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)

Hi,

On 2023-11-03 00:55:00 +0530, Bharath Rupireddy wrote:

On Wed, Nov 1, 2023 at 4:24 AM Michael Paquier <michael@paquier.xyz> 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

Thanks all for the comments!

On Fri, Nov 3, 2023 at 5:17 AM Matthias van de Meent
<boekewurm+postgres@gmail.com> 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 Corporation

On Sat, Nov 4, 2023 at 7:19 AM Andres Freund <andres@anarazel.de> wrote:

On 2023-11-03 00:55:00 +0530, Bharath Rupireddy wrote:

On Wed, Nov 1, 2023 at 4:24 AM Michael Paquier <michael@paquier.xyz> 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

On Mon, Nov 6, 2023 at 11:39 AM torikoshia <torikoshia@oss.nttdata.com> wrote:

Thanks all for the comments!

On Fri, Nov 3, 2023 at 5:17 AM Matthias van de Meent
<boekewurm+postgres@gmail.com> 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[] = {&stats_archiver->lock,  &stats_bgwriter->lock,
+                       &stats_checkpointer->lock, &stats_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 = &stats_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,
100000....n); 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.
        </para>

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

At Mon, 6 Nov 2023 14:00:13 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in

On Mon, Nov 6, 2023 at 11:39 AM torikoshia <torikoshia@oss.nttdata.com> 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,
100000....n); 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

On Wed, Nov 08, 2023 at 10:08:42AM +0900, Kyotaro Horiguchi wrote:

At Mon, 6 Nov 2023 14:00:13 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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

Hi,

On 2023-11-06 14:00:13 +0530, Bharath Rupireddy wrote:

On Mon, Nov 6, 2023 at 11:39 AM torikoshia <torikoshia@oss.nttdata.com> wrote:

Thanks all for the comments!

On Fri, Nov 3, 2023 at 5:17 AM Matthias van de Meent
<boekewurm+postgres@gmail.com> 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[] = {&stats_archiver->lock,  &stats_bgwriter->lock,
+                       &stats_checkpointer->lock, &stats_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 = &stats_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,
100000....n); 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

Hi,

On 2023-11-08 10:08:42 +0900, Kyotaro Horiguchi wrote:

At Mon, 6 Nov 2023 14:00:13 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in

On Mon, Nov 6, 2023 at 11:39 AM torikoshia <torikoshia@oss.nttdata.com> 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

On Wed, Nov 8, 2023 at 9:43 AM Andres Freund <andres@anarazel.de> 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

On Wed, 8 Nov 2023 at 05:13, Andres Freund <andres@anarazel.de> 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,
100000....n); 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)

On Wed, Nov 08, 2023 at 02:15:24PM +0530, Bharath Rupireddy wrote:

On Wed, Nov 8, 2023 at 9:43 AM Andres Freund <andres@anarazel.de> 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

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 <andres@anarazel.de>
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

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

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

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 <andres@anarazel.de> 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 Corporation

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 <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

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

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

Hi,

On November 8, 2023 11:28:08 PM PST, Michael Paquier <michael@paquier.xyz> 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 <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"?

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.

On 2023-11-10 13:18, Andres Freund wrote:

Hi,

On November 8, 2023 11:28:08 PM PST, Michael Paquier
<michael@paquier.xyz> 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 <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"?

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

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

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

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():

/messages/by-id/CALj2ACW4Fqc_m+OaavrOMEivZ5aBa24pVKvoXRTmuFECsNBfAg@mail.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

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():

/messages/by-id/CALj2ACW4Fqc_m+OaavrOMEivZ5aBa24pVKvoXRTmuFECsNBfAg@mail.gmail.com

Yep, the DEFAULT value and the argument name should be documented in
monitoring.sgml.
--
Michael

On Mon, Nov 13, 2023 at 9:45 AM torikoshia <torikoshia@oss.nttdata.com> 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():

/messages/by-id/CALj2ACW4Fqc_m+OaavrOMEivZ5aBa24pVKvoXRTmuFECsNBfAg@mail.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

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

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

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():

/messages/by-id/CALj2ACW4Fqc_m+OaavrOMEivZ5aBa24pVKvoXRTmuFECsNBfAg@mail.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 <itemizedlist>.

What do you think?

--
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

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 <itemizedlist>.

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

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 <itemizedlist>.

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 Corporation

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

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

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