Re: Make pg_stat_io view count IOs as bytes instead of blocks

On Thu, Jan 09, 2025 at 03:30:37PM +0000, Bertrand Drouvot wrote:
> We first use an Assert in is_ioop_tracked_in_bytes() and then we return
> a value "just" to check another Assert. I wonder if it wouldn't make more sense
> to get rid of this function and use a macro instead, something like?
>
> #define is_ioop_tracked_in_bytes(io_op) \
> ((io_op) < IOOP_NUM_TYPES && (io_op) >= IOOP_EXTEND)

Indeed. Your suggestion to use a macro makes more sense to me because
is_ioop_tracked_in_bytes() itself has an Assert(), while being itself
only used in an assertion, as you say. Better to document the
dependency on the ordering of IOOp, even if that's kind of hard to
miss.

> v7-0002:
>
> I wonder if it wouldn't make more sense to remove pgstat_count_io_op() first
> and then implement what currently is in v7-0001. What v7-0002 is removing is
> not produced by v7-0001.

This kind of cleanup should happen first, and it simplifies a bit the
reading of v7-0001 as we would just append a new argument to the
count_io_op() routine for the number of bytes. I was looking at that
and chose to stick with count_io_op() rather than count_io_op_n() as
we would have only one routine.

pgstat_count_io_op_time(IOOBJECT_RELATION, io_context, IOOP_EXTEND,
- io_start, extend_by);
+ io_start, 1, extend_by * BLCKSZ);
[...]
pgstat_count_io_op_time(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_EXTEND,
- io_start, extend_by);
+ io_start, 1, extend_by * BLCKSZ);

Something worth mentioning. I had a small doubt about this one for
temp and persistent relations, as it changes the extend count.
Anyway, I think that this is better: we are only doing one extend
batch, worth (extend_by * BLCKSZ).

Two times my fault today that the main patch does not apply anymore
(three times at least in total), so I have rebased it myself, and did
an extra review while on it, switching the code to use a macro. That
seems OK here. Please let me know if you have more comments.

(Still need to spend more time on the commit message, will do that
later).
--
Michael