From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Remove wal_[sync|write][_time] from pg_stat_wal and track_wal_io_timing |
Date: | 2025-02-25 05:20:48 |
Message-ID: | Z71TMKaNzG-mjMjc@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
If you take that into account, XLogWrite(), XLogFileInitInternal(),
issue_xlog_fsync() should trigger clock calculations only for
track_wal_io_timing. The write and fsyncs are the hot ones in
standalone servers.
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.
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.
pgstat_count_backend_io_op_time() would have triggered an assertion
failure, it needs to be adjusted.
With more tweaks applied to the docs, the attached is my result.
--
Michael
Attachment | Content-Type | Size |
---|---|---|
v2-0001-re-introduce-track_wal_io_timing.patch | text/x-diff | 17.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2025-02-25 05:31:56 | Re: pg_copy_logical_replication_slot doesn't copy the failover property |
Previous Message | Kyotaro Horiguchi | 2025-02-25 05:09:53 | Fix untranslatable split message |