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

Hi,

On Mon, Feb 24, 2025 at 04:41:36AM -0500, Andres Freund wrote:
> Hi,
>
> On 2025-02-20 14:37:18 +0900, Michael Paquier wrote:
> > a051e71e28a added this information into pg_stat_io (with more details and
> > granularity), so there is no need to keep it in pg_stat_wal. This also
> > allows to remove PendingWalStats and simplifies up coming commits related
> > to per backend WAL statistics. Once done, track_wal_io_timing becomes useless
> > so it is also removed.
> >
> > In passing remove the pgstat_prepare_io_time() parameter now that
> > track_wal_io_timing is gone.
>
> I don't think this is a good idea - there was a good reason for
> track_wal_io_timing to exist, namely that it happens while holding one of the
> two most contended locks in postgres. On many systems it'll be an ok constant
> overhead to enable track_io_timing, but enabling track_wal_io_timing will
> cause scalability issues. Now you made it impossible to separate those two
> situations, forcing disabling of all IO timing.

a051e71e28a added the "timing tracking" in the WAL code path with "only"
track_io_timing enabled (and track_wal_io_timing still false). Here, in this thread,
we remove unnecessary INSTR_TIME_SET_CURRENT()/INSTR_TIME_ACCUM_DIFF() calls when
both track_io_timing and track_wal_io_timing were enabled.

So I think that your main concern comes from the fact that a051e71e28a added the
"timing tracking" in the WAL code path with "only" track_io_timing enabled.

That's a concern we also had and discussed in [1]. The question was: should
we track the WAL timing stats in pg_stat_io only when track_wal_io_timing is
enabled or relying only on track_io_timing would be enough?

We ran several benchmarks ([2], [3] and [4]) and based on the results we reached
the conclusion that to rely only on track_io_timing to display the WAL timing
stats in pg_stat_io was "good" enough.

If you feel strongly that we should keep track_wal_io_timing then we would need
to change a bit the logic in a051e71e28a and then keep it in the current thread (
but still removing the "now useless" pg_stat_wal fields).

[1]: https://www.postgresql.org/message-id/CAN55FZ1qOt_gjhQgJdQZjO1KebBLuZcHz4DYmjfUvF3yGBSahw%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/Z6CcglxJF8XW%2BR7W%40ip-10-97-1-34.eu-west-3.compute.internal
[3]: https://www.postgresql.org/message-id/CAN55FZ0thZHTBbXwAsNhfrRdgmDwxWbLPiZyh%2BTG9CrC1V8TeA%40mail.gmail.com
[4]: https://www.postgresql.org/message-id/Z6HH150-Aw6ilQYE%40paquier.xyz

Regards,

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