Re: per backend I/O statistics

Hi,

On Wed, Dec 18, 2024 at 01:57:53PM +0900, Michael Paquier wrote:
> On Tue, Dec 17, 2024 at 09:35:37AM +0000, Bertrand Drouvot wrote:
> > Agree, we may need to add parameters to it but we'll see when the time comes.
> 
> Another thing that's been itching me: the loop in
> pg_stat_get_backend_io() that exists as well in pg_stat_get_io() looks
> worth refactoring in a single routine.  The only difference between
> both is the reset timestamp, still both of them can pass a value for
> it depending on their stats kind entry.

Yeah, I also had something like this in mind (see R2 in [1]). So, +1 for it.

> This shaves a bit more code
> in your own patch, even if the check on pgstat_tracks_io_bktype() and
> the Assert are not in the inner routine that fills the tuplestore with
> the I/O data.

Yeah, that's fine by me.

> See pg_stat_fill_io_data() in v10-0002.  If you have a
> better name for this routine, feel free..

I think I prefer pg_stat_io_build_tuples() and used that name in v11
attached.

> What do you think about this refactoring?

Makes fully sense to me (as per my first comment above).

> This should come in first,
> of course, so as the final patch introducing the backend stats is
> easier to parse.

Yeah, it's in 0001 attached, with a few changes:

=== 1

s/Save tuples with data from this PgStat_BktypeIO./save tuples with data from this PgStat_BktypeIO/

to be consistent with single line comments around.

=== 2

+  if (stat_reset_timestamp != 0)
+    values[IO_COL_RESET_TIME] = TimestampTzGetDatum(stat_reset_timestamp);
+  else
+    nulls[IO_COL_RESET_TIME] = true;

This is not necessary until the per backend I/O stat is introduced but I 
added it in 0001 to ease 0002 parsing.

=== 3

"In Assert builds, we can afford an extra loop through all of the"

I think that this comment is now confusing since the extra loop would be
done in pg_stat_io_build_tuples() and not where the comment is written. One
option could have been to move the assert and the comment in pg_stat_io_build_tuples()
but I think it's better to have the assert before the pgstat_tracks_io_bktype()
call in pg_stat_get_io(), so modified the comment a bit instead.

0002 is the per-backend stats I/O with yours tweaks.

Regards,

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