Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 19 Nov 2024 10:47:53 +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: <ZzvuScIJW9kwnuyE@paquier.xyz>
References: <ZyMRJIbUpNPoCXUe@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZyibjiLgoLx+kS4n@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZypXy3fH/EyALaTR@ip-10-97-1-34.eu-west-3.compute.internal>
 <Zyqsm6ilLLADAGw2@paquier.xyz>
 <Zyt0Rh6Uwvf+mlA1@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZywO84-2K8fxEIx8@paquier.xyz>
 <ZyzrrFNvXC2YvcNE@ip-10-97-1-34.eu-west-3.compute.internal>
 <Zy4bmvgHqGjcK1pI@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZzWZV9LdyZ9aFSWs@paquier.xyz>
 <ZzX7Y5i56UrOtlRC@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="NFyM3Idg+aKc5jcF"
Content-Disposition: inline
In-Reply-To: <ZzX7Y5i56UrOtlRC@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: <https://www.postgresql.org/message-id/ZzvuScIJW9kwnuyE%40paquier.xyz>
Precedence: bulk


--NFyM3Idg+aKc5jcF
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Nov 14, 2024 at 01:30:11PM +0000, Bertrand Drouvot wrote:
> - change the arguments to false in the pgstat_drop_entry_internal() call =
in=20
> pgstat_drop_all_entries()
> - start the engine
> - kill -9 postgres
> - restart the engine
>=20
> You'll see the assert failing due to the startup process. I don't think i=
t is
> doing something wrong though, just populating its backend stats. Do you h=
ave
> another view on it?

It feels sad that we have to plug in the internals at this level for
this particular case.  Perhaps there is something to do with more
callbacks.  Or perhaps there is just no point in tracking the stats of
auxiliary processes because we already have this data in the existing
pg_stat_io already?

> How would that work for someone doing "select * from pg_stat_get_backend_=
io(<pid>)"?
>=20
> Do you mean copy/paste part of the code from pg_stat_get_activity() into
> pg_stat_get_backend_io() to get the backend type? That sounds like an ide=
a,
> I'll have a look at it.

I was under the impression that we should keep PgStat_Backend for the
reset_timestamp part, but drop BackendType as it can be guessed from
pg_stat_activity itself.  For example a LATERAL to grab the current
live stats of these backends.

>> +typedef struct PgStat_PendingIO
>>=20
>> Perhaps this part should use a separate structure named
>> "BackendPendingIO"?  The definition of the structure has to be in
>> pgstat.h as this is the pending_size of the new stats kind.  It looks
>> like it would be cleaner to keep PgStat_PendingIO local to
>> pgstat_io.c, and define PgStat_PendingIO based on
>> PgStat_BackendPendingIO?
>=20
> I see what you meean, what about simply "PgStat_BackendPending" in pgstat=
=2Eh?

That should be OK.

>> Structurally, it may be cleaner to keep all the callbacks and the
>> backend-I/O specific logic into a separate file, perhaps
>> pgstat_io_backend.c or pgstat_backend_io?
>=20
> Yeah, I was wondering the same. What about pgstat_backend.c (that would c=
ontain
> only one "section" dedicated to IO currently)?

pgstat_backend.c looks good to me.  Could there be other stats than
just IO, actually?  Perhaps not, just asking..

> - if the idea is to 1. record checkpointer stats, 2. do stuff in the back=
end and
> 3. check again the checkpointer stats then I don't think setting stats_fe=
tch_consistency
> to snapshot is needed at all. In fact it's quite the opposite as 1. and 3=
=2E would
> report the same values with stats_fetch_consistency set to 'snapshot' (if=
 done=20
> in the same transaction).

Hmm.  Not sure.  It looks like you're right to treat this in a
separate patch as it would exist for different reasons than the
original proposal.
--
Michael

--NFyM3Idg+aKc5jcF
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmc77kkACgkQnvQgOdby
QH3J9Q/+NjMdHXJxHrrsDE6c9hjrccEBuELdnnqrvNq5tz4UMOWcN+JMOE+DHLYB
oDYjnoybYoySBMpIVyG+IPnl46yjRYxInmKRWbxvlMo3/hgafx9yLjnm3KzlISGO
t7+fM159D2ooJsrvwCqVhU3fK8OnccX7vj51LDXVchnTbGkuJKc9U6NKA4dM18iD
UkEJdwMt6ahKYfwZSt9Q3yT3G5UO/spWRxuCJrNNAJM5LH1zRzSkMcl/pMKH5hJ6
WAoX1iLyi8k1v0EzDw7USrKCFnU6rKOtJc5sJSXIlxH6KUKNkJELfiqswTmuKyeV
uKafVZAcsUxttLFUiVCvDc+/idJWwyfiqrd2mFRImqjPPsmKMoUnKy8NnaTFrNnA
HONrqxFPUM5crxlpXxbWPZZsnHUX53KRADX6sxs/Oqmew2buDeOuHDE8QwAmmlm2
ZliRhdp4cO0xjC+fly/ItrV5vr8hzlIMuimucsGvcIcYSwq21lXaNcgJujR1nzAz
LDjKi5BkkBaOd6Lh+CAZY9V7i5GOz2PG1X3SVgneJXy3E0KYV2J5oGR20cCYc0Ed
OBq50WAMsVHs8nsMjecWVLfRKcdQTWwo7NxfnpO1S50DEMb7nmAxkiH5K/O47uEe
HC7Wz85tHqNtkSImFvj58MAg8JUrdbkdrfQzS/WNhO7Hwn8+RB4=
=GALY
-----END PGP SIGNATURE-----

--NFyM3Idg+aKc5jcF--