Horton

Thread

Commits

  1. Rework macro pgstat_is_ioop_tracked_in_bytes()

  2. Remove assertion in pgstat_count_io_op()

  3. Make pg_stat_io count IOs as bytes instead of blocks for some operations

  4. Merge pgstat_count_io_op_n() and pgstat_count_io_op()

  1. Make pg_stat_io view count IOs as bytes instead of blocks

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-09-11T11:19:02Z

    Hi,

Currently, in the pg_stat_io view, IOs are counted as blocks. However,
there are two issues with this approach:

1- The actual number of IO requests to the kernel is lower because IO
requests can be merged before sending the final request. Additionally, it
appears that all IOs are counted in block size.
2- Some IOs may not align with block size. For example, WAL read IOs are
done in variable bytes and it is not possible to correctly show these IOs
in the pg_stat_io view [1].

To address this, I propose showing the total number of IO requests to the
kernel (as smgr function calls) and the total number of bytes in the IO. To
implement this change, the op_bytes column will be removed from the
pg_stat_io view. Instead, the [reads | writes | extends] columns will track
the total number of IO requests, and newly added [read | write |
extend]_bytes columns will track the total number of bytes in the IO.

Example benefit of this change:

Running query [2], the result is:

╔═══════════════════╦══════════╦══════════╦═══════════════╗
║    backend_type   ║  object  ║  context ║ avg_io_blocks ║
╠═══════════════════╬══════════╬══════════╬═══════════════╣
║   client backend  ║ relation ║ bulkread ║     15.99     ║
╠═══════════════════╬══════════╬══════════╬═══════════════╣
║ background worker ║ relation ║ bulkread ║     15.99     ║
╚═══════════════════╩══════════╩══════════╩═══════════════╝

You can rerun the same query [2] after setting io_combine_limit to 32 [3].
The result is:

╔═══════════════════╦══════════╦══════════╦═══════════════╗
║    backend_type   ║  object  ║  context ║ avg_io_blocks ║
╠═══════════════════╬══════════╬══════════╬═══════════════╣
║   client backend  ║ relation ║ bulkread ║     31.70     ║
╠═══════════════════╬══════════╬══════════╬═══════════════╣
║ background worker ║ relation ║ bulkread ║     31.60     ║
╚═══════════════════╩══════════╩══════════╩═══════════════╝

I believe that having visibility into avg_io_[bytes | blocks] is valuable
information that could help optimize Postgres.

Any feedback would be appreciated.

[1]
https://www.postgresql.org/message-id/CAN55FZ1ny%2B3kpdm5X3nGZ2Jp3wxZO-744eFgxktS6YQ3%3DOKR-A%40mail.gmail.com

[2]
CREATE TABLE t as select i, repeat('a', 600) as filler from
generate_series(1, 10000000) as i;
SELECT pg_stat_reset_shared('io');
SELECT * FROM t WHERE i = 0;
SELECT backend_type, object, context, TRUNC((read_bytes / reads / (SELECT
current_setting('block_size')::numeric)), 2) as avg_io_blocks FROM
pg_stat_io WHERE reads > 0;

[3] SET io_combine_limit TO 32;

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

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

    Melanie Plageman <melanieplageman@gmail.com> — 2024-11-27T16:08:01Z

    On Wed, Sep 11, 2024 at 7:19 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
> Currently, in the pg_stat_io view, IOs are counted as blocks. However, there are two issues with this approach:
>
> 1- The actual number of IO requests to the kernel is lower because IO requests can be merged before sending the final request. Additionally, it appears that all IOs are counted in block size.

I think this is a great idea. It will allow people to tune
io_combine_limit as you mention below.

> 2- Some IOs may not align with block size. For example, WAL read IOs are done in variable bytes and it is not possible to correctly show these IOs in the pg_stat_io view [1].

Yep, this makes a lot of sense as a solution.

> To address this, I propose showing the total number of IO requests to the kernel (as smgr function calls) and the total number of bytes in the IO. To implement this change, the op_bytes column will be removed from the pg_stat_io view. Instead, the [reads | writes | extends] columns will track the total number of IO requests, and newly added [read | write | extend]_bytes columns will track the total number of bytes in the IO.

smgr API seems like the right place for this.

> Example benefit of this change:
>
> Running query [2], the result is:
>
> ╔═══════════════════╦══════════╦══════════╦═══════════════╗
> ║    backend_type   ║  object  ║  context ║ avg_io_blocks ║
> ╠═══════════════════╬══════════╬══════════╬═══════════════╣
> ║   client backend  ║ relation ║ bulkread ║     15.99     ║
> ╠═══════════════════╬══════════╬══════════╬═══════════════╣
> ║ background worker ║ relation ║ bulkread ║     15.99     ║
> ╚═══════════════════╩══════════╩══════════╩═══════════════╝

