Hi,
On Mon, 3 Feb 2025 at 11:50, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Mon, Feb 03, 2025 at 01:07:26PM +0900, Michael Paquier wrote:
> > On Fri, Jan 31, 2025 at 11:29:31AM +0300, Nazir Bilal Yavuz wrote:
> > > On Wed, 29 Jan 2025 at 18:16, Bertrand Drouvot
> > > <bertranddrouvot.pg@gmail.com> wrote:
> > >> I think that's the main reason why ff99918c625 added this new GUC (looking at
> > >> the commit message). I'd feel more comfortable if we keep it.
> > >
> > > As Michael suggested, I will run a couple of benchmarks to see the
> > > actual effect of this change. Then let's see if this affects anything.
> >
> > I've looked at bit at all that today, and something like the attached
> > is what seems like the best streamlined version to me for the main
> > feature. I am also planning to run some short benchmarks with
> > track_io_timing=on on HEAD and with the patch, then see the
> > difference, without any relationship to track_wal_io_timing.
>
> Thanks!
>
> I've a few comments:
Thank you both for the v13 and the review!
> === 1
>
> + pgstat_count_io_op_time(IOOBJECT_WAL, IOCONTEXT_INIT, IOOP_WRITE,
> + io_start, 1, wal_segment_size);
>
> In case wal_init_zero is false, then we're only seeking to the end and write a
> solitary byte. Then, is reporting "wal_segment_size" correct?
I think you are right. It would make sense to have two
pgstat_count_io_op_time() calls here. One for wal_segment_size and one
for solitary byte.
> === 2
>
> + /*
> + * Measure I/O timing to write WAL data, for pg_stat_wal
> + * and/or pg_stat_io.
> + */
> + start = pgstat_prepare_io_time(track_wal_io_timing || track_io_timing);
>
> I think that makes sense done that way (as track_wal_io_timing does not have
> any effect in pgstat_count_io_op_time()). Nit: maybe change the order in the
> comment to reflect the code ordering? (I mean to say re-word to "for pg_stat_io
> and/or pg_stat_wal). The order is ok in issue_xlog_fsync() though.
>
> === 3
>
> What about adding a message in the doc as mentioned in [1]? (I'd not be surprised
> if some people wonder why the "bytes" fields differ).
>
> === 4
>
> pgstat_tracks_io_object() starts to be hard to read. I wonder if it could be
> simplified with switch but that could be done after this one goes in.
>
> === 5
>
> I think this patch will help simplify the per-backend WAL related patch, that's
> nice.
And I agree with the other comments you mentioned.
> === 6
>
> I'll also do some benchmark on my side.
Thanks!
--
Regards,
Nazir Bilal Yavuz
Microsoft
