Re: Show WAL write and fsync stats in pg_stat_io

On Wed, Jan 29, 2025 at 02:57:21PM +0300, Nazir Bilal Yavuz wrote:
> On Tue, 28 Jan 2025 at 07:23, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> What you doing in 0001 is a first good step towards this goal, as this
>> also plugs in a few things for backend statistics with the calls to
>> pgstat_count_io_op[_time]().
>
> I agree. Do you think that we need to do this simplification in this
> thread or does it need its own thread?

As far as I understand, the simplifications in PgStat_PendingWalStats
require the changes of this thread first, so keeping them around for
now sounds OK to me.

> I agree with you but it was discussed before in this thread [2]. It
> was decided to use both track_wal_io_timing and track_io_timing
> because of the overhead that track_wal_io_timing creates but we can
> still re-discuss it. Do you think that this discussion needs its own
> thread?

Let's decide it on this thread. You have done a benchmark with fsync
disabled for something that only stresses WAL. And it is very
dependent on the clock source. Would you really see a difference
under a normal pgbench workload? For example, should we compare HEAD
and the patch with track_io_timing=on but track_wal_io_timing=off
with a modified version of the patch so as IOOBJECT_WAL timing data is
controlled by track_io_timing=on? The previous results could have
been also influenced by the timings of pg_stat_wal because
track_wal_io_timing was on.

> If we continue to discuss it in this thread, I am in favor of removing
> track_wal_io_timing and using track_io_timing for all types of I/Os.
> Like you said, this cross-dependency makes things more complex than
> they used to be. Downside of removing track_wal_io_timing is affecting
> people who:
>
> 1- Want to track timings of only WAL I/Os.
> 2- Want to track timings of all IOs except WAL I/Os.
>
> I think the first group is more important than the second because
> track_io_timing already creates overhead.
>
> One additional thing is that I think track_io_timing is a general
> word. When it exists, I do not expect there to be another GUC like
> track_wal_io_timing to track WAL I/Os' timings.

Just to be clear here, I'd be okay to remove entirely the GUC
track_wal_io_timing iff pg_stat_wal has no more need for it if we feed
the data of pg_stat_io to pg_stat_wal. Having track_io_timing be
used for all the timing information in pg_stat_io makes the whole
design leaner, IMO, removing it from the patch and pgstat_io.c
simplifies a lot the user history.
--
Michael