Re: Adding locks statistics

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