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(at)paquier(dot)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