Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 19 Feb 2025 13:32:33 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Remove wal_[sync|write][_time] from pg_stat_wal and
 track_wal_io_timing
Message-ID: <Z7Ve4WLd73ZKi0jd@paquier.xyz>
References: <Z7RkQ0EfYaqqjgz/@ip-10-97-1-34.eu-west-3.compute.internal>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="5yK7D5bjglGSPYzS"
Content-Disposition: inline
In-Reply-To: <Z7RkQ0EfYaqqjgz/@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: <https://www.postgresql.org/message-id/Z7Ve4WLd73ZKi0jd%40paquier.xyz>
Precedence: bulk


--5yK7D5bjglGSPYzS
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Feb 18, 2025 at 10:43:15AM +0000, Bertrand Drouvot wrote:
> =3D=3D=3D Remarks

As mentioned on a previous thread, I am siding with the removal of
these fields.  There are more benefits on terms of granularity of this
data to encourage users to move away from pg_stat_wal and stick to
pg_stat_io.  Reasons are posted here:
https://www.postgresql.org/message-id/Z7PHiAyt6GI0_Tlt@paquier.xyz

> R1. The "bytes" fields differ (as pg_stat_wal.wal_bytes somehow "focus" o=
n the wal
> records size while the pg_stat_io's unit is the wal_block_size) so we kee=
p them
> in both places.

Yes, I don't think we should drop that anyway.  There is no equivalent
mapping and pg_stat_wal.bytes is not I/O.  This is also useful in the
WAL stats for the backends.

> R2. track_wal_io_timing becomes useless once those fields are gone
> from pg_stat_wal
>
> R3. PendingWalStats becomes empty, so removes it.=20

Sounds fine to me.  The simplifications are in pgstat_wal.c, where we
can now depend only on the diffs in the WalUsage, which we rely on for
EXPLAIN, VACUUM/ANALYZE and pgss.  This makes the implementation of
the backend WAL stats much simpler.

> =3D=3D=3D Patch structure
>=20
> PFA 3 sub patches:
>=20
> - 0001: Add details in the pg_stat_io doc about the wal object. That's ba=
sically
> more or less a copy/paste from the pg_stat_wal fields description that wi=
ll be
> removed in 0002. As we already track them in pg_stat_io, it's done in a d=
edicated
> patch.

These additions feel unbalanced with the existing contents, and
overlap with the follow-up paragraph about the tuning that can be
guessed from the contents of pg_stat_io because the new content refers
twice to the section "WAL Configuration".  Perhaps this could be
simpler, with one sentence in the tuning part?  I would propose
something like:
For the WAL object, "fsyncs" and "sync_time" track the sync activity
of WAL files via issue_xlog_fsync().  "writes" and "write_time" track
the write activity of WAL files via XLogWrite().  See <xref
linkend=3D"wal-configuration"/> for more information.

My point is that the "WAL configuration" section already provides all
the details that were in pg_stat_wal removed in 0002 and added in
0001, leading to duplicated contents.

> - 0002: Remove wal_[sync|write][_time] from pg_stat_wal, PendingWalStats =
and=20
> track_wal_io_timing. That does not make sense to split this work in sub-p=
atches
> so that all of this in done in 0002.
> - 0003: remove the pgstat_prepare_io_time() parameter. Now that track_wal=
_io_timing
> is gone there is no need for pgstat_prepare_io_time() to get a parameter.

0002 and 0003 can be grouped in the same commit, IMO.

-   When <xref linkend=3D"guc-track-wal-io-timing"/> is enabled, the total
+   When <xref linkend=3D"guc-track-io-timing"/> is enabled, the total
    amounts of time <function>XLogWrite</function> writes and
    <function>issue_xlog_fsync</function> syncs WAL data to disk are counte=
d as
-   <literal>wal_write_time</literal> and <literal>wal_sync_time</literal> =
in
-   <xref linkend=3D"pg-stat-wal-view"/>, respectively.
+   <literal>write_time</literal> and <literal>sync_time</literal> in
+   <xref linkend=3D"pg-stat-io-view"/> for the wal <literal>object</litera=
l>, respectively.

Okay to update these are part of 0002 that removes the fields, but
there is also an argument about updating that on its own because this
is actually the case on HEAD?  (No need to post an updated patch for
this remark.)

The removal with PgStat_PendingWalStats and the GUC gone looks clean
as presented.
--
Michael

--5yK7D5bjglGSPYzS
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAme1XuEACgkQnvQgOdby
QH29qg/9HQJrARHWc2VgL0VBs8/N7BB+4u7OJXnIpynzbrXwOePDLc6JKqTaS/Aj
+F/OVQIEIK4gW+5qV1ar9eiH0b9RzuKahe5dnJIZqV0nZF7zMkxAXqUbDgvgLOep
Rbqw1870IxWExRvvoJ88wA7hYOBdWSQYqq5qOelIcq5fA4n/iui2OuaOiZ0KvHJW
70Lu3MGTMtbS6D2GLWa108gVKIhBPNkjmpcEFxVqZxuwLnRAc0Ijk1Fg+Tts8G34
xGh6+abtl4c+oKoVdu8pcnAGVuhrszuTUGTDAYyXcARf3HoyV01mBis1siYzzuWz
vH5YDmow8VrsIhG7oAV38+tvI9166q6w6dVGYjwqh4jpiIsY+Uz41EGBqEv6RLZg
bn/2kvzT6cFtyNPuhN40NyfLWnP2y1r8ZnVpsyfEw4cVmrpNwkjHhXii5Wv/eg+5
d3EGPYXii6Ou7XzyRGUkf+DgtDENqK/om/zdeg2PcXvoy2sReYw+Bz4tKru/dbM0
AUbFxWXmHifI9c7iV3vD8awvCczobBDSiIshi3u5/ZS9lOBYs0Bt2/i5s5wdH6hY
B9ihjYxCZKWyxhR37bR3J26P8fLt3mS/dSE96SkcdQvkk/F3kc8WFI2SZkND0oCj
s+CEK0dWUZqMEncehV+syuBKsJIHLHThdqt64oFM90Sy1v8s4f0=
=cHXu
-----END PGP SIGNATURE-----

--5yK7D5bjglGSPYzS--