Date: Fri, 5 Jun 2026 08:29:12 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Tristan Partin <tristan@partin.io>
Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Add per-backend lock statistics
Message-ID: <aiKI2FSmZwyItCIz@bdtpg>
References: <aiAzEY+cMQb/W8yu@bdtpg>
 <DJ0L4JF9W2J7.2B0DAKKQYA4DF@partin.io>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <DJ0L4JF9W2J7.2B0DAKKQYA4DF@partin.io>
Archived-At: <https://www.postgresql.org/message-id/aiKI2FSmZwyItCIz%40bdtpg>
Precedence: bulk

Hi,

On Thu, Jun 04, 2026 at 09:32:39PM +0000, Tristan Partin wrote:
> On Wed Jun 3, 2026 at 1:59 PM UTC, Bertrand Drouvot wrote:
> 
> The motivation makes sense to me.

Thanks for looking at it and sharing your thoughts!

> > 0001: Refactor pg_stat_get_lock() to use a helper function
> 
> > +static void
> > +pg_stat_lock_build_tuples(ReturnSetInfo *rsinfo,
> > +                         PgStat_LockEntry *lock_stats,
> > +                         TimestampTz stat_reset_timestamp)
> 
> I think that the alignment of the second and third arguments could be 
> off by one. They should line up with the capital R in ReturnSetInfo.

They look ok to me in the C file, what about you?

> > -       values[i] = TimestampTzGetDatum(lock_stats->stat_reset_timestamp);
> > +       if (stat_reset_timestamp != 0)
> > +           values[i] = TimestampTzGetDatum(stat_reset_timestamp);
> > +       else
> > +           nulls[i] = true;
> 
> It's not super clear to me why this changed in the first patch.

It's to make less "noise" in the second patch and keep the second patch focusing
only on the "new feature". It's to ease to review but could be merged before
being pushed would the commiter decides to do so.

> > 0002: Add per-backend lock statistics
> 
> > +        Returns lock statistics about the backend with the specified
> > +        process ID. The output fields are exactly the same as the ones in the
> > +        <structname>pg_stat_lock</structname> view.
> 
> It probably makes sense to link to pg_stat_lock here.

Not sure as that would not be consistent with pg_stat_get_backend_io and
pg_stat_get_backend_wal descriptions in monitoring.sgml.

> Other than the few comments I had, this patchset looks good. It follows 
> patterns that were already established with the per-backend IO and WAL 
> stats.

Thanks!

Regards,

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