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