Subject: Re: Adding locks statistics

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 18 Mar 2026 04:36:04 +0000

Hi,

On Fri, Feb 27, 2026 at 05:14:31AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Fri, Feb 20, 2026 at 05:26:37PM +0000, Bertrand Drouvot wrote:
> > Hi,
> > 
> > On Fri, Feb 20, 2026 at 11:02:49AM -0500, Andres Freund wrote:
> > > Hi,
> > > 
> > > How could a user benefit from that split? To me this is pointless number
> > > gathering that wastes resources and confuses users.
> > 
> > I was thinking that could be useful to know the distribution between "long" waits
> > (greater than the deadlock timeout) among all the waits.
> > 
> > If the vast majority are long waits that may indicate that the application is
> > misbehaving (as opposed to a tiny percentage of long waits).
> > 
> > I was also thinking to bring those stats per-backend (as a next step) and that
> > could also probably be more useful (distribution per host for example, thanks to
> > joining with pg_stat_activity).
> 
> As it seems that I'm the only one thinking that this split could be useful, I'm
> removing it in the attached. We can still split later on if we have requests from
> the field.
> 
> So, we're back to what we were discussing before the split. As in v7, 0003 is
> adding the new GUC. So that we can see what having a new GUC implies in ProcSleep()
> and we can just get rid of 0003 if we think the GUC is not worth the extra complexity
> (I don't have a strong opinion on it but tempted to think that the extra GUC is
> not worth it).

PFA, a rebase due to fd6ecbfa75ff.

Regards,

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

---

From: Michael Paquier <[email-masked]>
Date: Wed, 18 Mar 2026 17:15:27 +0900

> PFA, a rebase due to fd6ecbfa75ff.

Looking again at this patch, all the fields that you are adding are in
non-critical paths, so it looks fine by me to begin with this data
set.  We may want to document that for future readers of the code to
not add counter increments in the fast code paths, where performance
could matter.

Let's also drop 0003 with the GUC.  log_lock_waits is enabled by
default and we are in a wait path which would not be
performance-critical.

Regarding the isolation test, the new permutations add 4 pg_sleep()
calls at 500ms each, making the stats test longer.  It also looks like
the outputs are the same for the two alternate expected files?  Do you
think that it could be possible to move these tests to a new file,
perhaps cutting a bit the sleeps to make it faster?
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 18 Mar 2026 14:51:01 +0000

Hi,

On Wed, Mar 18, 2026 at 05:15:27PM +0900, Michael Paquier wrote:
> set.

Thanks for looking at it!

> We may want to document that for future readers of the code to

Yeah, added a few words in the callers and on top of the function definitions.

> default and we are in a wait path which would not be

Yeah, that was also my vote.

> perhaps cutting a bit the sleeps to make it faster?

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)).

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Thu, 19 Mar 2026 16:01:39 +0900

> +        <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?

+ * Copyright (c) 2021-2025, PostgreSQL Global Development Group 

Incorrect date at the top of pgstat_lock.c.

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.

It would be nice to document at the top of the new spec file what this
test is here for, and perhaps name it stats-lock instead?
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 19 Mar 2026 12:25:41 +0000

Hi,

> 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.

> 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]).

> 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).

> 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,

---

From: Michael Paquier <[email-masked]>
Date: Sat, 21 Mar 2026 14:55:33 +0900

> I think are better suited in lock.h.

While looking at the full set of declarations, I've come to disagree
to that: by moving the macros, we also do not need to know about the
internal field names of LOCKTAG across multiple headers.

> if any (once locktag.h is in the tree).

I have checked after that, and did not spot an area (except your patch
of course).  And applied this part.

> Not sure as we already have multixact-stats.

Ah, right.  I did not notice this one.  This existing name sounds OK
by me, then.
--
Michael

---

From: Álvaro Herrera <[email-masked]>
Date: Sat, 21 Mar 2026 21:01:02 +0100

> of course).  And applied this part.

I checked this, and found a couple of headers that can benefit from a removal, as shown in the attached patches.

A special case (not modified here) is proc.h. It seems to me that lock.h _could_ be removed from there with some effort, but the amount of .c files that would benefit seems to me not large enough to justify the number of contortions needed.  Others could disagree though.

-- 
Álvaro Herrera

From aa6155106b7f5dc697df54dc4e4aeb5c6419ec92 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email-masked]>
Date: Sat, 21 Mar 2026 20:33:10 +0100
Subject: [PATCH 1/4] procarray.h does not need storage/lock.h for anything

No fallout from this change.  Weird
---
 src/include/storage/procarray.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index c5ab1574fe3..abdf021e66e 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -14,7 +14,6 @@
 #ifndef PROCARRAY_H
 #define PROCARRAY_H
 
-#include "storage/lock.h"
 #include "storage/standby.h"
 #include "utils/relcache.h"
 #include "utils/snapshot.h"
-- 
2.47.3

