Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix injection point detach timing problem in TAP test for lock stats

  2. Use single LWLock for lock statistics in pgstats

  3. Add tests for lock statistics, take two

  4. Remove isolation test lock-stats

  5. Avoid including clog.h in proc.h

  6. Don't include storage/lock.h in so many headers

  7. Add support for lock statistics in pgstats

  8. Move some code blocks in lock.c and proc.c

  9. Move declarations related to locktags from lock.h to new locktag.h

  1. Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-08-01T09:49:56Z

    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%40mail.gmail.com

Looking forward to your feedback,

Regards,

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

  2. Re: Adding locks statistics

    Greg Sabino Mullane <htamfids@gmail.com> — 2025-08-11T17:54:38Z

    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

  3. Re: Adding locks statistics

    Jeff Davis <pgsql@j-davis.com> — 2025-08-11T21:53:58Z

    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

  4. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2025-08-11T23:44:58Z

    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

  5. Re: Adding locks statistics

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-11T23:49:45Z

    Michael Paquier <michael@paquier.xyz> 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

  6. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-08-12T09:32:25Z

    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

  7. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-08-12T09:37:55Z

    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

  8. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-08-12T09:49:27Z

    Hi,

On Mon, Aug 11, 2025 at 07:49:45PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> 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

  9. Re: Adding locks statistics

    Greg Sabino Mullane <htamfids@gmail.com> — 2025-08-12T12:54:07Z

    On Tue, Aug 12, 2025 at 5:32 AM Bertrand Drouvot <
bertranddrouvot.pg@gmail.com> 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

  10. Re: Adding locks statistics

    Jeff Davis <pgsql@j-davis.com> — 2025-08-12T15:42:48Z

    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

  11. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-08-13T03:20:26Z

    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 <
> bertranddrouvot.pg@gmail.com> 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

  12. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-08-13T03:55:41Z

    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

  13. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-15T16:48:10Z

    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

  14. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-10T07:30:50Z

    Hi,

On Mon, Dec 15, 2025 at 04:48:10PM +0000, Bertrand Drouvot wrote:
> 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.

New rebase due to 73d60ac385a.

Regards,

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

  15. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-02-13T07:36:57Z

    On Tue, Feb 10, 2026 at 07:30:50AM +0000, Bertrand Drouvot wrote:
> New rebase due to 73d60ac385a.

I have been looking at this patch, and can get behind the data
gathered here in terms of being able to tune things, but not all.  See
below for the details of my reasoning.

max_locks_per_xact is a PGC_POSTMASTER, so backend-level stats with
this data would not be relevant for its tuning.  However, something
else can be said about deadlock_timeout, where one could rely on the
data gathered by this view to set it on a backend basis, particularly
if the load pattern is divided into a subsets of connections (say few
backend see a lot of the deadlock_timeout, for example).  Same
argument for lock_timeout, which is user-settable.

As the set of data gathered, I think that I'm OK with timeouts (for
lock_timeout), deadlock_timeouts (for deadlock_timeout), fastpath (for
max_locks_per_xact), that can all be compared with the number of
requests.

Regarding "deadlocks" and "waits", these two are less useful than the
three others because not really actionable.  They would become much
more relevant if and only if we know the distribution of the deadlocks
not only for the lock types, but for the objects involved, especially
if the activity is diluted across many objects.  So these have less
value IMO because they are not really actionable in the system.  The
other three can be directly tuned based on the GUCs we have.

So my suggestion for the moment would be to be more frugal (yeah I
know, sorry..) and limit ourselves to four fields: deadlock_timeout,
requests, fastpath and timeouts.  Three fields to compare with
requests, one for each GUC.

Regarding the implementation, you are right to use a fixed-sized stats
kind for the job.  I can see a lot of code has been copy-pasted from
pgstat_io.c, then slightly adjusted to fit into the picture.  That's
fine here, it makes the implementation straight-forward to read.

Regarding the documentation, listing all the values for locktype is a
recipe for rot.  I'd suggest to remove the list instead, with only a
link referring to pg_locks to avoid the duplication.
--
Michael

  16. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-13T10:24:52Z

    Hi,