I don't understand why background worker is listed here.

> You can rerun the same query [2] after setting io_combine_limit to 32 [3]. The result is:
>
> ╔═══════════════════╦══════════╦══════════╦═══════════════╗
> ║    backend_type   ║  object  ║  context ║ avg_io_blocks ║
> ╠═══════════════════╬══════════╬══════════╬═══════════════╣
> ║   client backend  ║ relation ║ bulkread ║     31.70     ║
> ╠═══════════════════╬══════════╬══════════╬═══════════════╣
> ║ background worker ║ relation ║ bulkread ║     31.60     ║
> ╚═══════════════════╩══════════╩══════════╩═══════════════╝
>
> I believe that having visibility into avg_io_[bytes | blocks] is valuable information that could help optimize Postgres.

In general, for this example, I think it would be more clear if you
compared what visibility we have in pg_stat_io on master with what
visibility we have with your patch.

I like that you show how io_combine_limit can be tuned using this, but
I don't think the problem statement is clear nor is the full
narrative.

> CREATE TABLE t as select i, repeat('a', 600) as filler from generate_series(1, 10000000) as i;
> SELECT pg_stat_reset_shared('io');
> SELECT * FROM t WHERE i = 0;
> SELECT backend_type, object, context, TRUNC((read_bytes / reads / (SELECT current_setting('block_size')::numeric)), 2) as avg_io_blocks FROM pg_stat_io WHERE reads > 0;

I like that you calculate the avg_io_blocks, but I think it is good to
show the raw columns as well.

- Melanie

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-28T13:39:21Z

    Hi,

On Wed, Nov 27, 2024 at 11:08:01AM -0500, Melanie Plageman wrote:
> On Wed, Sep 11, 2024 at 7:19 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> >
> > Currently, in the pg_stat_io view, IOs are counted as blocks. However, there are two issues with this approach:
> >
> > 1- The actual number of IO requests to the kernel is lower because IO requests can be merged before sending the final request. Additionally, it appears that all IOs are counted in block size.
> 
> I think this is a great idea. It will allow people to tune
> io_combine_limit as you mention below.
> 
> > 2- Some IOs may not align with block size. For example, WAL read IOs are done in variable bytes and it is not possible to correctly show these IOs in the pg_stat_io view [1].
> 
> Yep, this makes a lot of sense as a solution.

Thanks for the patch! I also think it makes sense.

A few random comments:

=== 1

+       /*
+        * If IO done in bytes and byte is <= 0, this means there is an error
+        * while doing an IO. Don't count these IOs.
+        */

s/byte/bytes/?

also:

The pgstat_io_count_checks() parameter is uint64. Does it mean it has to be
changed to int64?

Also from what I can see the calls are done with those values:

- 0
- io_buffers_len * BLCKSZ
- extend_by * BLCKSZ
- BLCKSZ

could io_buffers_len and extend_by be < 0? If not, is the comment correct?

=== 2

