Re: per backend I/O statistics

Hi,

On Thu, Nov 07, 2024 at 04:32:44PM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Thu, Nov 07, 2024 at 09:50:59AM +0900, Michael Paquier wrote:
> > On Wed, Nov 06, 2024 at 01:51:02PM +0000, Bertrand Drouvot wrote:
> > > That's not needed, the patch I'm working on stores the proc number in the
> > > objid field of the key.
> > 
> > Relying on the procnumber for the object ID gets a +1 here.
> 
> Thanks!
> 
> > That
> > provides an automatic cap on the maximum number of entries that can
> > exist at once for this new stats kind.
> 
> Yeah.
> 
> POC patch is now working, will wear a reviewer hat now and will share in one 
> or 2 days.
> 

Please find attached v5 that implements the per-backend IO stats as 
variable-numbered stats kind (as per the up-thread discussion).

It is split into 4 sub-patches:

==== 0001 (the largest one)

Introduces a new statistics KIND. It is named PGSTAT_KIND_PER_BACKEND as it could
be used in the future to store other statistics (than the I/O ones) per backend.
The new KIND is a variable-numbered one and has an automatic cap on the
maximum number of entries (as its hash key contains the proc number).

There is no need to serialize the per backend I/O stats to disk (no point to
see stats for backends that do not exist anymore after a re-start), so a
new "to_serialize" field is added in the PgStat_KindInfo struct.

It adds a new pg_my_stat_io view to display "my" backend I/O statistics.

It also adds a new function pg_stat_reset_single_backend_io_counters() to be
able to reset the I/O stats for a given backend pid.

It contains doc updates and dedicated tests.

A few remarks:

1. one assertion in pgstat_drop_entry_internal() is not necessary true anymore 
with this new stat kind. So, adding an extra bool as parameter to take care of it.

2. a new struct "PgStat_Backend" is created and does contain the backend type.
The backend type is used for filtering purpose when the stats are displayed.

3. when the stats are reset, we need to keep the backend type so the change
in shared_stat_reset_contents().

4. the pending stats are updated in the existing pgstat_count_io_op_n() and 
pgstat_count_io_op_time() functions.

5. this new kind has its own flush callback: pgstat_per_backend_flush_cb().
But there is no need to maintain 2 callbacks for the I/O stats, so 0002 will
merge both and remove the one from the fixed stats. The reason it's not done
in 0001 is to ease the review.

==== 0002

Merge both IO stats flush callback. There is no need to keep both callbacks.

Merging both allows to save O(N^3) while looping on IOOBJECT_NUM_TYPES,
IOCONTEXT_NUM_TYPES and IOCONTEXT_NUM_TYPES.

The patch removes:

1. pgstat_io_flush_cb()
2. PendingIOStats (as the same information is already stored in the pending
entries)
3. have_iostats (the patch relies on the pending entries instead)

==== 0003

Don't include other backend's stats in the snapshot.

When stats_fetch_consistency is set to 'snapshot', don't include other backend's
stats in the snapshot. There is no use case, so save memory usage.

==== 0004

Adding the pg_stat_get_backend_io() function to retrieve I/O statistics for
a particular backend pid.

Note that this function does not return any rows if stats_fetch_consistency
is set to 'snapshot' and the pid of interest is not our own pid (there is no use
case of retrieving other backend's stats with stats_fetch_consistency set to
'snapshot').

The patch:

1. replaces pg_stat_get_my_io() with pg_stat_get_backend_io()
2. adds pgstat_fetch_proc_stat_io()
3. adds doc
4. adds tests

=== Remarks

R1. The key field is still "objid" even if it stores the proc number. I think
that's fine and there is no need to rename it as it's related comment states:

"
 /* object ID (table, function, etc.), or identifier. */
"

R2. pg_stat_get_io() and pg_stat_get_backend_io() could probably be merged (
duplicated code). That's not mandatory to provide the new per backend I/O stats
feature. So let's keep it as future work to ease the review.

R3. There is no use case of retrieving other backend's IO stats with
stats_fetch_consistency set to 'snapshot'. Avoiding this behavior allows us to
save memory (that could be non negligeable as pgstat_build_snapshot() would add
_all_ the other backend's stats in the snapshot). I choose to document it and to
not return any rows: I think that's better than returning some data (that would
not "ensure" the snapshot consistency) or an error.

Looking forward to your feedback,

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com