Date: Thu, 19 Mar 2026 12:25:41 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>, Jeff Davis <pgsql@j-davis.com>,
	Greg Sabino Mullane <htamfids@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Adding locks statistics
Message-ID: <abvrRZo52Yx9ZzWQ@ip-10-97-1-34.eu-west-3.compute.internal>
References: <aZaMXK90sRjSQBJP@paquier.xyz>
 <v76tz252usppu2turrn6wxiyyxmybzxhqck5b3amtsbzrc6yto@n26qnwvpkxqw>
 <aZgBTyzxWmPJL0Tt@ip-10-97-1-34.eu-west-3.compute.internal>
 <isi5wszczhusgjetxwe6khxa5mbm5jms3msmbhwl73jktxw7ic@3mympoumua76>
 <aZiZTX1l5pX+BOXn@ip-10-97-1-34.eu-west-3.compute.internal>
 <aaEoN1XexvGiFvYp@ip-10-97-1-34.eu-west-3.compute.internal>
 <abortN3ngqMcaBJ7@ip-10-97-1-34.eu-west-3.compute.internal>
 <abpfH33mR4Mp9dtO@paquier.xyz>
 <abq71eL7qIdWAxMD@ip-10-97-1-34.eu-west-3.compute.internal>
 <abufUya2oK-_PJ3E@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="YVZ2rdHTqHRFZ4HX"
Content-Disposition: inline
In-Reply-To: <abufUya2oK-_PJ3E@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/abvrRZo52Yx9ZzWQ%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk


--YVZ2rdHTqHRFZ4HX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--YVZ2rdHTqHRFZ4HX
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v11-0001-Add-storage-locktag.h.patch"