Re: Show WAL write and fsync stats in pg_stat_io

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(dot)pg(at)gmail(dot)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