Re: Adding locks statistics

Hi,

On Thu, Mar 19, 2026 at 04:01:39PM +0900, Michael Paquier wrote:
> On Wed, Mar 18, 2026 at 02:51:01PM +0000, Bertrand Drouvot wrote:
> > This is done that way in the attached, so that we don't need the extra output in
> > the _1.out file and the test time is reduced (since the deadlock timeout is set
> > to 10ms in the test, I changed the sleep time to 50ms (I did not want to be very
> > close to 10ms)).
>
> > + <structfield>waits</structfield> <type>bigint</type>
> > + </para>
> > + <para>
> > + Number of times a lock of this type had to wait because of a
> > + conflicting lock. Only incremented when <xref linkend="guc-log-lock-waits"/>
> > + is enabled and the lock was successfully acquired after waiting longer
> > + than <xref linkend="guc-deadlock-timeout"/>.
> > + </para>
> > + </entry>
>
> It does not make much sense to me to decide that the counter is
> incremented if a GUC related to a control of the logs generated is
> enabled. It's a fact that log_lock_waits is enabled by default these
> days, hence we will be able to get the time calculation for free for
> most deployments, but it seems inconsistent to me to not count this
> information if the GUC is disabled. We should increment the counter
> and aggregate the time spent on the wait all the time, no?

Yeah that's another way to think about it. In my mind the GUC was a "protection"
to be able to avoid the extra GetCurrentTimestamp() call. But since it's done
only if we waited longer than the deadlock timeout that's also fine to call
GetCurrentTimestamp() even if log_lock_waits is off. Done that way in the
attached.

>
> + * Copyright (c) 2021-2025, PostgreSQL Global Development Group
>
> Incorrect date at the top of pgstat_lock.c.

Yeah, so replaced 2025 with 2026. I did not change 2021 because it's mainly copied
from pgstat_io.c and I also think that's not that important ([1]).

> storage/lock.h is included in pgstat.h as LOCKTAG_LAST_TYPE is wanted
> for the new lock stats structure. That would pull in a lot of header
> data into pgstat.h. How about creating a new header that splits a
> portion of lock.h into a new file? LockTagType, LOCKTAG_LAST_TYPE,
> LockTagTypeNames at least and perhaps a few other things? Or we could
> just limit ourselves to a locktag.h with these three, then include the
> new locktag.h in pgstat.h.

That's a good idea! I only moved LockTagType, LOCKTAG_LAST_TYPE, LockTagTypeNames
and LOCKTAG into a new locktag.h. I chose not to move the SET_LOCKTAG_* macros
because then we'd also need to move DEFAULT_LOCKMETHOD and USER_LOCKMETHOD that
I think are better suited in lock.h.

I did not check if there are any other files that could benefit of using
locktag.h instead of lock.h but that's something I'll do and open a dedicated
if any (once locktag.h is in the tree).

> It would be nice to document at the top of the new spec file what this
> test is here for,

Yeah, I copied stats.spec which has no comments but better to add some. Done in
the new version.

> and perhaps name it stats-lock instead?

Not sure as we already have multixact-stats.

[1]: https://postgr.es/m/202501211750.xf5j6thdkppy%40alvherre.pgsql

Regards,

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