Date: Tue, 12 Aug 2025 09:32:25 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Greg Sabino Mullane <htamfids@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Adding locks statistics
Message-ID: <aJsKKZqwM4GIPTcK@ip-10-97-1-34.eu-west-3.compute.internal>
References: <aIyNxBWFCybgBZBS@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAKAnmm+USTr5MeKuyrAvbCjHqbSFbZ+czJsV7fPYt9nECQkf-A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAKAnmm+USTr5MeKuyrAvbCjHqbSFbZ+czJsV7fPYt9nECQkf-A@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aJsKKZqwM4GIPTcK%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Mon, Aug 11, 2025 at 01:54:38PM -0400, Greg Sabino Mullane wrote:
> Great idea. +1. Here is a quick overall review to get things started.

Thanks for looking at it!

> Meta:
> patch did not apply via "git apply". Also has carriage returns (e.g. DOS
> file), and some errant whitespace.

Interesting, I'm not seeing those issues on my side when applying with "git am".

> Name:
> I think the name would read better as pg_stat_locks, especially as it
> returns multiple rows.

I considered pg_stat_locks, but chose the singular form to be consistent with
pg_stat_database, pg_stat_subscription, and friends.

> Docs: seem good. Needs a section on how to reset via
> SELECT pg_stat_reset_shared('lock');

That's already included in v1:

"
@@ -5055,6 +5200,12 @@ description | Waiting for a newly initialized WAL file to reach durable storage
           <structname>pg_stat_io</structname> view.
          </para>
         </listitem>
+        <listitem>
+         <para>
+          <literal>lock</literal>: Reset all the counters shown in the
+          <structname>pg_stat_lock</structname> view.
+         </para>
+        </listitem>
"

Do you think that this needs any additions or changes?

> Also plural better here ('locks')

I think having pg_stat_<XXX> and XXX being the same option in " pg_stat_reset_shared()"
makes sense. It's how it has been done so far, that's why I went with 'lock'.

> Code:
> 
> + * Copyright (c) 2021-2025, PostgreSQL Global Development Group
> 
> If a new file, we can simply say 2025?

I'm not sure that matters that much, see [1] and we can look at some files added
in 2025 for examples:

src/backend/storage/aio/aio_io.c
src/backend/access/nbtree/nbtpreprocesskeys.c

where 2025 is not "alone".

> + LWLock locks[LOCKTAG_LAST_TYPE + 1];
> + PgStat_Lock stats;
> 
> Adding a lock to the system that counts locks! :) (just found amusing, not
> a comment)

;-) 

> -#define PGSTAT_KIND_SLRU 11
> -#define PGSTAT_KIND_WAL 12
> +#define PGSTAT_KIND_LOCK 11
> +#define PGSTAT_KIND_SLRU 12
> +#define PGSTAT_KIND_WAL 13
> 
> Why not just add LOCK as #13?

Because it looks like that they are ordered by alphabetical order.

> What about the overhead of maintaining this system? I know it's overall
> very lightweight, but from my testing, the relation locktype in particular
> is very, very busy.

The counter increments do call a function generated that way:

"
#define PGSTAT_COUNT_LOCK_FUNC(stat)                    \
void                                                    \
CppConcat(pgstat_count_lock_,stat)(uint8 locktag_type)  \
{                                                       \
    Assert(locktag_type <= LOCKTAG_LAST_TYPE);          \
    PendingLockStats.stats[locktag_type].stat++;        \
    have_lockstats = true;                              \
    pgstat_report_fixed = true;                         \
}
"

So, it's pretty lightweight as you said (given that PendingLockStats is
not shared and just local to the backend that increments the counter).

There could be some contention when the pending stats are flushed but that's
the same for all the stats kinds.

We could do better, though, and avoid the function calls by creating macros instead
of functions. That would mean PendingLockStats to be visible to the outside
world but:

- that's not the direction that has been taken recently (for example, see the
"kept here to avoid exposing PendingBackendStats to the outside world" comment in 
pgstat_backend.c).

- I'm not sure that's worth for this particular case and code paths.

That said, if you (and/or others) have strong concerns about performance penalties,
I could study this area more in depth.

> Busier than I realized until I saw it in this useful
> view!

Thanks! Did you observe some noticeable performance penalties?

[1]: https://www.postgresql.org/message-id/202501211750.xf5j6thdkppy%40alvherre.pgsql 

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com