Re: per backend I/O statistics

On Thu, Nov 14, 2024 at 01:30:11PM +0000, Bertrand Drouvot wrote:
> - change the arguments to false in the pgstat_drop_entry_internal() call in
> pgstat_drop_all_entries()
> - start the engine
> - kill -9 postgres
> - restart the engine
>
> You'll see the assert failing due to the startup process. I don't think it is
> doing something wrong though, just populating its backend stats. Do you have
> another view on it?

It feels sad that we have to plug in the internals at this level for
this particular case. Perhaps there is something to do with more
callbacks. Or perhaps there is just no point in tracking the stats of
auxiliary processes because we already have this data in the existing
pg_stat_io already?

> How would that work for someone doing "select * from pg_stat_get_backend_io(<pid>)"?
>
> Do you mean copy/paste part of the code from pg_stat_get_activity() into
> pg_stat_get_backend_io() to get the backend type? That sounds like an idea,
> I'll have a look at it.

I was under the impression that we should keep PgStat_Backend for the
reset_timestamp part, but drop BackendType as it can be guessed from
pg_stat_activity itself. For example a LATERAL to grab the current
live stats of these backends.

>> +typedef struct PgStat_PendingIO
>>
>> Perhaps this part should use a separate structure named
>> "BackendPendingIO"? The definition of the structure has to be in
>> pgstat.h as this is the pending_size of the new stats kind. It looks
>> like it would be cleaner to keep PgStat_PendingIO local to
>> pgstat_io.c, and define PgStat_PendingIO based on
>> PgStat_BackendPendingIO?
>
> I see what you meean, what about simply "PgStat_BackendPending" in pgstat.h?

That should be OK.

>> Structurally, it may be cleaner to keep all the callbacks and the
>> backend-I/O specific logic into a separate file, perhaps
>> pgstat_io_backend.c or pgstat_backend_io?
>
> Yeah, I was wondering the same. What about pgstat_backend.c (that would contain
> only one "section" dedicated to IO currently)?

pgstat_backend.c looks good to me. Could there be other stats than
just IO, actually? Perhaps not, just asking..

> - if the idea is to 1. record checkpointer stats, 2. do stuff in the backend and
> 3. check again the checkpointer stats then I don't think setting stats_fetch_consistency
> to snapshot is needed at all. In fact it's quite the opposite as 1. and 3. would
> report the same values with stats_fetch_consistency set to 'snapshot' (if done
> in the same transaction).

Hmm. Not sure. It looks like you're right to treat this in a
separate patch as it would exist for different reasons than the
original proposal.
--
Michael