Horton

Re: Show WAL write and fsync stats in pg_stat_io

Michael Paquier <michael@paquier.xyz>

View thread
From: Michael Paquier <michael@paquier.xyz>
To: Nazir Bilal Yavuz <byavuz81@gmail.com>
Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, 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-04T07:55:03Z
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

On Mon, Feb 03, 2025 at 02:34:29PM +0300, Nazir Bilal Yavuz wrote:
> On Mon, 3 Feb 2025 at 11:50, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
>> === 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.

Ah, right.  We can just use one call with the size written set
depending on wal_init_zero, because this is still a IOOP_WRITE for a
IOCONTEXT_INIT in both cases.  Only the size changes as we are in
XLogFileInitInternal().

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

Sure.  Fine by me.  This makes things a bit more consistent across the
board.

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

Not sure about that.  Perhaps you have something in mind?

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

If you have a proposal, feel free.  The current style is something I'm
used to, as well, so that does not bother me much..

At the end, we want this patch and this data, and my benchmarcking is
not showing much differences even if going through a workload with
many pages, so I've used the version relying entirely on
track_io_timing and applied it.

If we split these timings across more GUCs, one thing to consider
would be a third GUC which is neither track_wal_io_timing nor
track_io_timing to keep things independent, but I am not really
convinced that's necessary.

Now, for the rest..
--
Michael