Re: per backend I/O statistics

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