Re: Track IO times in pg_stat_io

Hi,

On 2/26/23 5:03 PM, Melanie Plageman wrote:
> Hi,
>
> As suggested in [1], the attached patch adds IO times to pg_stat_io;

Thanks for the patch!

I started to have a look at it and figured out that a tiny rebase was needed (due to
728560db7d and b9f0e54bc9), so please find the rebase (aka V2) attached.

> The timings will only be non-zero when track_io_timing is on

That could lead to incorrect interpretation if one wants to divide the timing per operations, say:

- track_io_timing is set to on while there is already operations
- or set to off while it was on (and the number of operations keeps growing)

Might be worth to warn/highlight in the "track_io_timing" doc?

+ if (track_io_timing)
+ {
+ INSTR_TIME_SET_CURRENT(io_time);
+ INSTR_TIME_SUBTRACT(io_time, io_start);
+ pgstat_count_io_time(io_object, io_context, IOOP_EXTEND, io_time);
+ }
+
+
pgstat_count_io_op(io_object, io_context, IOOP_EXTEND);

vs

@@ -1042,6 +1059,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
INSTR_TIME_SUBTRACT(io_time, io_start);
pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time));
INSTR_TIME_ADD(pgBufferUsage.blk_read_time, io_time);
+ pgstat_count_io_time(io_object, io_context, IOOP_READ, io_time);
}

That leads to pgstat_count_io_time() to be called before pgstat_count_io_op() (for the IOOP_EXTEND case) and
after pgstat_count_io_op() (for the IOOP_READ case).

What about calling them in the same order and so that pgstat_count_io_time() is called before pgstat_count_io_op()?

If so, the ordering would also need to be changed in:

- FlushRelationBuffers()
- register_dirty_segment()

>
> There is one minor question (in the code as a TODO) which is whether or
> not it is worth cross-checking that IO counts and times are either both
> zero or neither zero in the validation function
> pgstat_bktype_io_stats_valid().
>

As pgstat_bktype_io_stats_valid() is called only in Assert(), I think that would be a good idea
to also check that if counts are not Zero then times are not Zero.

Regards,

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