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

Hi,

On Tue, Feb 25, 2025 at 02:20:48PM +0900, Michael Paquier wrote:
> On Mon, Feb 24, 2025 at 01:08:03PM +0000, Bertrand Drouvot wrote:
> > I agree that we've to put everything in the picture (system with or without
> > cheap timing functions, lock contention and WAL flush disk time) and that we
> > can certainly find configuration/workload that would get benefits from a
> > dedicated track_wal_io_timing GUC.
> >
> > PFA a patch to re-introduce the track_wal_io_timing GUC and to ensure that the
> > WAL write and fsync timing activities are tracked in pg_stat_io (and
> > pg_stat_get_backend_io()) only if both track_io_timing and track_wal_io_timing
> > are enabled.
> >
> > Note that to change the a051e71e28a behavior, the attached patch adds an extra
> > "track_io_guc" parameter to pgstat_count_io_op_time() (like pgstat_prepare_io_time
> > already had in a051e71e28a).
>
> + bool track_timing = track_io_timing && track_wal_io_timing;
> - start = pgstat_prepare_io_time();
> + start = pgstat_prepare_io_time(track_timing);
>
> This does not represent exactly what I am understanding from the
> comment of upthread, because WAL IO timings would require both
> track_wal_io_timing and track_io_timing to be enabled with this patch.
> It seems to me that what we should do is to decouple completely the
> timings for WAL and non-WAL IO across the two GUCs track_wal_io_timing
> and track_io_timing, without any dependency between one and the other.
> This way, it is possible to only enable one of them without affecting
> the paths of the other, or both if you are ready to pay the price.

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.

> Two new things tracked in pg_stat_io are WALRead() and XLogPageRead(),
> which are used at recovery, and for consistency with the rest there is
> a good argument for controlling these as well with
> track_wal_io_timing, I guess.

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.

I think both idea (v1 / v2) have pros and cons and I don't have a strong opinion
on it (though I do prefer v1 a bit for the reasons mentioned above).

> void
> pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op,
> - instr_time start_time, uint32 cnt, uint64 bytes)
> + instr_time start_time, uint32 cnt, uint64 bytes,
> + bool track_io_guc)
>
> Not much a fan of the new argument to pass the GUC value, which could
> be error prone. It would be simpler to check that start_time is 0
> instead. There is no need to change the other callers of
> pgstat_count_io_op_time() if we do that.

Yeah I thought about it too

- if (track_io_guc)
+ if (!INSTR_TIME_IS_ZERO(start_time))

INSTR_TIME_IS_ZERO is cheap so it is fine by me.

> pgstat_count_backend_io_op_time() would have triggered an assertion
> failure, it needs to be adjusted.

With v2 in place, yeah.

> With more tweaks applied to the docs, the attached is my result.

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)
"

Also in the track_wal_io_timing GUC description add the wal object restriction
for the function:

"
and in the output of the pg_stat_get_backend_io() function for the object wal
"

The proposed doc changes are in the .txt attached (that applies on top of v2).

Regards,

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