+       Assert((io_op == IOOP_READ || io_op == IOOP_WRITE || io_op == IOOP_EXTEND

and

+       if ((io_op == IOOP_READ || io_op == IOOP_WRITE || io_op == IOOP_EXTEND) &&

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.

=== 3

+pgstat_io_count_checks(IOObject io_object, IOContext io_context, IOOp io_op, uint64 bytes)
+{
+       Assert((unsigned int) io_object < IOOBJECT_NUM_TYPES);
+       Assert((unsigned int) io_context < IOCONTEXT_NUM_TYPES);
+       Assert((unsigned int) io_op < IOOP_NUM_TYPES);
+       Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op));

IOObject and IOContext are passed only for the assertions. What about removing
them from there and put the asserts in other places?

=== 4

+       /* Only IOOP_READ, IOOP_WRITE and IOOP_EXTEND can do IO in bytes. */

Not sure about "can do IO in bytes" (same wording is used in multiple places).

=== 5

/* Convert to numeric. */

"convert to numeric"? to be consistent with others single line comments around.

Regards,

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

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

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-12-04T11:29:00Z

    Hi,

Thanks for looking into this!

On Wed, 27 Nov 2024 at 19:08, Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Wed, Sep 11, 2024 at 7:19 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> >
> > Currently, in the pg_stat_io view, IOs are counted as blocks. However, there are two issues with this approach:
> >
> > 1- The actual number of IO requests to the kernel is lower because IO requests can be merged before sending the final request. Additionally, it appears that all IOs are counted in block size.
>
> I think this is a great idea. It will allow people to tune
> io_combine_limit as you mention below.
>
> > 2- Some IOs may not align with block size. For example, WAL read IOs are done in variable bytes and it is not possible to correctly show these IOs in the pg_stat_io view [1].
>
> Yep, this makes a lot of sense as a solution.
>
> > To address this, I propose showing the total number of IO requests to the kernel (as smgr function calls) and the total number of bytes in the IO. To implement this change, the op_bytes column will be removed from the pg_stat_io view. Instead, the [reads | writes | extends] columns will track the total number of IO requests, and newly added [read | write | extend]_bytes columns will track the total number of bytes in the IO.
>
> smgr API seems like the right place for this.
>
> > Example benefit of this change:
> >
> > Running query [2], the result is:
> >
> > ╔═══════════════════╦══════════╦══════════╦═══════════════╗
> > ║    backend_type   ║  object  ║  context ║ avg_io_blocks ║
> > ╠═══════════════════╬══════════╬══════════╬═══════════════╣
> > ║   client backend  ║ relation ║ bulkread ║     15.99     ║
> > ╠═══════════════════╬══════════╬══════════╬═══════════════╣
> > ║ background worker ║ relation ║ bulkread ║     15.99     ║
> > ╚═══════════════════╩══════════╩══════════╩═══════════════╝
>
> I don't understand why background worker is listed here.

Parallel sequential scan happens in this example and parallel workers
are listed as background workers. After setting
'max_parallel_workers_per_gather' to 0, it is gone.

> > You can rerun the same query [2] after setting io_combine_limit to 32 [3]. The result is:
> >
> > ╔═══════════════════╦══════════╦══════════╦═══════════════╗
> > ║    backend_type   ║  object  ║  context ║ avg_io_blocks ║
> > ╠═══════════════════╬══════════╬══════════╬═══════════════╣
> > ║   client backend  ║ relation ║ bulkread ║     31.70     ║
> > ╠═══════════════════╬══════════╬══════════╬═══════════════╣
> > ║ background worker ║ relation ║ bulkread ║     31.60     ║
> > ╚═══════════════════╩══════════╩══════════╩═══════════════╝
> >
> > I believe that having visibility into avg_io_[bytes | blocks] is valuable information that could help optimize Postgres.
>
> In general, for this example, I think it would be more clear if you
> compared what visibility we have in pg_stat_io on master with what
> visibility we have with your patch.

I am listing the changes as text, images are also attached.

* [reads | writes | extends] columns count the number of smgr function
calls now. They were counting the number of block IOs before.
* op_bytes column is removed from the view because each IO could have
a different size. They are not always equal to op_bytes.
* [read_bytes | write_bytes | extend_bytes] columns are added. These
columns count IO sizes as bytes.

There are two different IO cases:

1- Size of the IOs are constant:
* See 'client backend / bulkread' row, If you divide read_bytes
columns' value (6826754048) to BLCKSZ (8192) in the patched image, you
get the reads columns' value (833344) in the upstream image. So, we
actually do not lose any information when the size of the IOs are
constant.

2- Size of the IOs are different:
* Upstream version will give wrong information in this case. For
example see WALRead() function. pg_pread() is called with different
segbytes values in this function. It is not possible to correctly show
this stat in pg_stat_io view.

The problem with the upstream version of the pg_stat_io view is that
multiplying the number of blocks with the op_bytes does not always
give the total IO size. Also, it looks like Postgres is doing one IO
request per block. This patch tries to address these problems.

> I like that you show how io_combine_limit can be tuned using this, but
> I don't think the problem statement is clear nor is the full
> narrative.

I just wanted to show one piece of information that can be gathered
with the patched version, it was not possible to gather that before.

> > CREATE TABLE t as select i, repeat('a', 600) as filler from generate_series(1, 10000000) as i;
> > SELECT pg_stat_reset_shared('io');
> > SELECT * FROM t WHERE i = 0;
> > SELECT backend_type, object, context, TRUNC((read_bytes / reads / (SELECT current_setting('block_size')::numeric)), 2) as avg_io_blocks FROM pg_stat_io WHERE reads > 0;
>
> I like that you calculate the avg_io_blocks, but I think it is good to
> show the raw columns as well.

Images of the view after running the query [1] are attached.

P.S. I attached the images of the view because I do not know how they
will look if I copy paste them as text. If there is a way to add them
as text without distortion, please let me know.

[1]
SET track_io_timing to ON;
SET max_parallel_workers_per_gather TO 0;
SELECT pg_stat_reset_shared('io');
CREATE TABLE t as select i, repeat('a', 600) as filler from
generate_series(1, 10000000) as i;
SELECT * FROM t WHERE i = 0;
SELECT * FROM pg_stat_io;

--
Regards,
Nazir Bilal Yavuz
Microsoft

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

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-12-04T11:49:11Z

    Hi,

Thanks for looking into this!

On Thu, 28 Nov 2024 at 16:39, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Wed, Nov 27, 2024 at 11:08:01AM -0500, Melanie Plageman wrote:
> > On Wed, Sep 11, 2024 at 7:19 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> > >
> > > Currently, in the pg_stat_io view, IOs are counted as blocks. However, there are two issues with this approach:
> > >
> > > 1- The actual number of IO requests to the kernel is lower because IO requests can be merged before sending the final request. Additionally, it appears that all IOs are counted in block size.
> >
> > I think this is a great idea. It will allow people to tune
> > io_combine_limit as you mention below.
> >
> > > 2- Some IOs may not align with block size. For example, WAL read IOs are done in variable bytes and it is not possible to correctly show these IOs in the pg_stat_io view [1].
> >
> > Yep, this makes a lot of sense as a solution.
>
> Thanks for the patch! I also think it makes sense.
>
> A few random comments:
>
> === 1
>
> +       /*
> +        * If IO done in bytes and byte is <= 0, this means there is an error
> +        * while doing an IO. Don't count these IOs.
> +        */
>
> s/byte/bytes/?

This is removed, please look below for the explanation.

> also:
>
> The pgstat_io_count_checks() parameter is uint64. Does it mean it has to be
> changed to int64?
>
> Also from what I can see the calls are done with those values:
>
> - 0
> - io_buffers_len * BLCKSZ
> - extend_by * BLCKSZ
> - BLCKSZ
>
> could io_buffers_len and extend_by be < 0? If not, is the comment correct?

You are right, no need to have this check; it can not be less than 0.
I completely removed the function now.

> === 2
>
> +       Assert((io_op == IOOP_READ || io_op == IOOP_WRITE || io_op == IOOP_EXTEND
>
> and
>
> +       if ((io_op == IOOP_READ || io_op == IOOP_WRITE || io_op == IOOP_EXTEND) &&
>
> 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. Created an
inline function instead of macro and added this 'Assert((unsigned int)
io_object < IOOBJECT_NUM_TYPES);' to function.

> === 3
>
> +pgstat_io_count_checks(IOObject io_object, IOContext io_context, IOOp io_op, uint64 bytes)
> +{
> +       Assert((unsigned int) io_object < IOOBJECT_NUM_TYPES);
> +       Assert((unsigned int) io_context < IOCONTEXT_NUM_TYPES);
> +       Assert((unsigned int) io_op < IOOP_NUM_TYPES);
> +       Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op));
>
> IOObject and IOContext are passed only for the assertions. What about removing
> them from there and put the asserts in other places?

Done. I moved these checks to the pgstat_count_io_op_n() function. The
code looks more like its previous version now.

> === 4
>
> +       /* Only IOOP_READ, IOOP_WRITE and IOOP_EXTEND can do IO in bytes. */
>
> 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.

> === 5
>
> /* Convert to numeric. */
>
> "convert to numeric"? to be consistent with others single line comments around.

Done.

--
Regards,
Nazir Bilal Yavuz
Microsoft

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-05T09:13:53Z

    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.pg@gmail.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/

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

> 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?

Regards,

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

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

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-12-06T09:41:55Z

    Hi,

On Thu, 5 Dec 2024 at 12:13, Bertrand Drouvot
<bertranddrouvot.pg@gmail.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.pg@gmail.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

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

    Michael Paquier <michael@paquier.xyz> — 2024-12-24T06:12:48Z

    On Fri, Dec 06, 2024 at 12:41:55PM +0300, Nazir Bilal Yavuz wrote:
> 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).

Splitting op_bytes into three fields sounds like a good idea.  Count
me in regarding the concept to depend less on BLCKSZ.

 typedef enum IOOp
 {
+	/* IOs not tracked in bytes */
 	IOOP_EVICT,
-	IOOP_EXTEND,
 	IOOP_FSYNC,
 	IOOP_HIT,
-	IOOP_READ,
 	IOOP_REUSE,
-	IOOP_WRITE,
 	IOOP_WRITEBACK,
+
+	/* IOs tracked in bytes */
+	IOOP_EXTEND,
+	IOOP_READ,
+	IOOP_WRITE,
 } IOOp;

pg_stat_io_build_tuples() is now the code path taken when building the
tuples returned by pg_stat_io, meaning that the new function called
pg_stat_get_backend_io() is also going to need an update in its list
of output parameters to accomodate with what you are changing here.
Calling your new pgstat_get_io_byte_index() in the new refactored
routine is also necessary.  Sorry about that.

Could you send a rebase, please?  I can promise to review this
thread's new patch, as that will also move the needle regarding your
work with pg_stat_io to track WAL activity.
--
Michael

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

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-12-26T11:41:26Z

    Hi,

On Tue, 24 Dec 2024 at 09:13, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Dec 06, 2024 at 12:41:55PM +0300, Nazir Bilal Yavuz wrote:
> > 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).
>
> Splitting op_bytes into three fields sounds like a good idea.  Count
> me in regarding the concept to depend less on BLCKSZ.
>
>  typedef enum IOOp
>  {
> +       /* IOs not tracked in bytes */
>         IOOP_EVICT,
> -       IOOP_EXTEND,
>         IOOP_FSYNC,
>         IOOP_HIT,
> -       IOOP_READ,
>         IOOP_REUSE,
> -       IOOP_WRITE,
>         IOOP_WRITEBACK,
> +
> +       /* IOs tracked in bytes */
> +       IOOP_EXTEND,
> +       IOOP_READ,
> +       IOOP_WRITE,
>  } IOOp;
>
> pg_stat_io_build_tuples() is now the code path taken when building the
> tuples returned by pg_stat_io, meaning that the new function called
> pg_stat_get_backend_io() is also going to need an update in its list
> of output parameters to accomodate with what you are changing here.
> Calling your new pgstat_get_io_byte_index() in the new refactored
> routine is also necessary.  Sorry about that.

No problem at all, thank you for reminding me!

> Could you send a rebase, please?  I can promise to review this
> thread's new patch, as that will also move the needle regarding your
> work with pg_stat_io to track WAL activity.

Thanks! v4 is attached. I quickly tested the pg_stat_get_backend_io()
function and it seems it is working.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

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

    Michael Paquier <michael@paquier.xyz> — 2025-01-09T02:59:38Z

    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

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

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-01-09T07:15:20Z

    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

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

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-01-09T07:59:52Z

    Hi,

On Thu, 9 Jan 2025 at 10:15, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
> On Thu, 9 Jan 2025 at 05:59, Michael Paquier <michael@paquier.xyz> wrote:
> >
> >
> > +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.
>
> v5 is attached.

I missed a compilation error. Since the is_ioop_tracked_in_bytes() is
only used in the assert and the asserts are disabled in the production
builds, CompilerWarnings task gives -Wunused-function error:

pgstat_io.c:28:1: error: unused function 'is_ioop_tracked_in_bytes'
[-Werror,-Wunused-function]

pg_attribute_unused() is added to the function to silence that. v6 is attached.

--
Regards,
Nazir Bilal Yavuz
Microsoft

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

    Michael Paquier <michael@paquier.xyz> — 2025-01-09T08:11:02Z

    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.
--
Michael

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

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-01-09T11:20:16Z

    Hi,

On Thu, 9 Jan 2025 at 11:11, Michael Paquier <michael@paquier.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.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-09T15:30:37Z

    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@paquier.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

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

    Michael Paquier <michael@paquier.xyz> — 2025-01-10T01:46:46Z

    On Thu, Jan 09, 2025 at 03:30:37PM +0000, Bertrand Drouvot wrote:
> 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)

