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

Hi,

On 2023-11-06 14:00:13 +0530, Bharath Rupireddy wrote:
> On Mon, Nov 6, 2023 at 11:39 AM torikoshia <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