From c09489b4164d0d8c7942ca20a9fb645a1d68ea7f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email-masked]>
Date: Sat, 21 Mar 2026 20:35:51 +0100
Subject: [PATCH 2/4] lmgr.h doesn't need the full lock.h

---
 src/backend/utils/activity/wait_event.c | 6 ++++--
 src/backend/utils/cache/syscache.c      | 1 +
 src/include/storage/lmgr.h              | 2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c
index aca2c8fc742..c7e2d825120 100644
--- a/src/backend/utils/activity/wait_event.c
+++ b/src/backend/utils/activity/wait_event.c
@@ -22,9 +22,11 @@
  */
 #include "postgres.h"
 
-#include "storage/lmgr.h"		/* for GetLockNameFromTagType */
-#include "storage/lwlock.h"		/* for GetLWLockIdentifier */
+#include "storage/lmgr.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
 #include "storage/spin.h"
+#include "utils/hsearch.h"
 #include "utils/wait_event.h"

diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index 007a9a15d71..f4233f9e31a 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -32,6 +32,7 @@
 #include "lib/qunique.h"
 #include "miscadmin.h"
 #include "storage/lmgr.h"
+#include "storage/lock.h"
 #include "utils/catcache.h"
 #include "utils/inval.h"
 #include "utils/lsyscache.h"
diff --git a/src/include/storage/lmgr.h b/src/include/storage/lmgr.h
index 74a398ffc00..2a985ce5e15 100644
--- a/src/include/storage/lmgr.h
+++ b/src/include/storage/lmgr.h
@@ -16,7 +16,7 @@
 
 #include "lib/stringinfo.h"
 #include "storage/itemptr.h"
-#include "storage/lock.h"
+#include "storage/locktag.h"
 #include "utils/rel.h"

-- 
2.47.3

From 35b09d1a4e4e3e65c0754c88098259974c9a1973 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email-masked]>
Date: Sat, 21 Mar 2026 20:49:41 +0100
Subject: [PATCH 3/4] namespace.h doesn't need lock.h, only lockdefs.h

---
 src/backend/access/common/relation.c  | 1 +
 src/backend/commands/conversioncmds.c | 1 +
 src/backend/commands/discard.c        | 1 +
 src/backend/parser/parse_oper.c       | 1 +
 src/backend/utils/cache/ts_cache.c    | 1 +
 src/include/catalog/namespace.h       | 2 +-
 6 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/common/relation.c b/src/backend/access/common/relation.c
index 92a6e37d8bd..2e3a37c9272 100644
--- a/src/backend/access/common/relation.c
+++ b/src/backend/access/common/relation.c
@@ -24,6 +24,7 @@
 #include "access/xact.h"
 #include "catalog/namespace.h"
 #include "pgstat.h"
+#include "storage/lock.h"
 #include "storage/lmgr.h"
 #include "utils/inval.h"
 #include "utils/syscache.h"
diff --git a/src/backend/commands/conversioncmds.c b/src/backend/commands/conversioncmds.c
index 61aa8bb9fd1..5f2022d3072 100644
--- a/src/backend/commands/conversioncmds.c
+++ b/src/backend/commands/conversioncmds.c
@@ -19,6 +19,7 @@
 #include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
 #include "commands/conversioncmds.h"
+#include "fmgr.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
 #include "parser/parse_func.h"
diff --git a/src/backend/commands/discard.c b/src/backend/commands/discard.c
index 7b5520b9abe..17d172df076 100644
--- a/src/backend/commands/discard.c
+++ b/src/backend/commands/discard.c
@@ -19,6 +19,7 @@
 #include "commands/discard.h"
 #include "commands/prepare.h"
 #include "commands/sequence.h"
+#include "storage/lock.h"
 #include "utils/guc.h"
 #include "utils/portal.h"
 
diff --git a/src/backend/parser/parse_oper.c b/src/backend/parser/parse_oper.c
index a6b402f2d7b..2f218c1ab8b 100644
--- a/src/backend/parser/parse_oper.c
+++ b/src/backend/parser/parse_oper.c
@@ -25,6 +25,7 @@
 #include "parser/parse_oper.h"
 #include "parser/parse_type.h"
 #include "utils/builtins.h"
+#include "utils/hsearch.h"
 #include "utils/inval.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
diff --git a/src/backend/utils/cache/ts_cache.c b/src/backend/utils/cache/ts_cache.c
index 744c8e71d71..9e29f1386b0 100644
--- a/src/backend/utils/cache/ts_cache.c
+++ b/src/backend/utils/cache/ts_cache.c
@@ -44,6 +44,7 @@
 #include "utils/catcache.h"
 #include "utils/fmgroids.h"
 #include "utils/guc_hooks.h"
+#include "utils/hsearch.h"
 #include "utils/inval.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index 1a25973685c..9453a3e4932 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -15,7 +15,7 @@
 #define NAMESPACE_H
 
 #include "nodes/primnodes.h"
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
 #include "storage/procnumber.h"

-- 
2.47.3

