public inbox for [email protected]
help / color / mirror / Atom feedAdding locks statistics
43+ messages / 7 participants
[nested] [flat]
* Adding locks statistics
@ 2025-08-01 09:49 Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Bertrand Drouvot @ 2025-08-01 09:49 UTC (permalink / raw)
To: [email protected]
Hi hackers,
Please find attached a patch to add a new view (namely pg_stat_lock) that provides
lock statistics.
It’s output is like the following:
postgres=# select * from pg_stat_lock;
locktype | requests | waits | timeouts | deadlock_timeouts | deadlocks | fastpath | stats_reset
------------------+----------+-------+----------+-------------------+-----------+----------+-------------------------------
relation | 612775 | 1 | 0 | 0 | 0 | 531115 | 2025-08-01 09:18:26.476275+00
extend | 3128 | 0 | 0 | 0 | 0 | 0 | 2025-08-01 09:18:26.476275+00
frozenid | 11 | 0 | 0 | 0 | 0 | 0 | 2025-08-01 09:18:26.476275+00
page | 1 | 0 | 0 | 0 | 0 | 0 | 2025-08-01 09:18:26.476275+00
tuple | 3613 | 0 | 0 | 0 | 0 | 0 | 2025-08-01 09:18:26.476275+00
transactionid | 6130 | 0 | 0 | 0 | 0 | 0 | 2025-08-01 09:18:26.476275+00
virtualxid | 15390 | 0 | 0 | 0 | 0 | 15390 | 2025-08-01 09:18:26.476275+00
spectoken | 12 | 0 | 0 | 0 | 0 | 0 | 2025-08-01 09:18:26.476275+00
object | 8393 | 0 | 0 | 0 | 0 | 0 | 2025-08-01 09:18:26.476275+00
userlock | 0 | 0 | 0 | 0 | 0 | 0 | 2025-08-01 09:18:26.476275+00
advisory | 44 | 0 | 0 | 0 | 0 | 0 | 2025-08-01 09:18:26.476275+00
applytransaction | 0 | 0 | 0 | 0 | 0 | 0 | 2025-08-01 09:18:26.476275+00
It means that it provides historical trends of locks usage per lock type.
It can be used for example for:
1. checking if "waits" is close to "requests". Then it means you usually have to
wait before acquiring the lock, which means you may have a concurrency issue.
2. lock_timeout and deadlock_timeout tuning (lock_timeout is visible only in the
logs if log_min_error_statement is set appropriately).
3. checking the "requests"/"fastpath" ratio to see if "max_locks_per_transaction"
needs tuning (see c4d5cb71d2).
If any points need more details, it might be a good idea to start sampling pg_locks.
The patch is made of 2 sub-patches:
0001 - It adds a new stat kind PGSTAT_KIND_LOCK for the lock statistics.
This new statistic kind is a fixed one because its key is the lock type
so that we know its size is LOCKTAG_LAST_TYPE + 1.
This statistic kind records the following counters:
- requests: Number of requests for this lock type.
- waits: Number of times requests for this lock type had to wait.
- timeouts: Number of times requests for this lock type had to wait longer than
lock_timeout.
- deadlock_timeouts: Number of times requests for this lock type had to wait longer
than deadlock_timeout.
- deadlocks: Number of times a deadlock occurred on this lock type.
- fastpath: Number of times this lock type was taken via fast path.
No extra details is added (like the ones, i.e relation oid, database oid, we
can find in pg_locks). The idea is to provide an idea on what the locking
behaviour looks like.
Those new counters are incremented outside of the wait events code path,
as suggested in [1].
There are no major design choices, it relies on the current statistics machinery.
0002 - It adds the pg_stat_lock view
It also adds documentation and some tests.
Remarks:
- maybe we could add some metrics related to the lock duration (we have some hints
thanks to the timeout ounters though)
- if this is merged, a next step could be to record those metrics per backend
[1]: https://www.postgresql.org/message-id/CA%2BTgmobptuUWo7X5zcQrWKh22qeAn4eL%2B%3Dwtb8_ajCOR%2B7_tcw%40...
Looking forward to your feedback,
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2025-08-11 17:54 ` Greg Sabino Mullane <[email protected]>
2025-08-11 21:53 ` Re: Adding locks statistics Jeff Davis <[email protected]>
2025-08-12 09:32 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 2 replies; 43+ messages in thread
From: Greg Sabino Mullane @ 2025-08-11 17:54 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: [email protected]
Great idea. +1. Here is a quick overall review to get things started.
Meta:
patch did not apply via "git apply". Also has carriage returns (e.g. DOS
file), and some errant whitespace. Seems to pass pgindent, though.
Name:
I think the name would read better as pg_stat_locks, especially as it
returns multiple rows.
Docs: seem good. Needs a section on how to reset via
SELECT pg_stat_reset_shared('lock');
Also plural better here ('locks')
Code:
+ * Copyright (c) 2021-2025, PostgreSQL Global Development Group
If a new file, we can simply say 2025?
+ 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?
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. Busier than I realized until I saw it in this useful
view!
Cheers,
Greg
--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
@ 2025-08-11 21:53 ` Jeff Davis <[email protected]>
2025-08-11 23:44 ` Re: Adding locks statistics Michael Paquier <[email protected]>
1 sibling, 1 reply; 43+ messages in thread
From: Jeff Davis @ 2025-08-11 21:53 UTC (permalink / raw)
To: Greg Sabino Mullane <[email protected]>; Bertrand Drouvot <[email protected]>; +Cc: [email protected]
On Mon, 2025-08-11 at 13:54 -0400, Greg Sabino Mullane wrote:
> Great idea. +1. Here is a quick overall review to get things started.
Can you describe your use case? I'd like to understand whether this is
useful for users, hackers, or both.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
2025-08-11 21:53 ` Re: Adding locks statistics Jeff Davis <[email protected]>
@ 2025-08-11 23:44 ` Michael Paquier <[email protected]>
2025-08-11 23:49 ` Re: Adding locks statistics Tom Lane <[email protected]>
2025-08-12 09:37 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 2 replies; 43+ messages in thread
From: Michael Paquier @ 2025-08-11 23:44 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Greg Sabino Mullane <[email protected]>; Bertrand Drouvot <[email protected]>; [email protected]
On Mon, Aug 11, 2025 at 02:53:58PM -0700, Jeff Davis wrote:
> On Mon, 2025-08-11 at 13:54 -0400, Greg Sabino Mullane wrote:
> > Great idea. +1. Here is a quick overall review to get things started.
>
> Can you describe your use case? I'd like to understand whether this is
> useful for users, hackers, or both.
This is a DBA feature, so the questions I'd ask myself are basically:
- Is there any decision-making where these numbers would help? These
decisions would shape in tweaking the configuration of the server or
the application to as we move from a "bad" number trend to a "good"
number trend.
- What would be good numbers? In this case, most likely a threshold
reached over a certain period of time.
- Would these new stats overlap with similar statistics gathered in
the system, creating duplication and bloat in the pgstats for no real
gain?
Looking at the patch, the data gathered is this, and I don't think
that we have metrics gathered in the system to get an idea of the
contention in this area, for timeouts and deadlocks:
+ PgStat_Counter requests;
+ PgStat_Counter waits;
+ PgStat_Counter timeouts;
+ PgStat_Counter deadlock_timeouts;
+ PgStat_Counter deadlocks;
+ PgStat_Counter fastpath;
Isn't the "deadlock_timeout" one an overlap between timeout and
deadlock? Having three counters to track two concepts seems strange
to me.
Regarding the fastpath locking, you'd want to optimize the fastpath to
be close to the number of requests. If you had these numbers, one
thing that I could see a DBA do is tune max_locks_per_transaction,
which is what influences the per-backend limit of fast-path locks,
with FastPathLockGroupsPerBackend.
Using static counters for the pending data is going to be necessary
when these are called in critical sections, which is I guess why
you've done it this way, right?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
2025-08-11 21:53 ` Re: Adding locks statistics Jeff Davis <[email protected]>
2025-08-11 23:44 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2025-08-11 23:49 ` Tom Lane <[email protected]>
2025-08-12 09:49 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
1 sibling, 1 reply; 43+ messages in thread
From: Tom Lane @ 2025-08-11 23:49 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; Bertrand Drouvot <[email protected]>; [email protected]
Michael Paquier <[email protected]> writes:
> On Mon, Aug 11, 2025 at 02:53:58PM -0700, Jeff Davis wrote:
>> Can you describe your use case? I'd like to understand whether this is
>> useful for users, hackers, or both.
> This is a DBA feature, so the questions I'd ask myself are basically:
> - Is there any decision-making where these numbers would help? These
> decisions would shape in tweaking the configuration of the server or
> the application to as we move from a "bad" number trend to a "good"
> number trend.
> - What would be good numbers? In this case, most likely a threshold
> reached over a certain period of time.
> - Would these new stats overlap with similar statistics gathered in
> the system, creating duplication and bloat in the pgstats for no real
> gain?
I'm also wondering why slicing the numbers in this particular way
(i.e., aggregating by locktype) is a helpful way to look at the data.
Maybe it's just what you want, but that's not obvious to me.
regards, tom lane
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
2025-08-11 21:53 ` Re: Adding locks statistics Jeff Davis <[email protected]>
2025-08-11 23:44 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2025-08-11 23:49 ` Re: Adding locks statistics Tom Lane <[email protected]>
@ 2025-08-12 09:49 ` Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 43+ messages in thread
From: Bertrand Drouvot @ 2025-08-12 09:49 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On Mon, Aug 11, 2025 at 07:49:45PM -0400, Tom Lane wrote:
> Michael Paquier <[email protected]> writes:
> > On Mon, Aug 11, 2025 at 02:53:58PM -0700, Jeff Davis wrote:
> >> Can you describe your use case? I'd like to understand whether this is
> >> useful for users, hackers, or both.
>
> > This is a DBA feature, so the questions I'd ask myself are basically:
> > - Is there any decision-making where these numbers would help? These
> > decisions would shape in tweaking the configuration of the server or
> > the application to as we move from a "bad" number trend to a "good"
> > number trend.
> > - What would be good numbers? In this case, most likely a threshold
> > reached over a certain period of time.
> > - Would these new stats overlap with similar statistics gathered in
> > the system, creating duplication and bloat in the pgstats for no real
> > gain?
>
> I'm also wondering why slicing the numbers in this particular way
> (i.e., aggregating by locktype) is a helpful way to look at the data.
> Maybe it's just what you want, but that's not obvious to me.
Thanks for providing your thoughts!
I thought it was more natural to aggregate by locktype because:
- I think that matches how they are categorized in the doc (from a "wait event"
point of view i.e "Wait Events of Type Lock").
- It provides a natural drill-down path, spot issues by locktype in the stats and
then query pg_locks for specific objects when needed.
Does that make sense to you?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
2025-08-11 21:53 ` Re: Adding locks statistics Jeff Davis <[email protected]>
2025-08-11 23:44 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2025-08-12 09:37 ` Bertrand Drouvot <[email protected]>
2025-08-12 15:42 ` Re: Adding locks statistics Jeff Davis <[email protected]>
1 sibling, 1 reply; 43+ messages in thread
From: Bertrand Drouvot @ 2025-08-12 09:37 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On Tue, Aug 12, 2025 at 08:44:58AM +0900, Michael Paquier wrote:
> On Mon, Aug 11, 2025 at 02:53:58PM -0700, Jeff Davis wrote:
> > On Mon, 2025-08-11 at 13:54 -0400, Greg Sabino Mullane wrote:
> > > Great idea. +1. Here is a quick overall review to get things started.
> >
> > Can you describe your use case? I'd like to understand whether this is
> > useful for users, hackers, or both.
Thanks for looking at it!
I provided a few examples in the initial email ([1]):
"
It can be used for example for:
1. checking if "waits" is close to "requests". Then it means you usually have to
wait before acquiring the lock, which means you may have a concurrency issue.
2. lock_timeout and deadlock_timeout tuning (lock_timeout is visible only in the
logs if log_min_error_statement is set appropriately).
3. checking the "requests"/"fastpath" ratio to see if "max_locks_per_transaction"
needs tuning (see c4d5cb71d2).
"
Do these seem like useful use cases?
> - Is there any decision-making where these numbers would help? These
> decisions would shape in tweaking the configuration of the server or
> the application to as we move from a "bad" number trend to a "good"
> number trend.
Right, I think that could help for lock_timeout and deadlock_timeout tuning.
> - What would be good numbers? In this case, most likely a threshold
> reached over a certain period of time.
Also I think it's more a matter of ratio: waits/requests and requests/fastpath
for example.
> - Would these new stats overlap with similar statistics gathered in
> the system, creating duplication and bloat in the pgstats for no real
> gain?
I don't think there is currently stats that overlap with those.
> Looking at the patch, the data gathered is this, and I don't think
> that we have metrics gathered in the system to get an idea of the
> contention in this area, for timeouts and deadlocks:
> + PgStat_Counter requests;
> + PgStat_Counter waits;
> + PgStat_Counter timeouts;
> + PgStat_Counter deadlock_timeouts;
> + PgStat_Counter deadlocks;
> + PgStat_Counter fastpath;
>
> Isn't the "deadlock_timeout" one an overlap between timeout and
> deadlock?
I don't think so because:
- deadlock_timeout and lock_timeout are 2 distincts GUCs
- you could reach the deadlock_timeout without actually producing a deadlock
> Regarding the fastpath locking, you'd want to optimize the fastpath to
> be close to the number of requests. If you had these numbers, one
> thing that I could see a DBA do is tune max_locks_per_transaction,
> which is what influences the per-backend limit of fast-path locks,
> with FastPathLockGroupsPerBackend.
Exactly.
> Using static counters for the pending data is going to be necessary
> when these are called in critical sections, which is I guess why
> you've done it this way, right?
Yes and there is no need to over complicate this stuff as the max size is
known: LOCKTAG_LAST_TYPE + 1.
[1]: https://www.postgresql.org/message-id/aIyNxBWFCybgBZBS%40ip-10-97-1-34.eu-west-3.compute.internal
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
2025-08-11 21:53 ` Re: Adding locks statistics Jeff Davis <[email protected]>
2025-08-11 23:44 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2025-08-12 09:37 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2025-08-12 15:42 ` Jeff Davis <[email protected]>
2025-08-13 03:55 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Jeff Davis @ 2025-08-12 15:42 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; Michael Paquier <[email protected]>; +Cc: Greg Sabino Mullane <[email protected]>; [email protected]
On Tue, 2025-08-12 at 09:37 +0000, Bertrand Drouvot wrote:
> It can be used for example for:
>
> 1. checking if "waits" is close to "requests". Then it means you
> usually have to
> wait before acquiring the lock, which means you may have a
> concurrency issue.
>
> 2. lock_timeout and deadlock_timeout tuning (lock_timeout is visible
> only in the
> logs if log_min_error_statement is set appropriately).
>
> 3. checking the "requests"/"fastpath" ratio to see if
> "max_locks_per_transaction"
> needs tuning (see c4d5cb71d2).
> "
>
> Do these seem like useful use cases?
Those seem plausibly useful, but I don't recall needing that exact
information myself, and I wanted to hear more from others.
For instance, a view could be helpful to diagnose concurrency issues,
but I think that's worth discussing in more detail to see what kinds of
issues it can help with and how it complements other approaches. I
suspect when we get into the details, different people (or different
situations) would want slightly different information out of that view.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
2025-08-11 21:53 ` Re: Adding locks statistics Jeff Davis <[email protected]>
2025-08-11 23:44 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2025-08-12 09:37 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-12 15:42 ` Re: Adding locks statistics Jeff Davis <[email protected]>
@ 2025-08-13 03:55 ` Bertrand Drouvot <[email protected]>
2025-12-15 16:48 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Bertrand Drouvot @ 2025-08-13 03:55 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Michael Paquier <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On Tue, Aug 12, 2025 at 08:42:48AM -0700, Jeff Davis wrote:
> On Tue, 2025-08-12 at 09:37 +0000, Bertrand Drouvot wrote:
> > It can be used for example for:
> >
> > 1. checking if "waits" is close to "requests". Then it means you
> > usually have to
> > wait before acquiring the lock, which means you may have a
> > concurrency issue.
> >
> > 2. lock_timeout and deadlock_timeout tuning (lock_timeout is visible
> > only in the
> > logs if log_min_error_statement is set appropriately).
> >
> > 3. checking the "requests"/"fastpath" ratio to see if
> > "max_locks_per_transaction"
> > needs tuning (see c4d5cb71d2).
> > "
> >
> > Do these seem like useful use cases?
>
> Those seem plausibly useful,
Thanks for sharing your thoughts about the use cases above.
> and I wanted to hear more from others.
> For instance, a view could be helpful to diagnose concurrency issues,
> but I think that's worth discussing in more detail to see what kinds of
> issues it can help with and how it complements other approaches. I
> suspect when we get into the details, different people (or different
> situations) would want slightly different information out of that view.
Fully agree, getting input from others definitely helps validate the approach
and see if the current design needs some changes.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
2025-08-11 21:53 ` Re: Adding locks statistics Jeff Davis <[email protected]>
2025-08-11 23:44 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2025-08-12 09:37 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-12 15:42 ` Re: Adding locks statistics Jeff Davis <[email protected]>
2025-08-13 03:55 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2025-12-15 16:48 ` Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 43+ messages in thread
From: Bertrand Drouvot @ 2025-12-15 16:48 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Michael Paquier <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On Wed, Aug 13, 2025 at 03:55:41AM +0000, Bertrand Drouvot wrote:
> Fully agree, getting input from others definitely helps validate the approach
> and see if the current design needs some changes.
PFA a mandatory rebase.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
@ 2025-08-12 09:32 ` Bertrand Drouvot <[email protected]>
2025-08-12 12:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
1 sibling, 1 reply; 43+ messages in thread
From: Bertrand Drouvot @ 2025-08-12 09:32 UTC (permalink / raw)
To: Greg Sabino Mullane <[email protected]>; +Cc: [email protected]
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
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
2025-08-12 09:32 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2025-08-12 12:54 ` Greg Sabino Mullane <[email protected]>
2025-08-13 03:20 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Greg Sabino Mullane @ 2025-08-12 12:54 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: [email protected]
On Tue, Aug 12, 2025 at 5:32 AM Bertrand Drouvot <
[email protected]> wrote:
> I considered pg_stat_locks, but chose the singular form to be consistent
> with
> pg_stat_database, pg_stat_subscription, and friends.
>
Counter-examples: pg_stat_statements, pg_stat_subscription_stats. Our names
are not consistent. :) It just feels a better fit to have a plural name for
a table tracking aggregates of multiple types of objects.
(Ignore my git complaint, was obviously half-asleep when I wrote that).
> Docs: seem good. Needs a section on how to reset via
> > SELECT pg_stat_reset_shared('lock');
>
I meant something closer to the actual description of the view. Having it
buried in the pg_stat_reset_shared section is not intuitive for people
looking up the view in the docs.
> Because it looks like that they are ordered by alphabetical order.
>
That makes sense.
> - I'm not sure that's worth for this particular case and code paths.
>
Will let others opine on that.
Thanks! Did you observe some noticeable performance penalties?
>
No, but I did not give it any particularly heavy testing. More of a idle
thought when pulling up a brand new system and seeing the thousands of
locks for the tiniest bit of database action.
Cheers,
Greg
--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
2025-08-12 09:32 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-12 12:54 ` Re: Adding locks statistics Greg Sabino Mullane <[email protected]>
@ 2025-08-13 03:20 ` Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 43+ messages in thread
From: Bertrand Drouvot @ 2025-08-13 03:20 UTC (permalink / raw)
To: Greg Sabino Mullane <[email protected]>; +Cc: [email protected]
Hi,
On Tue, Aug 12, 2025 at 08:54:07AM -0400, Greg Sabino Mullane wrote:
> On Tue, Aug 12, 2025 at 5:32 AM Bertrand Drouvot <
> [email protected]> wrote:
>
> > Docs: seem good. Needs a section on how to reset via
> > > SELECT pg_stat_reset_shared('lock');
> >
>
> I meant something closer to the actual description of the view. Having it
> buried in the pg_stat_reset_shared section is not intuitive for people
> looking up the view in the docs.
I agree that we could add this extra information in the view documentation.
But none of pg_stat_archiver, pg_stat_bgwriter, pg_stat_checkpointer, pg_stat_io,
pg_stat_slru and pg_stat_wal have done so. The only exception is pg_stat_recovery_prefetch
and for a good reason as not all of its fields are reset.
While I think that's probably a good idea, I think it's worth a dedicated
thread (so that the discussion takes into account the other views mentioned
above).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
@ 2026-03-18 04:36 Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Bertrand Drouvot @ 2026-03-18 04:36 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
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
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2026-03-18 08:15 ` Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Michael Paquier @ 2026-03-18 08:15 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Wed, Mar 18, 2026 at 04:36:04AM +0000, Bertrand Drouvot wrote:
>> 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.
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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-03-18 14:51 ` Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Bertrand Drouvot @ 2026-03-18 14:51 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On Wed, Mar 18, 2026 at 05:15:27PM +0900, Michael Paquier wrote:
> On Wed, Mar 18, 2026 at 04:36:04AM +0000, Bertrand Drouvot wrote:
> >> 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.
>
> 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.
Thanks for looking at it!
> 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.
Yeah, added a few words in the callers and on top of the function definitions.
> 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.
Yeah, that was also my vote.
> 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?
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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2026-03-19 07:01 ` Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Michael Paquier @ 2026-03-19 07:01 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
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?
+ * 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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-03-19 12:25 ` Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Bertrand Drouvot @ 2026-03-19 12:25 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
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
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2026-03-21 05:55 ` Michael Paquier <[email protected]>
2026-03-21 20:01 ` Re: Adding locks statistics Álvaro Herrera <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
0 siblings, 2 replies; 43+ messages in thread
From: Michael Paquier @ 2026-03-21 05:55 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Thu, Mar 19, 2026 at 12:25:41PM +0000, Bertrand Drouvot wrote:
> 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.
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.
> 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).
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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-03-21 20:01 ` Álvaro Herrera <[email protected]>
2026-03-23 08:18 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
1 sibling, 1 reply; 43+ messages in thread
From: Álvaro Herrera @ 2026-03-21 20:01 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; Bertrand Drouvot <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; L. pgsql-hackers <[email protected]>
On 2026-03-21, Michael Paquier wrote:
> On Thu, Mar 19, 2026 at 12:25:41PM +0000, Bertrand Drouvot wrote:
>> 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).
>
> I have checked after that, and did not spot an area (except your patch
> 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
Attachments:
[text/x-patch] 0001-procarray.h-does-not-need-storage-lock.h-for-anythin.patch (741B, 2-0001-procarray.h-does-not-need-storage-lock.h-for-anythin.patch)
download | inline diff:
From aa6155106b7f5dc697df54dc4e4aeb5c6419ec92 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]>
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
[text/x-patch] 0002-lmgr.h-doesn-t-need-the-full-lock.h.patch (1.7K, 3-0002-lmgr.h-doesn-t-need-the-full-lock.h.patch)
download | inline diff:
From c09489b4164d0d8c7942ca20a9fb645a1d68ea7f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]>
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
[text/x-patch] 0003-namespace.h-doesn-t-need-lock.h-only-lockdefs.h.patch (2.9K, 4-0003-namespace.h-doesn-t-need-lock.h-only-lockdefs.h.patch)
download | inline diff:
From 35b09d1a4e4e3e65c0754c88098259974c9a1973 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]>
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
[text/x-patch] 0004-vacuum.h-doesn-t-need-lock.h.patch (1.6K, 5-0004-vacuum.h-doesn-t-need-lock.h.patch)
download | inline diff:
From 13fc9223da3fab2cc958e302c00175a6e0c516c8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]>
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
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-21 20:01 ` Re: Adding locks statistics Álvaro Herrera <[email protected]>
@ 2026-03-23 08:18 ` Bertrand Drouvot <[email protected]>
2026-03-24 03:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:32 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 2 replies; 43+ messages in thread
From: Bertrand Drouvot @ 2026-03-23 08:18 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; L. pgsql-hackers <[email protected]>
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
Thanks for looking at it!
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
-#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"
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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-21 20:01 ` Re: Adding locks statistics Álvaro Herrera <[email protected]>
2026-03-23 08:18 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2026-03-24 03:15 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 43+ messages in thread
From: Michael Paquier @ 2026-03-24 03:15 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; L. pgsql-hackers <[email protected]>
On Mon, Mar 23, 2026 at 08:18:05AM +0000, Bertrand Drouvot wrote:
> 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?
This patch set is related to an entirely different topic than what we
are discussing, so yes, a different thread would be adapted.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-21 20:01 ` Re: Adding locks statistics Álvaro Herrera <[email protected]>
2026-03-23 08:18 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2026-03-25 03:32 ` Bertrand Drouvot <[email protected]>
1 sibling, 0 replies; 43+ messages in thread
From: Bertrand Drouvot @ 2026-03-25 03:32 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; L. pgsql-hackers <[email protected]>
Hi,
On Tue, Mar 24, 2026 at 05:16:11PM +0100, Álvaro Herrera wrote:
> Hello,
>
> On 2026-Mar-23, Bertrand Drouvot wrote:
>
> > 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.
>
> 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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-03-24 07:02 ` Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-31 07:10 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
1 sibling, 2 replies; 43+ messages in thread
From: Michael Paquier @ 2026-03-24 07:02 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Sat, Mar 21, 2026 at 02:55:33PM +0900, Michael Paquier wrote:
> On Thu, Mar 19, 2026 at 12:25:41PM +0000, Bertrand Drouvot wrote:
>> 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).
>
> I have checked after that, and did not spot an area (except your patch
> 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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-03-24 20:09 ` Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
1 sibling, 1 reply; 43+ messages in thread
From: Andres Freund @ 2026-03-24 20:09 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On 2026-03-24 16:02:55 +0900, Michael Paquier wrote:
> 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.
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
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
@ 2026-03-24 23:46 ` Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Michael Paquier @ 2026-03-24 23:46 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Tue, Mar 24, 2026 at 04:09:37PM -0400, Andres Freund wrote:
> The test is extremely unstable on windows. On CI 10/16 runs since the test in
> failed due to it, afaict.
I am not surprised by that. Windows is good in catching race
conditions.
> I don't see how a test with a timeout setting that's anywhere remotely close
> 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.
> 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.
Yep, agreed.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-03-25 03:15 ` Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Bertrand Drouvot @ 2026-03-25 03:15 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On Wed, Mar 25, 2026 at 08:46:23AM +0900, Michael Paquier wrote:
> On Tue, Mar 24, 2026 at 04:09:37PM -0400, Andres Freund wrote:
> > The test is extremely unstable on windows. On CI 10/16 runs since the test in
> > failed due to it, afaict.
Thanks for the warning!
> I am not surprised by that. Windows is good in catching race
> conditions.
>
> > I don't see how a test with a timeout setting that's anywhere remotely close
> > to 10ms could be expected to be stable.
>
> Well, the low value of deadlock_timeout is not the problem.
Yeah, there are other tests making use of a 10ms deadlock timeout.
> 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.
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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2026-03-25 05:25 ` Michael Paquier <[email protected]>
2026-03-25 07:24 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 19:54 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 2 replies; 43+ messages in thread
From: Michael Paquier @ 2026-03-25 05:25 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Wed, Mar 25, 2026 at 03:15:19AM +0000, Bertrand Drouvot wrote:
> 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
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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-03-25 07:24 ` Bertrand Drouvot <[email protected]>
1 sibling, 0 replies; 43+ messages in thread
From: Bertrand Drouvot @ 2026-03-25 07:24 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On Wed, Mar 25, 2026 at 02:25:01PM +0900, Michael Paquier wrote:
> On Wed, Mar 25, 2026 at 03:15:19AM +0000, Bertrand Drouvot wrote:
> > 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
>
> Thanks for adding that. We are most likely going to need the help of
> 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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-03-25 19:54 ` Bertrand Drouvot <[email protected]>
2026-03-25 23:06 ` Re: Adding locks statistics Michael Paquier <[email protected]>
1 sibling, 1 reply; 43+ messages in thread
From: Bertrand Drouvot @ 2026-03-25 19:54 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On Wed, Mar 25, 2026 at 02:25:01PM +0900, Michael Paquier wrote:
> On Wed, Mar 25, 2026 at 03:15:19AM +0000, Bertrand Drouvot wrote:
> > 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
>
> 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.
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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 19:54 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2026-03-25 23:06 ` Michael Paquier <[email protected]>
2026-03-26 04:05 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Michael Paquier @ 2026-03-25 23:06 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Wed, Mar 25, 2026 at 07:54:42PM +0000, Bertrand Drouvot wrote:
> 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.
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 protected]>
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 protected]>
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
Attachments:
[text/plain] v2-0001-Add-tests-for-lock-stats-take-two.patch (8.4K, 2-v2-0001-Add-tests-for-lock-stats-take-two.patch)
download | inline diff:
From fd56d27596834eb019460bffb038aebfa0fa94a4 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
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 protected]>
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
[application/pgp-signature] signature.asc (833B, 3-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 19:54 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 23:06 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-03-26 04:05 ` Bertrand Drouvot <[email protected]>
2026-03-26 06:53 ` Re: Adding locks statistics Michael Paquier <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Bertrand Drouvot @ 2026-03-26 04:05 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On Thu, Mar 26, 2026 at 08:06:20AM +0900, Michael Paquier wrote:
>
> 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.
Yeah that works too.
> 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,
Same here.
> 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.
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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 19:54 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 23:06 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-26 04:05 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2026-03-26 06:53 ` Michael Paquier <[email protected]>
2026-04-06 00:05 ` Re: Adding locks statistics Michael Paquier <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Michael Paquier @ 2026-03-26 06:53 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Thu, Mar 26, 2026 at 04:05:40AM +0000, Bertrand Drouvot wrote:
> With wait + echo we don't need s2 to "on_error_stop => 0" anymore. I changed
> that in the attached.
Indeed.
> I'll run more CI tests during those 10 days. Let's sync
> up when you'll be about to push it.
.
Thanks.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 19:54 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 23:06 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-26 04:05 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-26 06:53 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-04-06 00:05 ` Michael Paquier <[email protected]>
2026-04-06 05:22 ` Re: Adding locks statistics Andres Freund <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Michael Paquier @ 2026-04-06 00:05 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Thu, Mar 26, 2026 at 03:53:37PM +0900, Michael Paquier wrote:
> On Thu, Mar 26, 2026 at 04:05:40AM +0000, Bertrand Drouvot wrote:
>> With wait + echo we don't need s2 to "on_error_stop => 0" anymore. I changed
>> that in the attached.
>
> Indeed.
Applied without the on_error_stop, after a few more runs in the CI,
that have all passed. (Looking now at the rest.)
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 19:54 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 23:06 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-26 04:05 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-26 06:53 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 00:05 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-04-06 05:22 ` Andres Freund <[email protected]>
2026-04-06 06:19 ` Re: Adding locks statistics Michael Paquier <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Andres Freund @ 2026-04-06 05:22 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On 2026-04-06 09:05:15 +0900, Michael Paquier wrote:
> On Thu, Mar 26, 2026 at 03:53:37PM +0900, Michael Paquier wrote:
> > On Thu, Mar 26, 2026 at 04:05:40AM +0000, Bertrand Drouvot wrote:
> >> With wait + echo we don't need s2 to "on_error_stop => 0" anymore. I changed
> >> that in the attached.
> >
> > Indeed.
>
> Applied without the on_error_stop, after a few more runs in the CI,
> 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...
[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
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 19:54 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 23:06 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-26 04:05 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-26 06:53 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 00:05 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 05:22 ` Re: Adding locks statistics Andres Freund <[email protected]>
@ 2026-04-06 06:19 ` Michael Paquier <[email protected]>
2026-04-06 06:34 ` Re: Adding locks statistics Michael Paquier <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Michael Paquier @ 2026-04-06 06:19 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Mon, Apr 06, 2026 at 01:22:23AM -0400, Andres Freund wrote:
> https://cirrus-ci.com/task/6253659454963712
> https://api.cirrus-ci.com/v1/artifact/task/6253659454963712/testrun/build/testrun/test_misc/011_lock...
>
> [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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 19:54 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 23:06 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-26 04:05 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-26 06:53 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 00:05 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 05:22 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-04-06 06:19 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-04-06 06:34 ` Michael Paquier <[email protected]>
2026-04-07 04:21 ` Re: Adding locks statistics Michael Paquier <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Michael Paquier @ 2026-04-06 06:34 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Mon, Apr 06, 2026 at 03:19:57PM +0900, Michael Paquier wrote:
> Now looking at it, and for the reason why 010 for concurrent indexes
> does not complain..
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 protected]>
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
Attachments:
[text/plain] 0001-Fix-detach-timing-problem-in-lock-stats-test.patch (1.4K, 2-0001-Fix-detach-timing-problem-in-lock-stats-test.patch)
download | inline diff:
From 684a26ad148a6d3d4261a633978702f76d7ee537 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
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
[application/pgp-signature] signature.asc (833B, 3-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 19:54 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 23:06 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-26 04:05 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-26 06:53 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 00:05 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 05:22 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-04-06 06:19 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 06:34 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-04-07 04:21 ` Michael Paquier <[email protected]>
2026-04-07 06:01 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Michael Paquier @ 2026-04-07 04:21 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Mon, Apr 06, 2026 at 03:34:44PM +0900, Michael Paquier wrote:
> 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.
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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 19:54 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 23:06 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-26 04:05 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-26 06:53 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 00:05 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 05:22 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-04-06 06:19 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 06:34 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-07 04:21 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-04-07 06:01 ` Bertrand Drouvot <[email protected]>
2026-04-07 23:30 ` Re: Adding locks statistics Michael Paquier <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Bertrand Drouvot @ 2026-04-07 06:01 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On Tue, Apr 07, 2026 at 01:21:39PM +0900, Michael Paquier wrote:
> On Mon, Apr 06, 2026 at 03:34:44PM +0900, Michael Paquier wrote:
> > 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.
>
> 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.
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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 20:09 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-03-24 23:46 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 03:15 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-25 19:54 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-25 23:06 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-26 04:05 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-26 06:53 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 00:05 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 05:22 ` Re: Adding locks statistics Andres Freund <[email protected]>
2026-04-06 06:19 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-06 06:34 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-07 04:21 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-04-07 06:01 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2026-04-07 23:30 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 43+ messages in thread
From: Michael Paquier @ 2026-04-07 23:30 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Tue, Apr 07, 2026 at 06:01:08AM +0000, Bertrand Drouvot wrote:
> 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.
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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-03-31 07:10 ` Bertrand Drouvot <[email protected]>
2026-04-06 05:11 ` Re: Adding locks statistics Michael Paquier <[email protected]>
1 sibling, 1 reply; 43+ messages in thread
From: Bertrand Drouvot @ 2026-03-31 07:10 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-31 07:10 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
@ 2026-04-06 05:11 ` Michael Paquier <[email protected]>
2026-04-07 06:02 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 43+ messages in thread
From: Michael Paquier @ 2026-04-06 05:11 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
On Tue, Mar 31, 2026 at 07:10:51AM +0000, Bertrand Drouvot wrote:
> On Mon, Mar 30, 2026 at 06:11:17PM +0200, Tomas Vondra wrote:
>> 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!
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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 43+ messages in thread
* Re: Adding locks statistics
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-18 14:51 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-19 12:25 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-24 07:02 ` Re: Adding locks statistics Michael Paquier <[email protected]>
2026-03-31 07:10 ` Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-04-06 05:11 ` Re: Adding locks statistics Michael Paquier <[email protected]>
@ 2026-04-07 06:02 ` Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 43+ messages in thread
From: Bertrand Drouvot @ 2026-04-07 06:02 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Andres Freund <[email protected]>; Jeff Davis <[email protected]>; Greg Sabino Mullane <[email protected]>; [email protected]
Hi,
On Mon, Apr 06, 2026 at 02:11:21PM +0900, Michael Paquier wrote:
> On Tue, Mar 31, 2026 at 07:10:51AM +0000, Bertrand Drouvot wrote:
> > On Mon, Mar 30, 2026 at 06:11:17PM +0200, Tomas Vondra wrote:
> >> 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!
>
> I should not have missed that,
So do I...
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 43+ messages in thread
end of thread, other threads:[~2026-04-07 23:30 UTC | newest]
Thread overview: 43+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-08-01 09:49 Adding locks statistics Bertrand Drouvot <[email protected]>
2025-08-11 17:54 ` Greg Sabino Mullane <[email protected]>
2025-08-11 21:53 ` Jeff Davis <[email protected]>
2025-08-11 23:44 ` Michael Paquier <[email protected]>
2025-08-11 23:49 ` Tom Lane <[email protected]>
2025-08-12 09:49 ` Bertrand Drouvot <[email protected]>
2025-08-12 09:37 ` Bertrand Drouvot <[email protected]>
2025-08-12 15:42 ` Jeff Davis <[email protected]>
2025-08-13 03:55 ` Bertrand Drouvot <[email protected]>
2025-12-15 16:48 ` Bertrand Drouvot <[email protected]>
2025-08-12 09:32 ` Bertrand Drouvot <[email protected]>
2025-08-12 12:54 ` Greg Sabino Mullane <[email protected]>
2025-08-13 03:20 ` Bertrand Drouvot <[email protected]>
2026-03-18 04:36 Re: Adding locks statistics Bertrand Drouvot <[email protected]>
2026-03-18 08:15 ` Michael Paquier <[email protected]>
2026-03-18 14:51 ` Bertrand Drouvot <[email protected]>
2026-03-19 07:01 ` Michael Paquier <[email protected]>
2026-03-19 12:25 ` Bertrand Drouvot <[email protected]>
2026-03-21 05:55 ` Michael Paquier <[email protected]>
2026-03-21 20:01 ` Álvaro Herrera <[email protected]>
2026-03-23 08:18 ` Bertrand Drouvot <[email protected]>
2026-03-24 03:15 ` Michael Paquier <[email protected]>
2026-03-25 03:32 ` Bertrand Drouvot <[email protected]>
2026-03-24 07:02 ` Michael Paquier <[email protected]>
2026-03-24 20:09 ` Andres Freund <[email protected]>
2026-03-24 23:46 ` Michael Paquier <[email protected]>
2026-03-25 03:15 ` Bertrand Drouvot <[email protected]>
2026-03-25 05:25 ` Michael Paquier <[email protected]>
2026-03-25 07:24 ` Bertrand Drouvot <[email protected]>
2026-03-25 19:54 ` Bertrand Drouvot <[email protected]>
2026-03-25 23:06 ` Michael Paquier <[email protected]>
2026-03-26 04:05 ` Bertrand Drouvot <[email protected]>
2026-03-26 06:53 ` Michael Paquier <[email protected]>
2026-04-06 00:05 ` Michael Paquier <[email protected]>
2026-04-06 05:22 ` Andres Freund <[email protected]>
2026-04-06 06:19 ` Michael Paquier <[email protected]>
2026-04-06 06:34 ` Michael Paquier <[email protected]>
2026-04-07 04:21 ` Michael Paquier <[email protected]>
2026-04-07 06:01 ` Bertrand Drouvot <[email protected]>
2026-04-07 23:30 ` Michael Paquier <[email protected]>
2026-03-31 07:10 ` Bertrand Drouvot <[email protected]>
2026-04-06 05:11 ` Michael Paquier <[email protected]>
2026-04-07 06:02 ` Bertrand Drouvot <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox