Re: Show WAL write and fsync stats in pg_stat_io

On Mon, Jan 27, 2025 at 05:13:39PM +0300, Nazir Bilal Yavuz wrote:
> On Mon, 27 Jan 2025 at 16:59, Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> wrote:
>>
>> On Mon, 27 Jan 2025 at 03:52, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> We use PendingWalStats.wal_[ write | sync ]_time only to show timings
>> on the pg_stat_wal view, right? And now these pg_stat_wal.wal_[ write
>> | sync ]_time datas are fetched from the pg_stat_io view when the
>> track_wal_io_timing is on. So, I think it is correct to remove these.

As you say, removing the counters in the second patch does not matter
as if you are going to combine them and..

>>
>> I made a mistake while splitting the patches. The places where
>> 'PendingWalStats.wal_[ write | sync ]_time are incremented (the code
>> piece you shared)' are removed in 0002 (0001 now), but they should be
>> removed in 0003 (0002 now) instead. This is corrected in v11.

My issue was in the first patch that should not have removed them. My
apologies for the confusion, I should have pointed out that this was
likely an incorrect rebase and/or patch split.

> If we agree with removing PendingWalStats.wal_[ write | sync ]_time
> variables, then it would make sense to remove PgStat_PendingWalStats
> struct completely. We have that struct because [1] it is cheap to
> store PendingWalStats.wal_[ write | sync ]_time as instr_time instead
> of PgStat_Counter.
>
> [1] ca7b3c4c00

I agree that some simplification would be nice because it also makes
Bertrand's patch around [1] to not have some special handling with
PgStat_PendingWalStats without us losing monitoring capabilities, I
hope. So maximizing simplifications before integrating more
capabilities should make the whole implementation effort easier.

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]().

+ /* Report pending statistics to the cumulative stats system */
+ pgstat_flush_io(false);

This has been discussed previously under a pgstat_report_wal() call.
Why do you need this specific call? Perhaps this should be documented
as a comment?

+ if (io_object == IOOBJECT_WAL)
+ return track_wal_io_timing

Hmm. Andres has commented in the past that we want pg_stat_io to
server as a central place for all the I/O statistics. Thinking more
about that, I am not really convinced that we actually need to make
this area of the code in pgstat_io.c rely on two GUCs. How about
simplifying things so as we just rely on track_io_timing for
everything, without creating a strange dependency on the IOObject with
more routines like pgstat_should_track_io_time()? I'd really want
less of these GUCs, not more of them with cross-dependencies depending
on the stats kinds we are dealing with. If we replace the timings
from pg_stat_wal with the ones in pg_stat_io, we should be in a good
position to remove track_wal_io_timing entirely, as well. This has
the merit of making your patch a lot simpler, meaning extra bonus
points.

[1]: https://commitfest.postgresql.org/52/5492/
--
Michael