From 13fc9223da3fab2cc958e302c00175a6e0c516c8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email-masked]>
Date: Sat, 21 Mar 2026 20:53:54 +0100
Subject: [PATCH 4/4] vacuum.h doesn't need lock.h

---
 src/backend/access/nbtree/nbtree.c    | 1 +
 src/backend/catalog/pg_subscription.c | 1 +
 src/include/commands/vacuum.h         | 1 -
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index aed74590cf4..6d870e4ebe7 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -30,6 +30,7 @@
 #include "storage/indexfsm.h"
 #include "storage/ipc.h"
 #include "storage/lmgr.h"
+#include "storage/lwlock.h"
 #include "storage/read_stream.h"
 #include "utils/datum.h"
 #include "utils/fmgrprotos.h"
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 3673d4f0bc1..1d140c513d0 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -26,6 +26,7 @@
 #include "foreign/foreign.h"
 #include "miscadmin.h"
 #include "storage/lmgr.h"
+#include "storage/lock.h"
 #include "utils/acl.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 953a506181e..1f45bca015c 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -23,7 +23,6 @@
 #include "catalog/pg_type.h"
 #include "parser/parse_node.h"
 #include "storage/buf.h"
-#include "storage/lock.h"
 #include "utils/relcache.h"
 
 /*
-- 
2.47.3

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 23 Mar 2026 08:18:05 +0000

Hi,

On Sun, Mar 22, 2026 at 08:37:47PM +0100, Álvaro Herrera wrote:
> On 2026-Mar-21, Álvaro Herrera wrote:
> 
> > I checked this, and found a couple of headers that can benefit from a
> > removal, as shown in the attached patches.
> 
> I looked again and found some more; the first 14 patches attached here
> do so.  A couple of them have no fallout to speak of, which I find
> really surprising; for the majority we just need a couple of extra
> includes somewhere or a typedef or two.  I unleashed CI on it to see
> what would happen,
> https://cirrus-ci.com/build/5522804717649920

I looked at the C files changes that now need to include lock.h (or other ones)
directly:

verify_heapam.c: lock.h was indirectly included through procarray.h
heapam_handler.c: lock.h was indirectly included through heapam.h -> tableam.h
-> vacuum.h
relation.c: lock.h was indirectly included through namespace.h
reloptions.c: lock.h was indirectly included through reloptions.h
indexam.c: lock.h was indirectly included through reloptions.h
relcache.c: lock.h was indirectly included through reloptions.h
syscache.c: lock.h was indirectly included through lmgr.h
discard.c: lock.h was indirectly included through namespace.h
pg_subscription.c: lock.h was indirectly included through heapam.h -> tableam.h
-> vacuum.h
nbtree.c: lock.h was indirectly included through vacuum.h
nbtutils.c: lock.h was indirectly included through reloptions.h
pg_inherits.c: lock.h was indirectly included through pg_inherits.h
inherit.c: lock.h was indirectly included through pg_inherits.h
conversioncmds.c: lock.h was indirectly included parse_func.h -> namespace.h
tablespace.c: lock.h was indirectly included through heapam.h -> tableam.h
-> vacuum.h
parse_oper.c: lock.h was indirectly included through parse_func.h -> namespace.h
sequencesync.c: lock.h was indirectly included through worker_internal.h
ts_cache.c: lock.h was indirectly included through namespace.h

So the changes done in your patches make sense to me.

I have 2 comments:

1/ wait_event.c

hsearch.h is already included into shmem.h so its direct include is not needed.
That said wait_event.c needs it so including it directly might make sense just from
a coding "style" point of view (given that it is  harmless as it is protected by
ifndef HSEARCH_H).

2/ Not directly linked to your patches

It looks like that aio_funcs.c does not need lock.h (reported by include-what-you-use).
If we remove its direct include, it's still indirectly included through proc.h
though. But I think that removing its direct include makes sense as it's not
needed at all.

If we still need to discuss about those lock.h related changes, maybe it's
worth creating a dedicated thread?

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Tue, 24 Mar 2026 12:15:05 +0900

> worth creating a dedicated thread?

This patch set is related to an entirely different topic than what we
are discussing, so yes, a different thread would be adapted.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 24 Mar 2026 16:02:55 +0900

> of course).  And applied this part.

The main patch has some churn in proc.c and lock.c, moving some code
blocks while the main focus of the patch is to add the two pgstat()
calls to report some data, so I have moved this part into its own
commit, and applied it.  One thing to not in lock.c: we will calculate
the time difference of the wait even if log_lock_waits is disabled,
without the lock stats part.  As this GUC is enabled by default, it
does not matter much in practice, I guess, and it matters even less
with the main patch merged.

The implementation of the main patch is close enough to pgstat_io.c
that your logic is a no-brainer (timestamp protected by the first
LWLock, each field incremented depending on locktags, etc.).  Instead
of a boolean flag tracking if some stats have been set, we could also
have used an approach like the checkpointer stats with is_all_zeros on
the pending data, perhaps?  At the end, with two incrementation
routines, I've let the code as-is.

Another thing I am not completely sure is if the sleep time of the
isolation tests is long enough.  I have tested things with
CLOBBER_CACHE_ALWAYS to make the setup more sensitive to timings but
could not get it to fail.  We'll know soon enough if the buildfarm
complains.

After a few more tweaks here and there (code, comments, some
beautification), done.
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Tue, 24 Mar 2026 16:09:37 -0400

Hi,

> After a few more tweaks here and there (code, comments, some

The test is extremely unstable on windows.  On CI 10/16 runs since the test in
failed due to it, afaict.

I don't see how a test with a timeout setting that's anywhere remotely close
to 10ms could be expected to be stable.

Also, anything that requires short sleeps (like pg_sleep(0.05);) is extremely
likely to be a long time test stability hazard. It's a huge "test smell" to
me, to the point that I think every single sleep in a test needs a comment
explaining why that one use of sleep is correct, and that comment better be
signed in blood.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Wed, 25 Mar 2026 08:46:23 +0900

> failed due to it, afaict.

I am not surprised by that.  Windows is good in catching race
conditions.

> to 10ms could be expected to be stable.

Well, the low value of deadlock_timeout is not the problem.  The
shorter the better to make the test go faster with more deadlock
checks.  What the buildfarm is telling is that we do not have the time
to process the deadlock_timeout request, and that we would need to pay
the cost of longer sleep if coded this way.  This is going to cost in
runtime on fast machines where the CheckDeadLock() call would happen
quickly. And on slow machines, we don't have the guarantee that the
sleep would be long enough to process the deadlock request.

This test would be better if rewritten as a TAP test, I guess, with a
NOTICE injection point before the CheckDeadLock() call in ProcSleep()
to make sure that the second session processes the deadlock timeout
request while it is waiting on its lock to be acquired.  One trickier
part is that we only care about the deadlock_timeout in s2, because we
want to measure the wait it has waited until the lock could be
acquired, meaning that we should make s1 use a large deadlock_timeout
to avoid interferences with a global injpoint.

I don't have the credits to test that in the CI for this month,
unfortunately, and this creates noise in the CI for the work of other
folks in this release cycle, so I am going to remove this test for
now.

> explaining why that one use of sleep is correct, and that comment better be

Yep, agreed.
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 25 Mar 2026 03:15:19 +0000

Hi,

> > failed due to it, afaict.

Thanks for the warning!

> Well, the low value of deadlock_timeout is not the problem. 

Yeah, there are other tests making use of a 10ms deadlock timeout.

> folks in this release cycle, so I am going to remove this test for

Thanks for the test removal. I created an open item for v19 so that we
don't forget to re-add a new version of the tests. I'll work on it.

[1]: https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 25 Mar 2026 03:32:05 +0000

Hi,

On Tue, Mar 24, 2026 at 05:16:11PM +0100, Álvaro Herrera wrote:
> Hello,
> 
> On 2026-Mar-23, Bertrand Drouvot wrote:
> 
> I'm not opposed to somebody else making this change, if they want, but I
> think there's little practical impact.

yeah, not sure it deserves its own commit... I was just mentioning here in case
you want to change that in passing while working on lock.h includes.

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Wed, 25 Mar 2026 14:25:01 +0900

> [1]: https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items

Thanks for adding that.  We are most likely going to need the help of
the CI here.  The buildfarm has been fortunately (unfortunately?)
stable on this one, and that would make for less noise overall.
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 25 Mar 2026 07:24:15 +0000

Hi,

On Wed, Mar 25, 2026 at 02:25:01PM +0900, Michael Paquier wrote:
> the CI here. 

Yeah. FWIW I always use the CI before submiting patches. This one was
green and did not catch the failure:

https://github.com/bdrouvot/postgres/runs/67730691634

Would probably had to be launched multiple times to catch it (as Andres mentioned
that 10/16 runs failed).

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 25 Mar 2026 19:54:42 +0000

Hi,

> stable on this one, and that would make for less noise overall.

PFA a new version of the tests using an injection point. This new injection
point is created in ProcSleep() when we know that the deadlock timeout fired.

The patch adds a new query_until_stderr() subroutine in BackgroundPsql.pm:
It does the same as query_until() except that it is waiting for a desired
stderr (and not stdout). Thanks to it the session can wait until it gets the
injection point notice.

I think that looks clearer that way than using the logfile to look for the
notice (should log_min_messages be set to an appropriate value).

If you like the idea, maybe we could introduce query_until_stderr() in a separate
commit. If you don't, then we could switch to looking in the server logfile
instead of the session stderr.

Then the new tests follow this workflow:

- session 1 holds a lock
- session 2 attaches to the new injection point with the notice action
- session 2 is blocked by session 1 and waits until the injection point notice
  is received
- session 1 releases its lock, session 2 commits
- pg_stat_lock is polled until we get the counters for the lock type or die
with a timeout

That way there is no sleep at all. Once we know that session 2 has waited longer
than the deadlock timeout (thanks to the new injection point notice) then we
can release the locks and poll pg_stat_lock to get the updated stats.

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Thu, 26 Mar 2026 08:06:20 +0900

> instead of the session stderr.

I like the patch, but I happen to not like my initial idea of relying
on a NOTICE in an injection point combined with your new API in
BackgroundPsql because we can already achieve the same with a wait
injection point and use BackgroundPsql::query_until() with an \echo to
detect that the command is blocked.

The updated version attached uses this method (edited quickly your
commit message).  Like your patch there is no need for hardcoded
sleeps and the CI's first impressions are actually good, but I am
going to need more runs to gain more confidence.  Note I should be
able to do something here in 10 days or so.  If you could confirm the
stability on your side for the time being with more runs, that would
help, of course.
--
Michael
From fd56d27596834eb019460bffb038aebfa0fa94a4 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Thu, 26 Mar 2026 08:03:31 +0900
Subject: [PATCH v2] Add tests for lock stats, take two

commit 7c64d56fd97 removed isolation test lock-stats because it was
unstable in the CI for Windows, at least.  This commit creates a new
test for the lock stats using an injection point.  This new injection
point is created in ProcSleep() when we know that the deadlock timeout
fired.

Then the new tests follow this workflow:
- session 1 holds a lock
- session 2 attaches to the new injection point with the wait action.
- session 2 is blocked by session 1 and waits until the injection point
wait is reached.
- session 1 releases its lock, session 2 commits
- pg_stat_lock is polled until we get the counters for the lock type or
die with a timeout.

That way there is no sleep at all.  Once we know that session 2 has
waited longer the deadlock_timeout (thanks to the new injection point
wait) then we can poll pg_stat_lock to get the updated stats.

Author: Bertrand Drouvot <[email-masked]>
Discussion: https://postgr.es/m/acNTR1lLHwQJ0o%2BP%40ip-10-97-1-34.eu-west-3.compute.internal
---
 src/backend/storage/lmgr/proc.c               |   2 +
 src/test/modules/test_misc/meson.build        |   1 +
 .../modules/test_misc/t/011_lock_stats.pl     | 249 ++++++++++++++++++
 3 files changed, 252 insertions(+)
 create mode 100644 src/test/modules/test_misc/t/011_lock_stats.pl

diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 5c47cf13473e..b857a10354f7 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -52,6 +52,7 @@
 #include "storage/procsignal.h"
 #include "storage/spin.h"
 #include "storage/standby.h"
+#include "utils/injection_point.h"
 #include "utils/timeout.h"
 #include "utils/timestamp.h"
 #include "utils/wait_event.h"
@@ -1560,6 +1561,7 @@ ProcSleep(LOCALLOCK *locallock)
 			int			usecs;
 			long		msecs;
 
+			INJECTION_POINT("deadlock-timeout-fired", NULL);
 			TimestampDifference(get_timeout_start_time(DEADLOCK_TIMEOUT),
 								GetCurrentTimestamp(),
 								&secs, &usecs);
diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build
index 6e8db1621a74..1b25d98f7f33 100644
--- a/src/test/modules/test_misc/meson.build
+++ b/src/test/modules/test_misc/meson.build
@@ -19,6 +19,7 @@ tests += {
       't/008_replslot_single_user.pl',
       't/009_log_temp_files.pl',
       't/010_index_concurrently_upsert.pl',
+      't/011_lock_stats.pl',
     ],
     # The injection points are cluster-wide, so disable installcheck
     'runningcheck': false,
diff --git a/src/test/modules/test_misc/t/011_lock_stats.pl b/src/test/modules/test_misc/t/011_lock_stats.pl
new file mode 100644
index 000000000000..4281e842c981
--- /dev/null
+++ b/src/test/modules/test_misc/t/011_lock_stats.pl
@@ -0,0 +1,249 @@
+
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# Test for the lock statistics
+#
+# This test creates multiple locking situations when a session (s2) has to
+# wait on a lock for longer than deadlock_timeout. The first tests each test a
+# dedicated lock type.
+# The last one checks that log_lock_waits has no impact on the statistics
+# counters.
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+plan skip_all => 'Injection points not supported by this build'
+  unless $ENV{enable_injection_points} eq 'yes';
+
+my $deadlock_timeout = 10;
+my $s1;
+my $s2;
+my $node;
+
+# Setup the 2 sessions
+sub setup_sessions
+{
+	$s1 = $node->background_psql('postgres');
+	$s2 = $node->background_psql('postgres', on_error_stop => 0);
+
+	# Setup injection points for the waiting session
+	$s2->query_safe(q[
+			SELECT injection_points_set_local();
+			SELECT injection_points_attach('deadlock-timeout-fired', 'wait');
+		]);
+}
+
+# Fetch waits and wait_time from pg_stat_lock for a given lock type
+# until they reached expected values: at least one wait and waiting longer
+# than the deadlock_timeout.
+sub wait_for_pg_stat_lock
+{
+	my ($node, $lock_type) = @_;
+
+	$node->poll_query_until(
+		'postgres', qq[
+		SELECT waits > 0 AND wait_time >= $deadlock_timeout
+		FROM pg_stat_lock
+		WHERE locktype = '$lock_type';
+	]) or die "Timed out waiting for pg_stat_lock for $lock_type"
+}
+
+# Convenience wrapper to wait for a point, then detach it.
+sub wait_and_detach
+{
+	my ($node, $point_name)  = @_;
+
+	$node->wait_for_event('client backend', $point_name);
+	$node->safe_psql('postgres',
+		"SELECT injection_points_detach('$point_name');");
+	$node->safe_psql('postgres',
+		"SELECT injection_points_wakeup('$point_name');");
+}
+
+# Node initialization
+$node = PostgreSQL::Test::Cluster->new('node');
+$node->init();
+$node->append_conf('postgresql.conf', "deadlock_timeout = ${deadlock_timeout}ms");
+$node->start();
+
+# Check if the extension injection_points is available
+plan skip_all => 'Extension injection_points not installed'
+  unless $node->check_extension('injection_points');
+
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+
+$node->safe_psql(
+	'postgres', q[
+CREATE TABLE test_stat_tab(key text not null, value int);
+INSERT INTO test_stat_tab(key, value) VALUES('k0', 1);
+]);
+
+############################################################################
+
+####### Relation lock
+
+setup_sessions();
+
+$s1->query_safe(
+	q[
+SELECT pg_stat_reset_shared('lock');
+BEGIN;
+LOCK TABLE test_stat_tab;
+]);
+
+# s2 setup
+$s2->query_safe(
+	q[
+BEGIN;
+SELECT pg_stat_force_next_flush();
+]);
+# s2 blocks on LOCK.
+$s2->query_until(
+	qr/lock_s2/, q[
+\echo lock_s2
+LOCK TABLE test_stat_tab;
+]);
+
+wait_and_detach($node, 'deadlock-timeout-fired');
+
+# deadlock_timeout fired, now commit in s1 and s2
+$s1->query_safe(q(COMMIT));
+$s2->query_safe(q(COMMIT));
+
+# check that pg_stat_lock has been updated
+wait_for_pg_stat_lock($node, 'relation');
+ok(1, "Lock stats ok for relation");
+
+# close sessions
+$s1->quit;
+$s2->quit;
+
+####### transaction lock
+
+setup_sessions();
+
+$s1->query_safe(
+	q[
+SELECT pg_stat_reset_shared('lock');
+INSERT INTO test_stat_tab(key, value) VALUES('k1', 1), ('k2', 1), ('k3', 1);
+BEGIN;
+UPDATE test_stat_tab SET value = value + 1 WHERE key = 'k1';
+]);
+
+# s2 setup
+$s2->query_safe(
+	q[
+SET log_lock_waits = on;
+BEGIN;
+SELECT pg_stat_force_next_flush();
+]);
+# s2 blocks here on UPDATE
+$s2->query_until(
+	qr/lock_s2/, q[
+\echo lock_s2
+UPDATE test_stat_tab SET value = value + 1 WHERE key = 'k1';
+]);
+
+wait_and_detach($node, 'deadlock-timeout-fired');
+
+# deadlock_timeout fired, now commit in s1 and s2
+$s1->query_safe(q(COMMIT));
+$s2->query_safe(q(COMMIT));
+
+# check that pg_stat_lock has been updated
+wait_for_pg_stat_lock($node, 'transactionid');
+ok(1, "Lock stats ok for transactionid");
+
+# Close sessions
+$s1->quit;
+$s2->quit;
+
+####### advisory lock
+
+setup_sessions();
+
+$s1->query_safe(
+	q[
+SELECT pg_stat_reset_shared('lock');
+SELECT pg_advisory_lock(1);
+]);
+
+# s2 setup
+$s2->query_safe(
+	q[
+SET log_lock_waits = on;
+BEGIN;
+SELECT pg_stat_force_next_flush();
+]);
+# s2 blocks on the advisory lock.
+$s2->query_until(
+	qr/lock_s2/, q[
+\echo lock_s2
+SELECT pg_advisory_lock(1);
+]);
+
+wait_and_detach($node, 'deadlock-timeout-fired');
+
+# deadlock_timeout fired, now unlock and commit s2
+$s1->query_safe(q(SELECT pg_advisory_unlock(1)));
+$s2->query_safe(
+	q[
+SELECT pg_advisory_unlock(1);
+COMMIT;
+]);
+
+# check that pg_stat_lock has been updated
+wait_for_pg_stat_lock($node, 'advisory');
+ok(1, "Lock stats ok for advisory");
+
+# Close sessions
+$s1->quit;
+$s2->quit;
+
+####### Ensure log_lock_waits has no impact
+
+setup_sessions();
+
+$s1->query_safe(
+	q[
+SELECT pg_stat_reset_shared('lock');
+BEGIN;
+LOCK TABLE test_stat_tab;
+]);
+
+# s2 setup
+$s2->query_safe(
+	q[
+SET log_lock_waits = off;
+BEGIN;
+SELECT pg_stat_force_next_flush();
+]);
+# s2 blocks on LOCK.
+$s2->query_until(
+	qr/lock_s2/, q[
+\echo lock_s2
+LOCK TABLE test_stat_tab;
+]);
+
+wait_and_detach($node, 'deadlock-timeout-fired');
+
+# deadlock_timeout fired, now commit in s1 and s2
+$s1->query_safe(q(COMMIT));
+$s2->query_safe(q(COMMIT));
+
+# check that pg_stat_lock has been updated
+wait_for_pg_stat_lock($node, 'relation');
+ok(1, "log_lock_waits has no impact on Lock stats");
+
+# close sessions
+$s1->quit;
+$s2->quit;
+
+# cleanup
+$node->safe_psql('postgres', q[DROP TABLE test_stat_tab;]);
+
+done_testing();
-- 
2.53.0

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 26 Mar 2026 04:05:40 +0000

Hi,

> detect that the command is blocked.

Yeah that works too.

> sleeps and the CI's first impressions are actually good,

Same here.

> but I am

With wait + echo we don't need s2 to "on_error_stop => 0" anymore. I changed
that in the attached. I'll run more CI tests during those 10 days. Let's sync
up when you'll be about to push it.

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Thu, 26 Mar 2026 15:53:37 +0900

On Thu, Mar 26, 2026 at 04:05:40AM +0000, Bertrand Drouvot wrote:
> that in the attached.

Indeed.

> I'll run more CI tests during those 10 days. Let's sync
.
Thanks.
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 31 Mar 2026 07:10:51 +0000

Hi,

On Mon, Mar 30, 2026 at 06:11:17PM +0200, Tomas Vondra wrote:
> Hi,
> 
> Isn't pgstat_lock_flush_cb a bit broken with nowait=true? It'll skip
> flushing stats for that particular lock type, but then it'll happily
> reset the pending stats anyway, forgetting the stats.
> 
> AFAIK it should keep the pending stats, and flush them sometime lager,
> when the lock is not contended. That's what the other flush callbacks
> do, at least. This probably means it needs to reset the entries one by
> one, not the whole struct at once.

Oh right, it's currently misbehaving, thanks for the warning!

> TBH I'm rather skeptical about having one lock per entry. Sure, it
> allows two backends to write different entries concurrently. But is it
> actually worth it? With nowait=true it might even be cheaper to try with
> a single lock, and AFAICS that's the case where it matters.
> 
> I wouldn't be surprised if this behaved quite poorly with contended
> cases, because the backends will be accessing the locks in exactly the
> same order and synchronize. So if one lock is contended, won't that
> "synchronize" the backends, making the other locks contended too?
> 
> Has anyone tested it actually improves the behavior? I only quickly
> skimmed the thread, I might have missed it ...
> 

I just did a few tests, with a per entry lock version fixed (to avoid the bug
mentioned above) and with a single lock.

The test is this one:

Setup:

deadlock_timeout set to 1ms.

CREATE TABLE t1(id int primary key, v int);
CREATE TABLE t2(id int primary key, v int);
INSERT INTO t1 SELECT i, 0 FROM generate_series(1,100) i;
INSERT INTO t2 SELECT i, 0 FROM generate_series(1,100) i;

test.sql:

\set id1 random(1, 100)
\set id2 random(1, 100)
BEGIN;
SELECT pg_advisory_xact_lock(:id1);
UPDATE t1 SET v=v+1 WHERE id=:id1;
UPDATE t2 SET v=v+1 WHERE id=:id2;
END;

Launched that way:

pgbench -c 32 -j 32 -T60 -f test.sql

One run produces, something like:

postgres=# select locktype, waits, wait_time from pg_stat_lock where waits > 0;
   locktype    | waits  | wait_time
---------------+--------+-----------
 tuple         |   5058 |      5092
 transactionid |  78287 |     79269
 advisory      | 105005 |    177253
(3 rows)

With one lock per entry, the avg (the test has been run 5 times) tps is 12099.
With one single lock, the avg (the test has been run 5 times) tps is 12118.

The difference looks like noise so that one lock per entry does not show
improved performance.

Also both kind of tests produce this perf profile:

0.00%     0.00%  postgres      postgres              [.] pgstat_lock_flush_cb

So, I'm tempted to say that one lock per entry adds complexity without observable
performance benefit. Also one single lock matches more naturally the intent of the
nowait path and I would also not be surprised if one lock per entry behaves
worse in some cases.

So, PFA a patch to move to a single lock instead.

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Mon, 6 Apr 2026 09:05:15 +0900

>> With wait + echo we don't need s2 to "on_error_stop => 0" anymore. I changed

Applied without the on_error_stop, after a few more runs in the CI,
that have all passed.  (Looking now at the rest.)
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Mon, 6 Apr 2026 14:11:21 +0900

> Oh right, it's currently misbehaving, thanks for the warning!

Not misbehaving, mistaken.  This would create loss of stats data that
should have been flushed.  I should not have missed that, sorry about
that.  A single-lock approach is also something we do for SLRU and WAL
data, and these are much hotter.

At the exception of one comment that could be simplified, the code
removed is correct, so addressed this one as well. 
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Mon, 6 Apr 2026 01:22:23 -0400

Hi,

> that have all passed.  (Looking now at the rest.)

https://cirrus-ci.com/task/6253659454963712
https://api.cirrus-ci.com/v1/artifact/task/6253659454963712/testrun/build/testrun/test_misc/011_lock_stats/log/regress_log_011_lock_stats

[04:33:31.756](0.067s) # die: error running SQL: 'psql:<stdin>:1: ERROR:  could not detach injection point "deadlock-timeout-fired"'

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Mon, 6 Apr 2026 15:19:57 +0900

> [04:33:31.756](0.067s) # die: error running SQL: 'psql:<stdin>:1: ERROR:  could not detach injection point "deadlock-timeout-fired"'

I got a way to reproduce the same error pattern with the following
trick:
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -357,6 +357,8 @@ InjectionPointDetach(const char *name)
    int         idx;
    int         max_inuse;

+   pg_usleep(5 * 1000000L);
+
    LWLockAcquire(InjectionPointLock, LW_EXCLUSIVE);

Now looking at it, and for the reason why 010 for concurrent indexes
does not complain..
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Mon, 6 Apr 2026 15:34:44 +0900

> Now looking at it, and for the reason why 010 for concurrent indexes

This one was a simple puzzle: there was a race condition between the
detach done by a local point and the wait/detach sequence.  As we want
a detach, dropping the local point is proving to work here.

I am going to do a few more runs to gain some more confidence.
Bertrand, could you confirm please?
--
Michael
From 684a26ad148a6d3d4261a633978702f76d7ee537 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Mon, 6 Apr 2026 15:34:03 +0900
Subject: [PATCH] Fix detach timing problem in lock stats test

---
 src/test/modules/test_misc/t/011_lock_stats.pl | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/test/modules/test_misc/t/011_lock_stats.pl b/src/test/modules/test_misc/t/011_lock_stats.pl
index 58a0046a52c3..908ade55a05f 100644
--- a/src/test/modules/test_misc/t/011_lock_stats.pl
+++ b/src/test/modules/test_misc/t/011_lock_stats.pl
@@ -31,9 +31,8 @@ sub setup_sessions
 	$s2 = $node->background_psql('postgres');
 
 	# Setup injection points for the waiting session
-	$s2->query_safe(
-		q[
-			SELECT injection_points_set_local();
+	$s2->query_until(qr/attaching_injection_point/, q[
+			\echo attaching_injection_point
 			SELECT injection_points_attach('deadlock-timeout-fired', 'wait');
 		]);
 }
@@ -59,10 +58,11 @@ sub wait_and_detach
 	my ($node, $point_name) = @_;
 
 	$node->wait_for_event('client backend', $point_name);
-	$node->safe_psql('postgres',
-		"SELECT injection_points_detach('$point_name');");
-	$node->safe_psql('postgres',
-		"SELECT injection_points_wakeup('$point_name');");
+	$node->safe_psql(
+		'postgres', qq[
+SELECT injection_points_detach('$point_name');
+SELECT injection_points_wakeup('$point_name');
+]);
 }
 
 # Node initialization
-- 
2.53.0

---

From: Michael Paquier <[email-masked]>
Date: Tue, 7 Apr 2026 13:21:39 +0900

> I am going to do a few more runs to gain some more confidence.

Done a total of 5 runs (or 6 actually), and fixed it.

> Bertrand, could you confirm please?

That's of course always welcome.  I'll keep an eye on the CI and the
buildfarm.
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 7 Apr 2026 06:01:08 +0000

Hi,

> That's of course always welcome.  I'll keep an eye on the CI and the

That looks to work, thanks! But I was wondering if this new version is not
introducing a new race: the injection point is not local anymore so it could be
that another process reach the new injection point. That said, even if this is
the case I think we're ok since s2 is using "query_until" so we could say that
"at least" s2 reached the injection point. The new version does not ensure that
"only" s2 reached the injection point but I think that's safe.

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 7 Apr 2026 06:02:35 +0000

Hi,

On Mon, Apr 06, 2026 at 02:11:21PM +0900, Michael Paquier wrote:
> I should not have missed that,

So do I...

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Wed, 8 Apr 2026 08:30:36 +0900

> "only" s2 reached the injection point but I think that's safe.

Yes, the lookups based on pg_stat_activity should be enough, I hope.
From what I can see, the buildfarm is silent this morning for this
test, as much as [1] in the CI, so I'd like to think that we are done
here.  Again, I'm hoping so.

[1]: https://cfbot.cputube.org/highlights/all.html
--
Michael