Indeed.  Your suggestion to use a macro makes more sense to me because
is_ioop_tracked_in_bytes() itself has an Assert(), while being itself
only used in an assertion, as you say.  Better to document the
dependency on the ordering of IOOp, even if that's kind of hard to
miss.

> 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.

This kind of cleanup should happen first, and it simplifies a bit the
reading of v7-0001 as we would just append a new argument to the
count_io_op() routine for the number of bytes.  I was looking at that
and chose to stick with count_io_op() rather than count_io_op_n() as
we would have only one routine. 

     pgstat_count_io_op_time(IOOBJECT_RELATION, io_context, IOOP_EXTEND,
-                            io_start, extend_by);
+                            io_start, 1, extend_by * BLCKSZ);
[...]
     pgstat_count_io_op_time(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_EXTEND,
-                            io_start, extend_by);
+                            io_start, 1, extend_by * BLCKSZ);

Something worth mentioning.  I had a small doubt about this one for
temp and persistent relations, as it changes the extend count.
Anyway, I think that this is better: we are only doing one extend
batch, worth (extend_by * BLCKSZ).

Two times my fault today that the main patch does not apply anymore
(three times at least in total), so I have rebased it myself, and did
an extra review while on it, switching the code to use a macro.  That
seems OK here.  Please let me know if you have more comments.