On Fri, Feb 13, 2026 at 04:36:57PM +0900, Michael Paquier wrote:
> On Tue, Feb 10, 2026 at 07:30:50AM +0000, Bertrand Drouvot wrote:
> > New rebase due to 73d60ac385a.
> 
> I have been looking at this patch, and can get behind the data
> gathered here in terms of being able to tune things

Thanks!

> 
> So my suggestion for the moment would be to be more frugal (yeah I
> know, sorry..) and limit ourselves to four fields: deadlock_timeout,
> requests, fastpath and timeouts.  Three fields to compare with
> requests, one for each GUC.

That's fine by me. We could still add the others in the future if we feel the
need. Done that way in the attached.

> Regarding the implementation, you are right to use a fixed-sized stats
> kind for the job.  I can see a lot of code has been copy-pasted from
> pgstat_io.c, then slightly adjusted to fit into the picture.  That's
> fine here, it makes the implementation straight-forward to read.

Yeah, no need to reinvent the wheel.

> Regarding the documentation, listing all the values for locktype is a
> recipe for rot.  I'd suggest to remove the list instead, with only a
> link referring to pg_locks to avoid the duplication.

Makes sense, done that way. Makes me think that we could do the same for pg_locks
and just link it to the Wait Events of Type Lock? (Table 27.11.)

Regards,

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

  17. Re: Adding locks statistics

    Andres Freund <andres@anarazel.de> — 2026-02-13T21:13:39Z

    Hi,

On 2026-02-13 10:24:52 +0000, Bertrand Drouvot wrote:
> On Fri, Feb 13, 2026 at 04:36:57PM +0900, Michael Paquier wrote:
> > On Tue, Feb 10, 2026 at 07:30:50AM +0000, Bertrand Drouvot wrote:
> > > New rebase due to 73d60ac385a.
> > So my suggestion for the moment would be to be more frugal (yeah I
> > know, sorry..) and limit ourselves to four fields: deadlock_timeout,
> > requests, fastpath and timeouts.  Three fields to compare with
> > requests, one for each GUC.
> 
> That's fine by me. We could still add the others in the future if we feel the
> need. Done that way in the attached.

I'm not sure that it's unproblematic to add multiple pgstat count calls to
every lock acquisition, particularly if it's a fastpath acquisition or a
virtualxid lock. Notably these are external function calls, not just
increments of a counter in an inline function.

I also don't really know what one would do with some of the information?

What does the number of virtualxid lock acquisitions tell you that the numbers
of transactions doesn't already tell you in a more understandable way?

What does it tell you that the deadlock checker ran N times? It notably
doesn't count deadlocks, it counts how often we checked for deadlocks.

The percentage of fastpath locks also seems not really informative, because
that could be because we ran out of space for fastpath locks, or because a
lock mode that's ineligible for fastpath locks was used.


What I would actually count is the amount of time waiting for locks, that
seems vastly more useful than the number of acquisitions.  We already do a
GetCurrentTimestamp() inside the timer activations for deadlock timeout, we
probably can figure out a way to reuse that to reduce the increase in overhead
due to timing.  We could also just count the wait time after the deadlock
check has run.


> @@ -17,6 +17,7 @@
>  #include "postmaster/pgarch.h"	/* for MAX_XFN_CHARS */
>  #include "replication/conflict.h"
>  #include "replication/worker_internal.h"
> +#include "storage/lock.h"
>  #include "utils/backend_progress.h" /* for backward compatibility */	/* IWYU pragma: export */
>  #include "utils/backend_status.h"	/* for backward compatibility */	/* IWYU pragma: export */
>  #include "utils/pgstat_kind.h"
> @@ -342,6 +343,25 @@ typedef struct PgStat_IO
>  	PgStat_BktypeIO stats[BACKEND_NUM_TYPES];
>  } PgStat_IO;

I don't like the amount of headers this addition will indirectly include in a
lot of places.

I'm also pretty unhappy about an include of access/transam.h recently having
been added. And worker_internal.h quite obviously, given the name, has no
business being included here, and also includes a lot more. Grrr.  I'll start
a thread.

Greetings,

Andres Freund

  18. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-02-16T04:18:32Z

    On Fri, Feb 13, 2026 at 04:13:39PM -0500, Andres Freund wrote:
> I'm not sure that it's unproblematic to add multiple pgstat count calls to
> every lock acquisition, particularly if it's a fastpath acquisition or a
> virtualxid lock. Notably these are external function calls, not just
> increments of a counter in an inline function.

Right.  I am not completely sure if this is really free, either,
particularly for a fastpath lock that we want to be..  Faster.

> What I would actually count is the amount of time waiting for locks, that
> seems vastly more useful than the number of acquisitions.  We already do a
> GetCurrentTimestamp() inside the timer activations for deadlock timeout, we
> probably can figure out a way to reuse that to reduce the increase in overhead
> due to timing.  We could also just count the wait time after the deadlock
> check has run.

That sounds like an interesting suggestion, yes.  That's not going to
be bounded by performance if we know that we are already waiting.
--
Michael

  19. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-16T10:10:21Z

    Hi,

On Fri, Feb 13, 2026 at 04:13:39PM -0500, Andres Freund wrote:
> Hi,
> 
> On 2026-02-13 10:24:52 +0000, Bertrand Drouvot wrote:
> > 
> > That's fine by me. We could still add the others in the future if we feel the
> > need. Done that way in the attached.
> 
> I'm not sure that it's unproblematic to add multiple pgstat count calls to
> every lock acquisition, particularly if it's a fastpath acquisition or a
> virtualxid lock. Notably these are external function calls, not just
> increments of a counter in an inline function.

Yeah, we could create inline functions instead.

> I also don't really know what one would do with some of the information?
> 
> What does the number of virtualxid lock acquisitions tell you that the numbers
> of transactions doesn't already tell you in a more understandable way?

I agree not that much, except being able to compute a transactions/virtualxid
ratio or such. Also the idea was to report the same as pg_locks so that one
could start sampling pg_locks if he needs more details.

> What does it tell you that the deadlock checker ran N times? It notably
> doesn't count deadlocks, it counts how often we checked for deadlocks.
> 
> The percentage of fastpath locks also seems not really informative, because
> that could be because we ran out of space for fastpath locks, or because a
> lock mode that's ineligible for fastpath locks was used.

Right, maybe we could add an "exceed fastpath slots" or such counter?

> What I would actually count is the amount of time waiting for locks, that
> seems vastly more useful than the number of acquisitions.  We already do a
> GetCurrentTimestamp() inside the timer activations for deadlock timeout, we
> probably can figure out a way to reuse that to reduce the increase in overhead
> due to timing.  We could also just count the wait time after the deadlock
> check has run.

Yeah, providing the wait_time would be great. Just to be sure, are you suggesting
to remove all the fields (i.e requests, timeouts, deadlock_timeouts and fastpath)
and just add a wait_time field instead? I think that keeping requests would make
sense to be able to get the average wait time per request.

Regards,

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

  20. Re: Adding locks statistics

    Andres Freund <andres@anarazel.de> — 2026-02-16T21:00:56Z

    Hi,

On 2026-02-16 10:10:21 +0000, Bertrand Drouvot wrote:
> On Fri, Feb 13, 2026 at 04:13:39PM -0500, Andres Freund wrote:
> > On 2026-02-13 10:24:52 +0000, Bertrand Drouvot wrote:
> > > 
> > > That's fine by me. We could still add the others in the future if we feel the
> > > need. Done that way in the attached.
> > 
> > I'm not sure that it's unproblematic to add multiple pgstat count calls to
> > every lock acquisition, particularly if it's a fastpath acquisition or a
> > virtualxid lock. Notably these are external function calls, not just
> > increments of a counter in an inline function.
> 
> Yeah, we could create inline functions instead.

I think unless you have a heck of a lot more of a usecase than "it's
information", counting every lock acqusition seems just worth ti.


> > I also don't really know what one would do with some of the information?
> > 
> > What does the number of virtualxid lock acquisitions tell you that the numbers
> > of transactions doesn't already tell you in a more understandable way?
> 
> I agree not that much, except being able to compute a transactions/virtualxid
> ratio or such. Also the idea was to report the same as pg_locks so that one
> could start sampling pg_locks if he needs more details.

I don't think that's worth adding overhead to something extremely frequently
happening.


