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
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6186648ccaf..e55700f35b8 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8378,9 +8378,11 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH
csv;
displayed in <link linkend="monitoring-pg-stat-database-view">
<structname>pg_stat_database</structname></link>,
<link linkend="monitoring-pg-stat-io-view">
- <structname>pg_stat_io</structname></link>, in the output of the
+ <structname>pg_stat_io</structname></link> (if
<varname>object</varname>
+ is not <literal>wal</literal>), in the output of the
<link linkend="pg-stat-get-backend-io">
- <function>pg_stat_get_backend_io()</function></link> function, in the
+ <function>pg_stat_get_backend_io()</function></link> function (if
+ <varname>object</varname> is not <literal>wal</literal>), in the
output of <xref linkend="sql-explain"/> when the
<literal>BUFFERS</literal>
option is used, in the output of <xref linkend="sql-vacuum"/> when
the <literal>VERBOSE</literal> option is used, by autovacuum
@@ -8411,7 +8413,8 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH
csv;
<structname>pg_stat_io</structname></link> for the
<varname>object</varname> <literal>wal</literal> and in the output of
the <link linkend="pg-stat-get-backend-io">
- <function>pg_stat_get_backend_io()</function></link> function.
+ <function>pg_stat_get_backend_io()</function></link> function for the
+ <varname>object</varname> <literal>wal</literal>.
Only superusers and users with the appropriate <literal>SET</literal>
privilege can change this setting.
</para>