(Still need to spend more time on the commit message, will do that
later).
--
Michael

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

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-01-10T07:15:52Z

    Hi,

On Fri, 10 Jan 2025 at 04:47, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jan 09, 2025 at 03:30:37PM +0000, Bertrand Drouvot wrote:
> > 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)
>
> Indeed.  Your suggestion to use a macro makes more sense to me because
> is_ioop_tracked_in_bytes() itself has an Assert(), while being itself
> only used in an assertion, as you say.  Better to document the
> dependency on the ordering of IOOp, even if that's kind of hard to
> miss.

I did not make it like this because now the
pgstat_is_ioop_tracked_in_bytes macro will return false when io_op >=
IOOP_NUM_TYPES. But I agree that having a macro has more benefits,
also there already is a check for the 'io_op < IOOP_NUM_TYPES' in the
pgstat_count_io_op() function.

>
> > 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.
>
> This kind of cleanup should happen first, and it simplifies a bit the
> reading of v7-0001 as we would just append a new argument to the
> count_io_op() routine for the number of bytes.  I was looking at that
> and chose to stick with count_io_op() rather than count_io_op_n() as
> we would have only one routine.
>
>      pgstat_count_io_op_time(IOOBJECT_RELATION, io_context, IOOP_EXTEND,
> -                            io_start, extend_by);
> +                            io_start, 1, extend_by * BLCKSZ);
> [...]
>      pgstat_count_io_op_time(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_EXTEND,
> -                            io_start, extend_by);
> +                            io_start, 1, extend_by * BLCKSZ);
>
> Something worth mentioning.  I had a small doubt about this one for
> temp and persistent relations, as it changes the extend count.
> Anyway, I think that this is better: we are only doing one extend
> batch, worth (extend_by * BLCKSZ).

