Re: per backend I/O statistics

On Mon, Dec 16, 2024 at 03:42:04PM +0000, Bertrand Drouvot wrote:
> It's not necessary per say, but it ensures that pg_stat_get_backend_io() does not
> return rows full of "invalid" combinations.

Okay. I am not planning to fight more over this point.

> I'm not sure that would be possible for a PGSTAT_KIND_BACKEND kind to return
> false here but I agree that's better to call pgstat_request_entry_refs_gc()
> if that's the case. Done in v9 attached.

It seems to me that it could be possible if a different backend keeps
a reference to this entry.

>> 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. So I'm
OK because the drop is a no-op if the entry cannot be found, and we
would not try a pgstat_request_entry_refs_gc().

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.

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

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

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.

s/pgstat_fetch_proc_stat_io/pgstat_fetch_stat_backend/.
s/pgstat_create_backend_stat/pgstat_create_backend/.

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.
--
Michael