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

Hi,

On Thu, Jan 09, 2025 at 02:20:16PM +0300, Nazir Bilal Yavuz wrote:
> Hi,
>
> On Thu, 9 Jan 2025 at 11:11, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >
> > On Thu, Jan 09, 2025 at 10:15:20AM +0300, Nazir Bilal Yavuz wrote:
> > > I am a bit confused, are you suggesting these two alternatives:
> > > 1- Making pgstat_count_io_op_n() static and continuing to use
> > > pgstat_count_io_op() as it is.
> > > 2- Removing pgstat_count_io_op() and instead using
> > > pgstat_count_io_op_n() everywhere.
> >
> > Either of these options is OK by me. The current state of things just
> > seems a bit strange because we publish a routine that's used nowhere.
> > If you have plans for it in a different patch, that's also fine.
>
> I followed the second option as it is similar to
> pgstat_count_io_op_time() and also more future proof. I attached it as
> another patch. v7 is attached.

Thanks for the patches!

v7-0001:

+pg_attribute_unused()
+static inline bool
+is_ioop_tracked_in_bytes(IOOp io_op)
+{
+ Assert((unsigned int) io_op < IOOP_NUM_TYPES);
+ return io_op >= IOOP_EXTEND;
+}

and then

+ Assert(is_ioop_tracked_in_bytes(io_op) || bytes == 0);

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)

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.

Regards,

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