I agree with you.

>
> Two times my fault today that the main patch does not apply anymore
> (three times at least in total), so I have rebased it myself, and did
> an extra review while on it, switching the code to use a macro.  That
> seems OK here.  Please let me know if you have more comments.

No worries, thank you for all of these!

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-10T08:23:46Z

    Hi,

On Fri, Jan 10, 2025 at 10:15:52AM +0300, Nazir Bilal Yavuz wrote:
> Hi,
> 
> On Fri, 10 Jan 2025 at 04:47, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Thu, Jan 09, 2025 at 03:30:37PM +0000, Bertrand Drouvot wrote:
> > > 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)
> >
> > Indeed.  Your suggestion to use a macro makes more sense to me because
> > is_ioop_tracked_in_bytes() itself has an Assert(), while being itself
> > only used in an assertion, as you say.  Better to document the
> > dependency on the ordering of IOOp, even if that's kind of hard to
> > miss.
> 
> But I agree that having a macro has more benefits,
> also there already is a check for the 'io_op < IOOP_NUM_TYPES' in the
> pgstat_count_io_op() function.

Yeah, I think we can remove the "io_op < IOOP_NUM_TYPE" assertion in
pgstat_count_io_op() (but keep this check as part of the macro).

> > That
> > seems OK here.  Please let me know if you have more comments.

Appart from the above, LGTM.

Regards,

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

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

    Michael Paquier <michael@paquier.xyz> — 2025-01-14T03:18:23Z

    On Fri, Jan 10, 2025 at 08:23:46AM +0000, Bertrand Drouvot wrote:
> On Fri, Jan 10, 2025 at 10:15:52AM +0300, Nazir Bilal Yavuz wrote:
>> But I agree that having a macro has more benefits,
>> also there already is a check for the 'io_op < IOOP_NUM_TYPES' in the
>> pgstat_count_io_op() function.
> 
> Yeah, I think we can remove the "io_op < IOOP_NUM_TYPE" assertion in
> pgstat_count_io_op() (but keep this check as part of the macro).
>
> Appart from the above, LGTM.

Okay, so applied.

And I've somewhat managed to fat-finger the business with
pgstat_count_io_op() with an incorrect rebase.  Will remove in a
minute..
--
Michael

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

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-01-14T13:43:28Z

    Hi,

On Tue, 14 Jan 2025 at 06:18, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jan 10, 2025 at 08:23:46AM +0000, Bertrand Drouvot wrote:
> > On Fri, Jan 10, 2025 at 10:15:52AM +0300, Nazir Bilal Yavuz wrote:
> >> But I agree that having a macro has more benefits,
> >> also there already is a check for the 'io_op < IOOP_NUM_TYPES' in the
> >> pgstat_count_io_op() function.
> >
> > Yeah, I think we can remove the "io_op < IOOP_NUM_TYPE" assertion in
> > pgstat_count_io_op() (but keep this check as part of the macro).
> >
> > Appart from the above, LGTM.
>
> Okay, so applied.
>
> And I've somewhat managed to fat-finger the business with
> pgstat_count_io_op() with an incorrect rebase.  Will remove in a
> minute..

