From: | Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> |
---|---|
To: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
Cc: | Melanie Plageman <melanieplageman(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Make pg_stat_io view count IOs as bytes instead of blocks |
Date: | 2024-12-04 11:49:11 |
Message-ID: | CAN55FZ0mezEWPSfGJi7U=WdxBVZJ6Wgnh5Y4dzj6KFMJEM4YeQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
Thanks for looking into this!
On Thu, 28 Nov 2024 at 16:39, Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)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(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
v2-0001-Make-pg_stat_io-count-IOs-as-bytes-instead-of-blo.patch | text/x-patch | 21.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Matthias van de Meent | 2024-12-04 11:56:08 | Re: Incorrect result of bitmap heap scan. |
Previous Message | Michael Paquier | 2024-12-04 11:38:58 | Re: Memory leak in WAL sender with pgoutput (v10~) |