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

Hi,

Thanks for the review!

On Thu, 9 Jan 2025 at 05:59, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Dec 26, 2024 at 02:41:26PM +0300, Nazir Bilal Yavuz wrote:
> > Thanks! v4 is attached. I quickly tested the pg_stat_get_backend_io()
> > function and it seems it is working.
>
> Thanks a lot for the rebased version.  This looks pretty solid.  Here
> are some comments.
>
>  void
> -pgstat_count_io_op(IOObject io_object, IOContext io_context, IOOp io_op)
> +pgstat_count_io_op(IOObject io_object, IOContext io_context, IOOp io_op, uint64 bytes)
>  {
> -    pgstat_count_io_op_n(io_object, io_context, io_op, 1);
> +    pgstat_count_io_op_n(io_object, io_context, io_op, 1, bytes);
>
> pgstat_count_io_op_n() is only used locally in pgstat_io.c.  I'm OK to
> keep it as it is used with the time calculations, but wouldn't it be
> better to make it static in pgstat_io.c instead and not declare it in
> pgstat.h?  Do we really have a need for pgstat_count_io_op() at all at
> the end or would it be better to change it so as it can handle a
> number of operations given by the caller?

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.

>
>  typedef struct PgStat_BktypeIO
>  {
> +    uint64        bytes[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES];
>
> This wastes a bit of memory, while keeping the code simpler to
> understand.  That's better than more element number manipulations, so
> I'm OK with what you have here.
>
> +static inline bool
> +is_ioop_tracked_in_bytes(IOOp io_op)
> +{
> +    Assert((unsigned int) io_op < IOOP_NUM_TYPES);
> +    return io_op >= IOOP_EXTEND;
> +}
>
> This is only used in an assertion of pgstat_count_io_op_n() in
> pgstat_io.c.  Let's also keep it this routine local to the file.  The
> assert to make sure that the callers don't assign bytes to the
> operations that don't support the counters is a good idea.

Makes sense, done.

>
> + * If an IOOp does not tracked in bytes, IO_COL_INVALID is returned.
>
> s/does not tracked/is not tracked/.

Done.

>
> +/*
> + * Get the number of the column containing IO bytes for the specified IOOp.
> + * If an IOOp does not tracked in bytes, IO_COL_INVALID is returned.
> + */
> +static io_stat_col
> +pgstat_get_io_byte_index(IOOp io_op)
> +{
>
> Makes sense to me, and that's consistent with how the time attributes
> are handled.
>
> -            /*
> -             * Hard-code this to the value of BLCKSZ for now. Future values
> -             * could include XLOG_BLCKSZ, once WAL IO is tracked, and constant
> -             * multipliers, once non-block-oriented IO (e.g. temporary file
> -             * IO) is tracked.
> -             */
> -            values[IO_COL_CONVERSION] = Int64GetDatum(BLCKSZ);
>
> Glad to see that gone.
>
> +        <structfield>write_bytes</structfield> <type>bigint</type>
> +        <structfield>extend_bytes</structfield> <type>bigint</type>
> +        <structfield>read_bytes</structfield> <type>bigint</type>
>
> These additions in the documentation are incorrect.  All these
> attributes are of type numeric, not bigint.

Done.

>
> +        Number of units of size <symbol>BLCKSZ</symbol> which the process
>
> It seems to me that this had better mention 8192 as the default.

I agree, done.

v5 is attached.


--
Regards,
Nazir Bilal Yavuz
Microsoft