Thank you!

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-15T17:19:03Z

    Nazir Bilal Yavuz <byavuz81@gmail.com> writes:
> On Tue, 14 Jan 2025 at 06:18, Michael Paquier <michael@paquier.xyz> wrote:
>> And I've somewhat managed to fat-finger the business with
>> pgstat_count_io_op() with an incorrect rebase.  Will remove in a
>> minute..

> Thank you!

Commit f92c854cf has caused some of the buildfarm members to
spout a new warning, eg at [1]:

ccache clang -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wformat-security -Wmissing-variable-declarations -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -g -O2 -I. -I. -I../../../../src/include  -D_GNU_SOURCE -I/usr/include/libxml2   -c -o pgstat_io.o pgstat_io.c
pgstat_io.c:81:9: warning: comparison of constant 8 with expression of type 'IOOp' (aka 'enum IOOp') is always true [-Wtautological-constant-out-of-range-compare]
        Assert(pgstat_is_ioop_tracked_in_bytes(io_op) || bytes == 0);
        ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pgstat_io.c:32:11: note: expanded from macro 'pgstat_is_ioop_tracked_in_bytes'
        ((io_op) < IOOP_NUM_TYPES && (io_op) >= IOOP_EXTEND)
                 ^ ~~~~~~~~~~~~~~
../../../../src/include/c.h:829:9: note: expanded from macro 'Assert'
                if (!(condition)) \\
                      ^~~~~~~~~
1 warning generated.

I don't see a reasonable way to alter that check to suppress this;
for instance, "(io_op) <= IOOP_WRITE" would probably still draw the
same warning.  I think most likely we have to remove that check, ie

 #define pgstat_is_ioop_tracked_in_bytes(io_op) \
-	((io_op) < IOOP_NUM_TYPES && (io_op) >= IOOP_EXTEND)
+	((io_op) >= IOOP_EXTEND)

I suppose one alternative is to re-order the enum so that the
upper-limit check in this macro *isn't* tautological ... but
that seems a bit silly.

			regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=demoiselle&dt=2025-01-15%2005%3A20%3A59&stg=build

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

    Michael Paquier <michael@paquier.xyz> — 2025-01-16T04:14:31Z

    On Wed, Jan 15, 2025 at 12:19:03PM -0500, Tom Lane wrote:
> I don't see a reasonable way to alter that check to suppress this;
> for instance, "(io_op) <= IOOP_WRITE" would probably still draw the
> same warning.  I think most likely we have to remove that check, ie
> 
>  #define pgstat_is_ioop_tracked_in_bytes(io_op) \
> -	((io_op) < IOOP_NUM_TYPES && (io_op) >= IOOP_EXTEND)
> +	((io_op) >= IOOP_EXTEND)
> 
> I suppose one alternative is to re-order the enum so that the
> upper-limit check in this macro *isn't* tautological ... but
> that seems a bit silly.

I cannot reproduce that, perhaps I'm just missing something with these
switches.  Do you think that a cast would cool things?  Please see the
attached for the idea.
--
Michael

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-16T04:34:14Z

    Michael Paquier <michael@paquier.xyz> writes:
> I cannot reproduce that, perhaps I'm just missing something with these
> switches.  Do you think that a cast would cool things?  Please see the
> attached for the idea.

There are only three animals showing this warning (ayu, batfish,
demoiselle) so it likely requires particular clang versions
as well as the right -W switches.

Maybe a cast would silence it, but your draft seems
underparenthesized.  (Which raises the question of whether we
should be using a macro for this at all --- probably there's
not much risk of double-evaluation being a problem, but a
static inline would remove the risk.)

			regards, tom lane

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

    Michael Paquier <michael@paquier.xyz> — 2025-01-16T05:13:08Z

    On Wed, Jan 15, 2025 at 11:34:14PM -0500, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > I cannot reproduce that, perhaps I'm just missing something with these
> > switches.  Do you think that a cast would cool things?  Please see the
> > attached for the idea.
> 
> There are only three animals showing this warning (ayu, batfish,
> demoiselle) so it likely requires particular clang versions
> as well as the right -W switches.

All of them on ppc with clang 4 or 5.  That's dated.  :)

> Maybe a cast would silence it, but your draft seems
> underparenthesized.  (Which raises the question of whether we
> should be using a macro for this at all --- probably there's
> not much risk of double-evaluation being a problem, but a
> static inline would remove the risk.)

Just for an assert, I would just remove the macro rather than have an
inline function.
--
Michael

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-16T05:18:38Z

    Michael Paquier <michael@paquier.xyz> writes:
> Just for an assert, I would just remove the macro rather than have an
> inline function.

Oh, I'd not noticed that there is only one caller.

