Horton

Re: Show WAL write and fsync stats in pg_stat_io

Nazir Bilal Yavuz <byavuz81@gmail.com>

View thread
From: Nazir Bilal Yavuz <byavuz81@gmail.com>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>, Melanie Plageman <melanieplageman@gmail.com>, Andres Freund <andres@anarazel.de>, pgsql-hackers <pgsql-hackers@postgresql.org>, "bharath.rupireddyforpostgres@gmail.com" <bharath.rupireddyforpostgres@gmail.com>, Thomas Munro <thomas.munro@gmail.com>
Date: 2025-02-03T11:34:29Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix copy-paste error related to the autovacuum launcher in pgstat_io.c

  2. Move SQL tests of pg_stat_io for WAL data to recovery test 029_stats_restart

  3. Add data for WAL in pg_stat_io and backend statistics

  4. Improve comment on top of pgstat_count_io_op_time()

  5. Refactor pgstat_prepare_io_time() with an input argument instead of a GUC

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