> > What does it tell you that the deadlock checker ran N times? It notably
> > doesn't count deadlocks, it counts how often we checked for deadlocks.
> > 
> > The percentage of fastpath locks also seems not really informative, because
> > that could be because we ran out of space for fastpath locks, or because a
> > lock mode that's ineligible for fastpath locks was used.
> 
> Right, maybe we could add an "exceed fastpath slots" or such counter?

That I could get behind.


> > What I would actually count is the amount of time waiting for locks, that
> > seems vastly more useful than the number of acquisitions.  We already do a
> > GetCurrentTimestamp() inside the timer activations for deadlock timeout, we
> > probably can figure out a way to reuse that to reduce the increase in overhead
> > due to timing.  We could also just count the wait time after the deadlock
> > check has run.
> 
> Yeah, providing the wait_time would be great. Just to be sure, are you suggesting
> to remove all the fields (i.e requests, timeouts, deadlock_timeouts and fastpath)
> and just add a wait_time field instead?

Well, I'd maybe make it waits, wait_time and perhaps fastpath exceeded.


> I think that keeping requests would make sense to be able to get the average
> wait time per request.

I don't think I'd request for that (as that would require counting in the
normal case), I'd use the number of waits.


I don't think you have provided any actual motivation for why it's worth the
expense to track the number of lock requests.  Just trackings stats that
nobody has a usecase for that are not free to collect makes no sense.

Greetings,

Andres Freund

  21. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-17T16:33:54Z

    Hi,

On Mon, Feb 16, 2026 at 04:00:56PM -0500, Andres Freund wrote:
> Hi,
> 
> On 2026-02-16 10:10:21 +0000, Bertrand Drouvot wrote:
> > Yeah, providing the wait_time would be great. Just to be sure, are you suggesting
> > to remove all the fields (i.e requests, timeouts, deadlock_timeouts and fastpath)
> > and just add a wait_time field instead?
> 
> Well, I'd maybe make it waits, wait_time and perhaps fastpath exceeded.

Okay, done that way in the attached. To avoid overhead due to timing as much as
possible, the patch simply relies on log_lock_waits and deadlock_timeout. It means
that it relies on the existing code, and increments waits and wait_time only if
log_lock_waits is on and if the session waited longer than deadlock_timeout.

I did not want to dissociate the waits and wait_time increments so that their
ratio could still make sense.

That sounds like a good compromise, thoughts?

> > I think that keeping requests would make sense to be able to get the average
> > wait time per request.
> 
> I don't think I'd request for that (as that would require counting in the
> normal case), I'd use the number of waits.

Yeah, I meant to say waits and not requests.

Regards,

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

  22. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-02-19T04:06:52Z

    On Tue, Feb 17, 2026 at 04:33:54PM +0000, Bertrand Drouvot wrote:
> Okay, done that way in the attached. To avoid overhead due to timing as much as
> possible, the patch simply relies on log_lock_waits and deadlock_timeout. It means
> that it relies on the existing code, and increments waits and wait_time only if
> log_lock_waits is on and if the session waited longer than deadlock_timeout.
> 
> I did not want to dissociate the waits and wait_time increments so that their
> ratio could still make sense.
> 
> That sounds like a good compromise, thoughts?

             else if (myWaitStatus == PROC_WAIT_STATUS_OK)