However, the macro does provide a convenient place to hang the
warning comment about keeping it sync'd with the enum.
Personally I'd keep the macro but move it to pgstat.h, close
to the enum declaration, so that there's more than epsilon
chance of someone who's changing the enum noticing they need
to update it.

			regards, tom lane

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

    Michael Paquier <michael@paquier.xyz> — 2025-01-16T05:43:43Z

    On Thu, Jan 16, 2025 at 12:18:38AM -0500, Tom Lane wrote:
> However, the macro does provide a convenient place to hang the
> warning comment about keeping it sync'd with the enum.
> Personally I'd keep the macro but move it to pgstat.h, close
> to the enum declaration, so that there's more than epsilon
> chance of someone who's changing the enum noticing they need
> to update it.

Not completely sure about the number of parenthesis, but I hope that
this should be enough (extra set around io_op):
+#define pgstat_is_ioop_tracked_in_bytes(io_op) \
+	(((unsigned int) (io_op)) < IOOP_NUM_TYPES && \
+	 ((unsigned int) (io_op)) >= IOOP_EXTEND)
--
Michael

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-16T05:47:17Z

    Michael Paquier <michael@paquier.xyz> writes:
> Not completely sure about the number of parenthesis, but I hope that
> this should be enough (extra set around io_op):
> +#define pgstat_is_ioop_tracked_in_bytes(io_op) \
> +	(((unsigned int) (io_op)) < IOOP_NUM_TYPES && \
> +	 ((unsigned int) (io_op)) >= IOOP_EXTEND)

Yeah, that's safe parenthesis-wise.  Whether it'll silence
the warning from those old clangs remains to be seen.

(But if it doesn't, maybe it's not worth working harder,
given that they're old.)

			regards, tom lane

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-16T07:12:34Z

    Hi,

On Thu, Jan 16, 2025 at 12:47:17AM -0500, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > Not completely sure about the number of parenthesis, but I hope that
> > this should be enough (extra set around io_op):
> > +#define pgstat_is_ioop_tracked_in_bytes(io_op) \
> > +	(((unsigned int) (io_op)) < IOOP_NUM_TYPES && \
> > +	 ((unsigned int) (io_op)) >= IOOP_EXTEND)
> 
> Yeah, that's safe parenthesis-wise.  Whether it'll silence
> the warning from those old clangs remains to be seen.

Thanks for the report and the proposed "fix".

From what I can see, the above proposal does (at least) silent the warning
here (clang 5.0.1 and same as demoiselle): https://godbolt.org/z/cGosfzGne (we
can see the warning by using the current define and that the warning is gone
with the new define).

Let's see on the BF.

Regards,

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

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

    Michael Paquier <michael@paquier.xyz> — 2025-01-16T07:57:19Z

    On Thu, Jan 16, 2025 at 07:12:34AM +0000, Bertrand Drouvot wrote:
> From what I can see, the above proposal does (at least) silent the warning
> here (clang 5.0.1 and same as demoiselle): https://godbolt.org/z/cGosfzGne (we
> can see the warning by using the current define and that the warning is gone
> with the new define).

Forgot this trick, thanks for the reminder.  I'll check all that
tomorrow.
--
Michael

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

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-01-16T08:55:34Z

    Hi,

On Thu, 16 Jan 2025 at 10:12, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Thu, Jan 16, 2025 at 12:47:17AM -0500, Tom Lane wrote:
> > Michael Paquier <michael@paquier.xyz> writes:
> > > Not completely sure about the number of parenthesis, but I hope that
> > > this should be enough (extra set around io_op):
> > > +#define pgstat_is_ioop_tracked_in_bytes(io_op) \
> > > +   (((unsigned int) (io_op)) < IOOP_NUM_TYPES && \
> > > +    ((unsigned int) (io_op)) >= IOOP_EXTEND)
> >
> > Yeah, that's safe parenthesis-wise.  Whether it'll silence
> > the warning from those old clangs remains to be seen.
>
> Thanks for the report and the proposed "fix".
>
> From what I can see, the above proposal does (at least) silent the warning
> here (clang 5.0.1 and same as demoiselle): https://godbolt.org/z/cGosfzGne (we
> can see the warning by using the current define and that the warning is gone
> with the new define).

Thanks all!

I checked clang 4 as well on the link you sent and it also fixes the
warning there.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

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

    Michael Paquier <michael@paquier.xyz> — 2025-01-16T23:27:50Z

    On Thu, Jan 16, 2025 at 11:55:34AM +0300, Nazir Bilal Yavuz wrote:
> I checked clang 4 as well on the link you sent and it also fixes the
> warning there.

Confirmed here, so done this way.
--
Michael