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

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?

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.

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

s/does not tracked/is not tracked/.

+/*
+ * 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.

+ Number of units of size <symbol>BLCKSZ</symbol> which the process

It seems to me that this had better mention 8192 as the default.
--
Michael