Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 20 Sep 2024 13:26:49 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: 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: <Zuz5iQ4AjcuOMx_w@paquier.xyz>
References: <Zta482TuN7ri1Bis@ip-10-97-1-34.eu-west-3.compute.internal>
 <202409051303.qumelxccgzzn@alvherre.pgsql>
 <Ztm811fM73GFt4z/@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZtsZtaRza9bFFeF8@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAN55FZ0mnVe4rra2WXca=j+7fabVRY8uhRFPXFmkdRW9J4dKbQ@mail.gmail.com>
 <ZuRjxA+ysI4mpTSq@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAN55FZ0_0Fj6No-axtDF-jrPQKh6a-qLoU73mpSAG417D+A-NA@mail.gmail.com>
 <Zul++79EOPsFB+Gd@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAN55FZ0xEC6K=EqTwboHJtzZnENDoEwvWOerKkHbZwFT-cU6XQ@mail.gmail.com>
 <ZumKktat4RtHmxL/@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="J0oayVaz+4AUiI8/"
Content-Disposition: inline
In-Reply-To: <ZumKktat4RtHmxL/@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: <https://www.postgresql.org/message-id/Zuz5iQ4AjcuOMx_w%40paquier.xyz>
Precedence: bulk


--J0oayVaz+4AUiI8/
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Sep 17, 2024 at 01:56:34PM +0000, Bertrand Drouvot wrote:
> No problem at all! (I re-explained because I'm not always 100% sure that my
> explanations are crystal clear ;-) )

We've discussed a bit this patch offline, but after studying the patch
I doubt that this part is a good idea:

+	/* has to be at the end due to FLEXIBLE_ARRAY_MEMBER */
+	PgStatShared_IO io;
 } PgStat_ShmemControl;

We are going to be in trouble if we introduce a second member in this
routine that has a FLEXIBLE_ARRAY_MEMBER, because PgStat_ShmemControl
relies on the fact that all its members after deterministic offset
positions in this structure.  So this lacks flexibility.  This choice
is caused by the fact that we don't exactly know the number of
backends because that's controlled by the PGC_POSTMASTER GUC
max_connections so the size of the structure would be undefined.

There is a parallel with replication slot statistics here, where we
save the replication slot data in the dshash based on their index
number in shmem.  Hence, wouldn't it be better to do like replication
slot stats, where we use the dshash and a key based on the procnum of
each backend or auxiliary process (ProcNumber in procnumber.h)?  If at
restart max_connections is lower than what was previously used, we
could discard entries that would not fit anymore into the charts.
This is probably not something that happens often, so I'm not really
worried about forcing the removal of these stats depending on how the
upper-bound of ProcNumber evolves.

So, using a new kind of ID and making this kind variable-numbered may
ease the implementation quite a bit, while avoiding any extensibility
issues with the shmem portion of the patch if these are
fixed-numbered.  The reporting of these stats comes down to having a
parallel with pgstat_count_io_op_time(), but to make sure that the
stats are split by connection slot number rather than the current
split of pg_stat_io.  All its callers are in localbuf.c, bufmgr.c and
md.c, living with some duplication in the code paths to gather the
stats may be OK.

pg_stat_get_my_io() is based on a O(n^3).  IOOBJECT_NUM_TYPES is
fortunately low, still that's annoying.

This would rely on the fact that we would use the ProcNumber for the
dshash key, and this information is not provided in pg_stat_activity.
Perhaps we should add this information in pg_stat_activity so as it
would be easily possible to do joins with a SQL function that returns
a SRF with all the stats associated with a given connection slot
(auxiliary or backend process)?  That would be a separate patch.
Perhaps that's even something that has popped up for the work with
threading (did not follow this part closely, TBH)?

The active PIDs of the live sessions are not stored in the active
stats, why not?  Perhaps that's useless anyway if we expose the
ProcNumbers in pg_stat_activity and make the stats available with a
single function taking in input a ProcNumber.  Just mentioning an
option to consider.
--
Michael

--J0oayVaz+4AUiI8/
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmbs+YkACgkQnvQgOdby
QH1OsQ/+LeD3zCr+61dxG5vrEYzJvtjyRE1vzqSlyfMWi0nAd1a6MeQzbUBoIBM8
v7ekyMwkitljoZ2zIwmdquhKOxrsucsC1bqBxTFoc/NQ2oak0XoNkzR/G+dnlBmA
msDAk01SSxEfagPd1NfTwG1RWZhHZ3etH9D2MEttngJx3sICuxZLuSSFoEBmhWaz
5BO6qU4vzD9p5fWihJQZZ6u19txHmzFei9rkcHsGT/C3PYct50s2wW8qNKpph62g
45SX6You4+NKJWO+h6tHOx4nAWOcCYbGSaetHwrEhnYbw0WGiedN0Fh7fqsySORy
3sWqzcdR96O65vUZ5jSzHKx9BZsMpSiYAQ8CSXsTGPeTIIHpXSsbjf6HT7iJOitT
UHG7tUYUYUapa3CtkbNx1mpMB36714QRQq6GuOW2atseR/cwYaM44w2xPlgWR5ye
wi0lW4cXr0FXCT/otJ4PbBVtFyuL9se+zwdnJ1FXiAjTLdVoHVjea7KMl6C46+Vl
bJ0tJwBxKUxKhQEc9QVWNRUVzk8U+efEaFawg79LlL0xEJvyHrWzQfIiJMutjd+7
Ha77vVMJnmRtZlrYPJHQzkeFjBVTT+uj7OGBnO5ct3330tqcFSL6piN+3Gmu+hYb
W8khgAOodB8J8pHk+Jd9qiSxqrHkqgK6GRWOYJSSBSzWYHWdnCY=
=zkWh
-----END PGP SIGNATURE-----

--J0oayVaz+4AUiI8/--