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)