+            {
+                /* Increment the lock statistics counters */
+                pgstat_count_lock_waits(locallock->tag.lock.locktag_type);
+                pgstat_count_lock_wait_time(locallock->tag.lock.locktag_type, msecs);

Not sure that it makes much sense to me to rely on log_lock_waits
being enabled to decide if this count and this time are aggregated.
The log information and the stats gathering are two separate things.
Wouldn't it make more sense to call pgstat_count_lock_waits() outside
of this code path, when we know myWaitStatus?

While relying on the time calculating for the logs data is a good
idea, it seems to me that we should have a separate GUC to enable this
number, like a new track_lock_timings?  If track_lock_timings or
log_lock_waits is enabled, we should calculate the time difference.
All these decisions also depends on what deadlock_state holds on top
of myWaitStatus, I guess..
--
Michael

  23. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-19T12:03:32Z

    Hi,

On Thu, Feb 19, 2026 at 01:06:52PM +0900, Michael Paquier wrote:
> On Tue, Feb 17, 2026 at 04:33:54PM +0000, Bertrand Drouvot wrote:
> > Okay, done that way in the attached. To avoid overhead due to timing as much as
> > possible, the patch simply relies on log_lock_waits and deadlock_timeout. It means
> > that it relies on the existing code, and increments waits and wait_time only if
> > log_lock_waits is on and if the session waited longer than deadlock_timeout.
> > 
> > I did not want to dissociate the waits and wait_time increments so that their
> > ratio could still make sense.
> > 
> > That sounds like a good compromise, thoughts?
> 
>              else if (myWaitStatus == PROC_WAIT_STATUS_OK)
> +            {
> +                /* Increment the lock statistics counters */
> +                pgstat_count_lock_waits(locallock->tag.lock.locktag_type);
> +                pgstat_count_lock_wait_time(locallock->tag.lock.locktag_type, msecs);
> 
> Not sure that it makes much sense to me to rely on log_lock_waits
> being enabled to decide if this count and this time are aggregated.
> The log information and the stats gathering are two separate things.
> Wouldn't it make more sense to call pgstat_count_lock_waits() outside
> of this code path, when we know myWaitStatus?

> While relying on the time calculating for the logs data is a good
> idea, it seems to me that we should have a separate GUC to enable this
> number, like a new track_lock_timings?  If track_lock_timings or
> log_lock_waits is enabled, we should calculate the time difference.
> All these decisions also depends on what deadlock_state holds on top
> of myWaitStatus, I guess..

The idea was to avoid adding a new GUC and I did not want to increment the
waits independently of the wait time (so that wait time/waits could make
sense).

That said, your point of view also makes (more) sense, so in the attached:

- adds a new GUC (namely track_lock_timing)
- tracks the wait_time if the GUC is on and the session waited longer than
deadlock_timeout
- when wait_time is incremented, then a new timed_waits counter is also
incremented (so that wait_time / timed_waits makes sense)
- waits is incremented unconditionally

Note that due to the new GUC behavior (wait_time incremented only if we waited
longer than deadlock_timeout), then it is on by default (same idea as for
2aac62be8cb).

Regards,

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

  24. Re: Adding locks statistics

    Andres Freund <andres@anarazel.de> — 2026-02-19T18:27:10Z

    Hi,

On 2026-02-19 13:06:52 +0900, Michael Paquier wrote:
> On Tue, Feb 17, 2026 at 04:33:54PM +0000, Bertrand Drouvot wrote:
> > Okay, done that way in the attached. To avoid overhead due to timing as much as
> > possible, the patch simply relies on log_lock_waits and deadlock_timeout. It means
> > that it relies on the existing code, and increments waits and wait_time only if
> > log_lock_waits is on and if the session waited longer than deadlock_timeout.
> >
> > I did not want to dissociate the waits and wait_time increments so that their
> > ratio could still make sense.
> >
> > That sounds like a good compromise, thoughts?
>
>              else if (myWaitStatus == PROC_WAIT_STATUS_OK)
> +            {
> +                /* Increment the lock statistics counters */
> +                pgstat_count_lock_waits(locallock->tag.lock.locktag_type);
> +                pgstat_count_lock_wait_time(locallock->tag.lock.locktag_type, msecs);

Why do two external function calls?  The function calls are at least as
expensive as the work inside them, so doing this separately adds a fair bit of
overhead.


> Not sure that it makes much sense to me to rely on log_lock_waits
> being enabled to decide if this count and this time are aggregated.
> The log information and the stats gathering are two separate things.
> Wouldn't it make more sense to call pgstat_count_lock_waits() outside
> of this code path, when we know myWaitStatus?

IDK, it doesn't seem unreasonable to not duplicate work. If the delay is very
short it's probably also not that interesting to track, but I guess that's
debatable.


> While relying on the time calculating for the logs data is a good
> idea, it seems to me that we should have a separate GUC to enable this
> number, like a new track_lock_timings?  If track_lock_timings or
> log_lock_waits is enabled, we should calculate the time difference.
> All these decisions also depends on what deadlock_state holds on top
> of myWaitStatus, I guess..

I don't think it's worth having a separate GUC to track this. The realistic
number of calls in a certain timespan to this is way way lower than something
like track_io_timing, track_wal_io_timing or such. So I don't think we need an
opt-out here like for those.  If we eventually can reduce the overhead of the
other track_* GUCs, we should remove them too, but I think that's further out.

Greetings,

Andres Freund

  25. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-20T06:38:07Z

    Hi,

On Thu, Feb 19, 2026 at 01:27:10PM -0500, Andres Freund wrote:
> Hi,
> 
> On 2026-02-19 13:06:52 +0900, Michael Paquier wrote:
> > On Tue, Feb 17, 2026 at 04:33:54PM +0000, Bertrand Drouvot wrote:
> > > Okay, done that way in the attached. To avoid overhead due to timing as much as
> > > possible, the patch simply relies on log_lock_waits and deadlock_timeout. It means
> > > that it relies on the existing code, and increments waits and wait_time only if
> > > log_lock_waits is on and if the session waited longer than deadlock_timeout.
> > >
> > > I did not want to dissociate the waits and wait_time increments so that their
> > > ratio could still make sense.
> > >
> > > That sounds like a good compromise, thoughts?
> >
> >              else if (myWaitStatus == PROC_WAIT_STATUS_OK)
> > +            {
> > +                /* Increment the lock statistics counters */
> > +                pgstat_count_lock_waits(locallock->tag.lock.locktag_type);
> > +                pgstat_count_lock_wait_time(locallock->tag.lock.locktag_type, msecs);
> 
> Why do two external function calls?  The function calls are at least as
> expensive as the work inside them, so doing this separately adds a fair bit of
> overhead.

Yeah, I also realized that and that was changed in v6 (you were looking at v5
to make this comment).

> > Not sure that it makes much sense to me to rely on log_lock_waits
> > being enabled to decide if this count and this time are aggregated.
> > The log information and the stats gathering are two separate things.
> > Wouldn't it make more sense to call pgstat_count_lock_waits() outside
> > of this code path, when we know myWaitStatus?
> 
> IDK, it doesn't seem unreasonable to not duplicate work.

I don't think that the new GUC is introducing duplicate work (see 0003 attached),
that said it's introducing extra complexity in ProcSleep().

> If the delay is very
> short it's probably also not that interesting to track, but I guess that's
> debatable.

v6 was introducing timed_waits so that we have:

waits
timed_waits
wait_time
fastpath_exceeded

timed_waits and wait_time were incremented together and waits was incremented
unconditionally. I like the idea of being able to track the numbers of waits
whatever the value of log_lock_waits (or the new track_lock_timing) is. Also
one could compare waits vs timed_waits.

It's still done that way in the attached.

> I don't think it's worth having a separate GUC to track this. The realistic
> number of calls in a certain timespan to this is way way lower than something
> like track_io_timing, track_wal_io_timing or such. So I don't think we need an
> opt-out here like for those.  If we eventually can reduce the overhead of the
> other track_* GUCs, we should remove them too, but I think that's further out.

Yeah, OTOH having a dedicated GUC for a clear separation of duties also makes
sense.

I don't have a strong opinion on it, but in the attached 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.

Regards,

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

  26. Re: Adding locks statistics

    Andres Freund <andres@anarazel.de> — 2026-02-20T16:02:49Z

    Hi,

On 2026-02-20 06:38:07 +0000, Bertrand Drouvot wrote:
> > If the delay is very
> > short it's probably also not that interesting to track, but I guess that's
> > debatable.
> 
> v6 was introducing timed_waits so that we have:
> 
> waits
> timed_waits
> wait_time
> fastpath_exceeded
> 
> timed_waits and wait_time were incremented together and waits was incremented
> unconditionally. I like the idea of being able to track the numbers of waits
> whatever the value of log_lock_waits (or the new track_lock_timing) is. Also
> one could compare waits vs timed_waits.

How could a user benefit from that split? To me this is pointless number
gathering that wastes resources and confuses users.

Seriously, youre introducing stats left and right, you really need to stop and
first carefully think about what those stats could possibly be useful
for. Before writing a patch implementing the stats.

Greetings,

Andres Freund

  27. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-20T17:26:37Z

    Hi,

On Fri, Feb 20, 2026 at 11:02:49AM -0500, Andres Freund wrote:
> Hi,
> 
> On 2026-02-20 06:38:07 +0000, Bertrand Drouvot wrote:
> > > If the delay is very
> > > short it's probably also not that interesting to track, but I guess that's
> > > debatable.
> > 
> > v6 was introducing timed_waits so that we have:
> > 
> > waits
> > timed_waits
> > wait_time
> > fastpath_exceeded
> > 
> > timed_waits and wait_time were incremented together and waits was incremented
> > unconditionally. I like the idea of being able to track the numbers of waits
> > whatever the value of log_lock_waits (or the new track_lock_timing) is. Also
> > one could compare waits vs timed_waits.
> 
> 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).

Regards,

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

  28. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-27T05:14:31Z

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

Regards,

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

  29. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-03-18T04:36:04Z

    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

  30. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-03-18T08:15:27Z

    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

  31. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-03-18T14:51:01Z

    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

  32. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-03-19T07:01:39Z

    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

  33. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-03-19T12:25:41Z

    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

  34. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-03-21T05:55:33Z

    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

  35. Re: Adding locks statistics

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-03-21T20:01:02Z

    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

  36. Re: Adding locks statistics

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-03-22T19:37:47Z

    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

The 15th one removes the inclusion of clog.h in proc.h.  Not really
related to this thread, but I noticed while looking at proc.h for
removal of lock.h.


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

I looked at it again while doing the above and came to the same
conclusion.  In order to make it work we'd need half a dozen typedefs,
and it doesn't really look very pretty.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

  37. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-03-23T08:18:05Z

    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

  38. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-03-24T03:15:05Z

    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

  39. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-03-24T07:02:55Z

    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

  40. Re: Adding locks statistics

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-03-24T16:16:11Z

    Hello,

On 2026-Mar-23, Bertrand Drouvot wrote:

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

Hmm, right -- I don't need to add both.  Done that way.  I also made
some alphabetical sorts and pushed this part.

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

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo lógico y coherente. Pero el universo real se halla siempre
un paso más allá de la lógica" (Irulan)

  41. Re: Adding locks statistics

    Andres Freund <andres@anarazel.de> — 2026-03-24T20:09:37Z

    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

  42. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-03-24T23:46:23Z

    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

  43. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-03-25T03:15:19Z

    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

  44. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-03-25T03:32:05Z

    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

  45. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-03-25T05:25:01Z

    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

  46. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-03-25T07:24:15Z

    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

  47. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-03-25T19:54:42Z

    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

  48. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-03-25T23:06:20Z

    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

  49. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-03-26T04:05:40Z

    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

  50. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-03-26T06:53:37Z

    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

  51. Re: Adding locks statistics

    Tomas Vondra <tomas@vondra.me> — 2026-03-30T16:11:17Z

    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.

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

If per-entry locks help, maybe a good micro-optimization would be to
check if there even is anything to sync before acquiring the lock. I
mean, if (waits==0), why obtain the lock?


regards

-- 
Tomas Vondra

  52. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-03-31T07:10:51Z

    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

  53. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-04-06T00:05:15Z

    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

  54. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-04-06T05:11:21Z

    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

  55. Re: Adding locks statistics

    Andres Freund <andres@anarazel.de> — 2026-04-06T05:22:23Z

    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_stats/log/regress_log_011_lock_stats

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

Greetings,

Andres Freund

  56. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-04-06T06:19:57Z

    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_stats/log/regress_log_011_lock_stats
> 
> [04:33:31.756](0.067s) # die: error running SQL: 'psql:<stdin>:1: ERROR:  could not detach injection point "deadlock-timeout-fired"'

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

  57. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-04-06T06:34:44Z

    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

  58. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-04-07T04:21:39Z

    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

  59. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-04-07T06:01:08Z

    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

  60. Re: Adding locks statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-04-07T06:02:35Z

    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

  61. Re: Adding locks statistics

    Michael Paquier <michael@paquier.xyz> — 2026-04-07T23:30:36Z

    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