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