Re: Adding locks statistics - Mailing list pgsql-hackers

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