Horton

Thread

Commits

  1. Add target "slru" to pg_stat_reset_shared()

  2. doc: Improve description of targets for pg_stat_reset_shared()

  3. Add support for pg_stat_reset_slru without argument

  4. Add ability to reset all shared stats types in pg_stat_reset_shared()

  1. Add new option 'all' to pg_stat_reset_shared()

    torikoshia <torikoshia@oss.nttdata.com> — 2023-10-30T07:35:19Z

    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

  2. Re: Add new option 'all' to pg_stat_reset_shared()

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-10-30T08:17:13Z

    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

  3. Re: Add new option 'all' to pg_stat_reset_shared()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2023-10-30T08:45:53Z

    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

  4. Re: Add new option 'all' to pg_stat_reset_shared()

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-10-31T02:59:31Z

    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

  5. Re: Add new option 'all' to pg_stat_reset_shared()

    torikoshia <torikoshia@oss.nttdata.com> — 2023-10-31T07:26:18Z

    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

  6. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-10-31T22:54:46Z

    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

  7. Re: Add new option 'all' to pg_stat_reset_shared()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2023-11-02T19:25:00Z

    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

  8. Re: Add new option 'all' to pg_stat_reset_shared()

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2023-11-02T20:17:09Z

    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)

  9. Re: Add new option 'all' to pg_stat_reset_shared()

    Andres Freund <andres@anarazel.de> — 2023-11-04T01:49:05Z

    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

  10. Re: Add new option 'all' to pg_stat_reset_shared()

    torikoshia <torikoshia@oss.nttdata.com> — 2023-11-06T06:09:28Z

    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

  11. Re: Add new option 'all' to pg_stat_reset_shared()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2023-11-06T06:18:52Z

    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

  12. Re: Add new option 'all' to pg_stat_reset_shared()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2023-11-06T08:30:13Z

    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

  13. Re: Add new option 'all' to pg_stat_reset_shared()

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-11-08T01:08:42Z

    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

  14. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-08T01:26:32Z

    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

  15. Re: Add new option 'all' to pg_stat_reset_shared()

    Andres Freund <andres@anarazel.de> — 2023-11-08T04:13:31Z

    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

  16. Re: Add new option 'all' to pg_stat_reset_shared()

    Andres Freund <andres@anarazel.de> — 2023-11-08T04:18:30Z

    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

  17. Re: Add new option 'all' to pg_stat_reset_shared()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2023-11-08T08:45:24Z

    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

  18. Re: Add new option 'all' to pg_stat_reset_shared()

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2023-11-08T13:17:00Z

    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)

  19. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-08T23:58:48Z

    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

  20. Re: Add new option 'all' to pg_stat_reset_shared()

    torikoshia <torikoshia@oss.nttdata.com> — 2023-11-09T01:10:39Z

    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

  21. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-09T01:25:18Z

    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

  22. Re: Add new option 'all' to pg_stat_reset_shared()

    Andres Freund <andres@anarazel.de> — 2023-11-09T01:29:52Z

    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

  23. Re: Add new option 'all' to pg_stat_reset_shared()

    torikoshia <torikoshia@oss.nttdata.com> — 2023-11-09T04:50:34Z

    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

  24. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-09T07:28:08Z

    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

  25. Re: Add new option 'all' to pg_stat_reset_shared()

    torikoshia <torikoshia@oss.nttdata.com> — 2023-11-10T03:33:50Z

    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

  26. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-10T04:15:50Z

    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

  27. Re: Add new option 'all' to pg_stat_reset_shared()

    Andres Freund <andres@anarazel.de> — 2023-11-10T04:18:28Z

    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.

  28. Re: Add new option 'all' to pg_stat_reset_shared()

    torikoshia <torikoshia@oss.nttdata.com> — 2023-11-10T11:32:34Z

    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

  29. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-12T07:46:51Z

    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

  30. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-12T07:54:38Z

    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

  31. Re: Add new option 'all' to pg_stat_reset_shared()

    torikoshia <torikoshia@oss.nttdata.com> — 2023-11-13T04:15:14Z

    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

  32. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-13T05:55:19Z

    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

  33. Re: Add new option 'all' to pg_stat_reset_shared()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2023-11-13T08:37:21Z

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

  34. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-13T10:31:41Z

    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

  35. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-14T00:55:14Z

    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

  36. Re: Add new option 'all' to pg_stat_reset_shared()

    torikoshia <torikoshia@oss.nttdata.com> — 2023-11-14T13:02:32Z

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

What do you think?


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

  37. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-15T00:47:03Z

    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

  38. Re: Add new option 'all' to pg_stat_reset_shared()

    torikoshia <torikoshia@oss.nttdata.com> — 2023-11-15T02:58:38Z

    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

  39. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-15T07:25:14Z

    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

  40. Re: Add new option 'all' to pg_stat_reset_shared()

    Michael Paquier <michael@paquier.xyz> — 2023-11-16T07:48:39Z

    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

  41. Re: Add new option 'all' to pg_stat_reset_shared()

    torikoshia <torikoshia@oss.nttdata.com> — 2023-11-16T08:12:26Z

    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