Re: per backend I/O statistics

On Tue, Sep 17, 2024 at 01:56:34PM +0000, Bertrand Drouvot wrote:
> No problem at all! (I re-explained because I'm not always 100% sure that my
> explanations are crystal clear ;-) )

We've discussed a bit this patch offline, but after studying the patch
I doubt that this part is a good idea:

+ /* has to be at the end due to FLEXIBLE_ARRAY_MEMBER */
+ PgStatShared_IO io;
} PgStat_ShmemControl;

We are going to be in trouble if we introduce a second member in this
routine that has a FLEXIBLE_ARRAY_MEMBER, because PgStat_ShmemControl
relies on the fact that all its members after deterministic offset
positions in this structure. So this lacks flexibility. This choice
is caused by the fact that we don't exactly know the number of
backends because that's controlled by the PGC_POSTMASTER GUC
max_connections so the size of the structure would be undefined.

There is a parallel with replication slot statistics here, where we
save the replication slot data in the dshash based on their index
number in shmem. Hence, wouldn't it be better to do like replication
slot stats, where we use the dshash and a key based on the procnum of
each backend or auxiliary process (ProcNumber in procnumber.h)? If at
restart max_connections is lower than what was previously used, we
could discard entries that would not fit anymore into the charts.
This is probably not something that happens often, so I'm not really
worried about forcing the removal of these stats depending on how the
upper-bound of ProcNumber evolves.

So, using a new kind of ID and making this kind variable-numbered may
ease the implementation quite a bit, while avoiding any extensibility
issues with the shmem portion of the patch if these are
fixed-numbered. The reporting of these stats comes down to having a
parallel with pgstat_count_io_op_time(), but to make sure that the
stats are split by connection slot number rather than the current
split of pg_stat_io. All its callers are in localbuf.c, bufmgr.c and
md.c, living with some duplication in the code paths to gather the
stats may be OK.

pg_stat_get_my_io() is based on a O(n^3). IOOBJECT_NUM_TYPES is
fortunately low, still that's annoying.

This would rely on the fact that we would use the ProcNumber for the
dshash key, and this information is not provided in pg_stat_activity.
Perhaps we should add this information in pg_stat_activity so as it
would be easily possible to do joins with a SQL function that returns
a SRF with all the stats associated with a given connection slot
(auxiliary or backend process)? That would be a separate patch.
Perhaps that's even something that has popped up for the work with
threading (did not follow this part closely, TBH)?

The active PIDs of the live sessions are not stored in the active
stats, why not? Perhaps that's useless anyway if we expose the
ProcNumbers in pg_stat_activity and make the stats available with a
single function taking in input a ProcNumber. Just mentioning an
option to consider.
--
Michael