Date: Tue, 17 Dec 2024 08:13:59 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Nazir Bilal Yavuz <byavuz81@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: per backend I/O statistics
Message-ID: <Z2EyxyEJPMMldj1t@ip-10-97-1-34.eu-west-3.compute.internal>
References: <Z0QjeIkwC0HNI16K@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z0Qk3nZcaVpbtCKU@paquier.xyz>
 <Z0ScL5W6EeWXShb3@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z1pr86DCR_P6iTSV@paquier.xyz>
 <Z1rs/j5JMdTbUIJJ@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z1uVzXFE9WhSicOx@paquier.xyz>
 <Z1v8TQbeTyHH5kxU@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z1_f2FzvUoL9KZsQ@paquier.xyz>
 <Z2BKTK9KX5Kc3HaU@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z2EZegYbH22LQHhN@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Z2EZegYbH22LQHhN@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/Z2EyxyEJPMMldj1t%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Tue, Dec 17, 2024 at 03:26:02PM +0900, Michael Paquier wrote:
> On Mon, Dec 16, 2024 at 03:42:04PM +0000, Bertrand Drouvot wrote:
> >> Perhaps there's an argument for an entirely separate
> >> callback that would run before pgstat is plugged off, like a new
> >> before_shmem_exit() callback registered after the entry is created?
> > 
> > As the ProcKill() is run in shmem_exit() (and so after before_shmem_exit()),
> > I think that the way we currently drop the backend stats entry is fine (unless
> > I miss something about your concern).
> 
> Looking closely at this one, I think that you're right as long as the
> shutdown callback is registered before the entry is created.

Yeah, which is the case.

> Anyway, I have put my hands on v9.  A couple of notes, while hacking
> through it.  See v9-0002 for the parts I have modified, that applies
> on top of your v9-0001.

Thanks!

> You may have noticed fee2b3ea2ecdg, which led me to fix two comments
> as these paths are not related to only database objects.

Yeap ;-)

> Added more documentation, tweaked quite a bit the comments, a bit less
> the docs (no need for the two linkends) and applied an indentation.

Looks good to me.

> pg_stat_get_backend_io() can be simpler, and does not need the loop
> with the extra LocalPgBackendStatus.  It is possible to call once
> pgstat_get_beentry_by_proc_number() and retrieve the PID and the
> bktype for the two sanity checks we want to do on them.

Looked at the changes you've done and agree that's simpler.

> s/pgstat_fetch_proc_stat_io/pgstat_fetch_stat_backend/.

Not sure about this one as being called in pg_stat_get_backend_io().

> s/pgstat_create_backend_stat/pgstat_create_backend/.

LGTM.

> I've been wondering for quite a bit about PgStat_BackendPendingIO and
> PgStat_PendingIO, and concluded to define both in pgstat.h, with the
> former being defined based on the latter to keep the dependency
> between both at the same place.

That's fine by me.

Also:

=== 1

the changes in pgstat_flush_backend() make sense to me.

=== 2

- * Create the backend statistics entry for procnum.
+ * Create backend statistics entry for proc number.

Yeah, "proc number" looks better as already used in multiple places.

=== 3

the changes in stats.sql look good to me.

Having said that, v9-0002 looks good to me (except the pgstat_fetch_proc_stat_io
renaming).

Regards,

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