Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 20 Jan 2025 21:11:16 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Andres Freund <andres@anarazel.de>,
	Nazir Bilal Yavuz <byavuz81@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: per backend I/O statistics
Message-ID: <Z449ZFNMXMbzrhRX@paquier.xyz>
References: <Z4dyHHxGfGK7bGjd@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z4d_eggsxtBEdJAG@paquier.xyz>
 <66efowskppsns35v5u2m7k4sdnl7yoz5bo64tdjwq7r5lhplrz@y7dme5xwh2r5>
 <Z4mfYkpzm5Fe44lZ@paquier.xyz>
 <Z4nza3pgLpD+rmrE@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z4n01COjAA4WYC6e@paquier.xyz>
 <Z4ovtCrpJCikdMM3@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z4tsCxa0dG6FdA4h@paquier.xyz>
 <Z43ugZPG4oKT3Q_Q@paquier.xyz>
 <Z44vMD/rALy8pfVE@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="/4nRbYgHmZFcycDN"
Content-Disposition: inline
In-Reply-To: <Z44vMD/rALy8pfVE@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: <https://www.postgresql.org/message-id/Z449ZFNMXMbzrhRX%40paquier.xyz>
Precedence: bulk


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

On Mon, Jan 20, 2025 at 11:10:40AM +0000, Bertrand Drouvot wrote:
> I think that it would be better to make the distinction based on "local/s=
tatic"
> vs "dynamic memory" pending stats instead: I did so in v3 attached, using:
>=20
> .flush_dynamic_cb(): flushes pending entries tracked in dynamic memory
> .flush_static_cb(): flushes pending stats from static/global variable

"static" and "dynamic" feel a bit off when used together.  For
"static", the stats are from a static state but they can be pushed to
the dshash, as well, which is in dynamic shared memory.

Hmm.  Thinking more about that, I think that we should leave the
existing "flush_pending_cb" alone.  For the two others, how about
"have_static_pending_cb" and "flush_static_cb" rather than "fixed"?
It seems important to me to show the relationship between these two.

> Not sure about this one, see above. I mean it is currently Ok but once we=
'll
> introduce the WAL part then it will not be correct depending of the flag =
value
> being passed.
>=20
> So, I did put back the previous logic in place (setting to zero only the =
stats
> the flush callback is responsible for) in v3 attached.

Hmm.  I think that I'm OK with what you are doing here with the next
parts in mind, based on this argument.

> I don't think setting have_backendstats to false in pgstat_flush_backend()
> is correct. I mean, it is correct currently but once we'll add the WAL pa=
rt it
> won't necessary be correct if the flags !=3D PGSTAT_BACKEND_FLUSH_ALL. So=
, using a=20
> pg_memory_is_all_zeros check on PendingBackendStats instead in the attach=
ed.

Indeed.  I got this part slightly wrong here.  What you are doing with
an all-zero check looks simpler in the long run.
--
Michael

--/4nRbYgHmZFcycDN
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmeOPWQACgkQnvQgOdby
QH0Opg//Sd7mS2Ck8Vj+7MRtKsVSThF63StgHphoIQFOdE5ui7AeQCEcuZR61gfq
yrQok6AEbVvGyGi0Ljohq+Yvn7tzUD3VIWM9sBbG7qgnWrgTgkL4Z+3WVKQ0AhOv
FL7/KwIxw1lNUoNVrxgiLjwkA9eSOE2YKC9U0FTBrvWF1kVISAyB+6aCl/wabEvx
hHb+ufmn7hzb4RueiOWX8p8jk3qNcoLC5sE0HrAWqxK2FGUzCoSl//R51Z8MtKuM
6XMwnjrzYbCUQb1u2J/nq7F4H6btCCup8ewl+08I/MKnsuDp66OR6WqopwfFfgHJ
8gRbokHxSgC9quyUY0yVHAI9nq5K5eTbX0sMjXH7HHBlNMG3Rf3NGH8DAwhkAvYD
6qbEiI/LvtuUBiR5671okfY0QzMYcvFAJ5xku8YTsQPkRlzxCddJQQYAMkwKbWMi
tSESz6wMg2tWY5qgbSR/3C/hs7iPzrBeu332vfsaKlFIoo1tgAiL5/7eZEzbx4rT
7+dVUcJuQpQ/RYNoTKLy4imzM4uxF9z2ZyS1o2aE0cYxwt/iWNyazhRDlNpd/T/2
He89Q/ID9dhwkxOA3ELFI5LPNCf2gZBNNEaVtUjK+p1b7l/BVDBqRC12CfB5wrdG
mDGT9ifY83HQm85tUarl3BPUnXzPxKkNUcHpRYs391xqNxrRins=
=sKYF
-----END PGP SIGNATURE-----

--/4nRbYgHmZFcycDN--