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
signature.asc
Description: PGP signature
