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

Hi,

On Thu, 5 Dec 2024 at 12:13, Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Wed, Dec 04, 2024 at 02:49:11PM +0300, Nazir Bilal Yavuz wrote:
> > On Thu, 28 Nov 2024 at 16:39, Bertrand Drouvot
> > <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> > >
> > You are right, no need to have this check; it can not be less than 0.
> > I completely removed the function now.
>
> Thanks! Yeah I think that makes sense.
>
> > > What about ordering the enum in IOOp (no bytes/bytes) so that we could check
> > > that io_op >= "our firt bytes enum" instead?
> > >
> > > Also we could create a macro on top of that to make it clear. And a comment
> > > would be needed around the IOOp definition.
> > >
> > > I think that would be simpler to maintain should we add no bytes or bytes op in
> > > the future.
> >
> > I think this is a good idea. I applied all the comments.
>
> Thanks!
>
> + * non-byte-measured and byte-measured operations. So, that makes is easier
> + * to check whether IO is measured in bytes.
>
> s/that makes is/that makes it/

Done.

> + *
> + * Note: If this enum is modified, ensure that both `IOOP_NUM_TYPES` macro
> + * and the `ioop_measured_in_bytes` function are updated accordingly.
>
> Yeah, or?
>
> "Ensure IOOP_EXTEND is the first and IOOP_WRITE the last ones in the measured in
> bytes" group and that the groups stay in that order.
>
> That would make the "checks" local to the enum def should someone modify it.

Makes sense, done.

> > Created an
> > inline function instead of macro and added this 'Assert((unsigned int)
> > io_object < IOOBJECT_NUM_TYPES);' to function.
>
> An inline function seems the right choice for me too.
>
> > Done. I moved these checks to the pgstat_count_io_op_n() function. The
> > code looks more like its previous version now.
>
> Thanks! Yeah, more easier to follow now.
>
> > > Not sure about "can do IO in bytes" (same wording is used in multiple places).
> >
> > I changed it to 'measured in bytes' but I am not sure if this is
> > better, open to suggestions.
>
> I'm tempted to say something around "track", would mean things like:
>
> 1.
>
> ioop_measured_in_bytes => is_ioop_tracked_in_bytes
>
> 2.
>
> s/If an op does not do IO in bytes/If an op does not track bytes/
>
> 3.
>
> s/not every IO measured in bytes/not every IO tracks bytes/
>
> 4.
>
> s/non-byte-measured and byte-measured/non-tracking and tracking bytes/
>
> Thoughts?

Thanks! I think 'track' is a better word in this context. I used
'tracked in ...', as it sounded more correct to me (I hope it is).

--
Regards,
Nazir Bilal Yavuz
Microsoft