Re: Remove wal_[sync|write][_time] from pg_stat_wal and track_wal_io_timing

On Tue, Feb 25, 2025 at 08:12:53AM +0000, Bertrand Drouvot wrote:
> The idea was to not let track_io_timing alone enable the timing in the WAL
> code path. My reasoning was: if you want to see timing in pg_stat_io then you
> need to enable track_io_timing. But that's not enough if you also want to see
> WAL timing, then you also need to set track_wal_io_timing. Your proposal also
> ensures that "track_io_timing alone can not enable the timing in the WAL code path",
> with a clear separation of duties, it's probably better so I'm fine with it.

One problem with this approach is that you would need to always pay
the price of the timings under track_io_timing if the WAL part is
enabled, so you'd lose flexibility compared to the past configuration.
Based on the complaint of upthread, a split makes things more
flexible, as it is possible to monitor one or the other or both.  I'm
OK to tweak things more as required, we still have time for that.

> Yeah, I though about it too but decided to not change the READ part in v1 (because
> I think they are less of a concern). OTOH, if you want to see the READ timing then
> you need to set track_wal_io_timing but then once the recovery is over then you'll
> need to disable track_wal_io_timing (if you don't want to pay the price for
> write/fsync activities).
> 
> OTOH pre a051e71e28a track_wal_io_timing was impacting only the write/fsyncs,
> in that regard v1 was close to that.

Still less consistent..  We've never tracked WAL reads in the stats up
to now, so we need to put it in one of these buckets.

> In the track_io_timing GUC description Shouldn't we also mention the
> wal object restriction, something like?
> 
> "
> in pg_stat_database, pg_stat_io (if object is not wal), in the output of the
> pg_stat_get_backend_io() function (if object is not wal)
> "
>
> The proposed doc changes are in the .txt attached (that applies on top of v2).

Sounds like a good set of additions as we have the two GUCs.

Bonus idea: We could also have more GUCs to control all that, or just
put eveything in one single GUC that takes a list of values made of
pairs of (object,op), then set the timings only for the combinations
listed in the GUC.  That feels a bit over-engineered, but you could
deeply control the areas where the data is aggregated.  I'm not really
convinced it is strictly necessary, but, well, that would not be the
first time people tell me I'm wrong..  All things said, v2 sounds like
it has the right balance for now based on what I am reading, providing
the same control as pg_stat_wal previously, so I've used it for now.
--
Michael