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
