Horton

Re: Show WAL write and fsync stats in pg_stat_io

Bertrand Drouvot <bertranddrouvot.pg@gmail.com>

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

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

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

=== 6

I'll also do some benchmark on my side.

[1]: https://www.postgresql.org/message-id/Z5o9OQ0nwWD9tKTR%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com