Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 16 Jan 2025 09:55:10 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Nazir Bilal Yavuz <byavuz81@gmail.com>
Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org,
	Andres Freund <andres@anarazel.de>
Subject: Re: per backend I/O statistics
Message-ID: <Z4hY7pKjgLP5jrnX@paquier.xyz>
References: <Z2EyxyEJPMMldj1t@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z2FAyFSg-lRGFZio@paquier.xyz>
 <Z2FF6ScWef9yIrJL@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z2JWUaY8AHBs99yp@paquier.xyz>
 <Z2KDy56Ls4nBl1Y8@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z2OfYh3QJ7M5D0i-@paquier.xyz>
 <CAN55FZ0ZQTf-D+5Rup4H6zp+n3E2EuonYuzTiPkp=1GhesZU_A@mail.gmail.com>
 <Z4dyHHxGfGK7bGjd@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z4d_eggsxtBEdJAG@paquier.xyz>
 <CAN55FZ1uOq=FVJObp0bdj-Z8q1ZRNmA-RymPqbMD+p4QaHXP3A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="dJ42aCNBiJKeL8rK"
Content-Disposition: inline
In-Reply-To: <CAN55FZ1uOq=FVJObp0bdj-Z8q1ZRNmA-RymPqbMD+p4QaHXP3A@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z4hY7pKjgLP5jrnX%40paquier.xyz>
Precedence: bulk


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

On Wed, Jan 15, 2025 at 05:20:57PM +0300, Nazir Bilal Yavuz wrote:
> I think allowing only pgStatPendingContext to have
> MemoryContextAllowInCriticalSection() is not enough. We need to allow
> at least pgStatSharedRefContext as well to have
> MemoryContextAllowInCriticalSection() as it can be allocated too.
>=20
> '''
> pgstat_prep_pending_entry() ->
> pgstat_get_entry_ref() ->
> pgstat_get_entry_ref_cached() ->
> MemoryContextAlloc(pgStatSharedRefContext, sizeof(PgStat_EntryRef))
> '''

Yep, I was pretty sure that we have a bit more going on.  Last time I
began digging into the issue I was loading injection_points in
shared_preload_libraries with stats enabled to see how much I could
break, and this was one pattern once I've forced the pending part.  I
didn't get through the whole exercise.

> While doing the initdb, we are restoring stats with the
> pgstat_restore_stats() and we do not expect any pending stats. The
> problem goes like that:
>=20
> '''
> pgstat_restore_stats() ->
> pgstat_read_statsfile() ->
> pgstat_reset_after_failure() ->
> pgstat_drop_all_entries() ->
> pgstat_drop_entry_internal() ->
> We have an assertion there which checks if there is a pending stat entry:
>=20
>     /* should already have released local reference */
>     if (pgStatEntryRefHash)
>         Assert(!pgstat_entry_ref_hash_lookup(pgStatEntryRefHash, shent->k=
ey));
> '''

You mean that this is not something that happens on HEAD, but as an
effect of trying to add WAL stats to pg_stat_io, right?

> I was a bit surprised that Bertrand did not encounter the same problem
> while working on the 'per backend WAL statistics' patch. Then I found
> the reason, it is because this problem happens only for WAL read and
> WAL init IOs which are starting to be tracked in my patch. By saying
> that, I could not decide which thread to write about this problem,
> migrating WAL stats thread or this thread. Since this thread is active
> and other stats may cause the same problem, I decided to write here.
> Please warn me if you think I need to write this to the migrating WAL
> stats thread.

I'd rather treat that on a separate thread (please add me in CC if I'm
not there yet!).  I am OK to discuss any issues related to backend
statistics here if HEAD is impacted based on how the feature is
limited currently now, though.

Another, simpler, idea would be to force more calls
pgstat_prep_backend_pending() where this is problematic, like before
entering a critical section when generating a WAL record.  I am not
sure that it would take care of everything, as you mention; that would
leave some holes depending on the kind of interactions with the
pgstats dshash we do in these paths.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmeIWO4ACgkQnvQgOdby
QH0PwhAAprM9S2GN3fAHzlPKlyZ9UztFSmQLBTX3LlDft7K4947AngSy4ELMPW87
e3JcM9N9efZ7ozHGp5Yjn+m2ZHD5/NwJXfnDy4J9WFzLqqojIHpmmIeFurj4g5PE
EcEI2qwWI2bHMATQNJ9GpsXroiEl/20RgkQu89vkl/upyYsEiYZBPTRwrcpNt5dx
bnH0klvoGUCsUaV81zkN+Lf6AvUngRAetPSrpD3N9BVQMrAZ4IHlMdQ9tLflQ6k3
s3SwIBAn8n0CpkAeaybwRw0+Lr0Z0U+NIt57Mixg/ktdZgYpAZmI2KnJWz2/o7ZP
4oRcwlGKDEiAvsAXm1nXxtiSS0lkgKeAdSZSm22+SN0Bu32S8o68ZA40dn3ms/h/
YpXv2SNnHrMr0WiMASTTpozrTxq7YkuFxTjdi3VSPKl0MV4uT9Lnb9N0h5t9eeWP
O0cv/d8Cqd0d5Vbmkeht4i9xaRpzqpU+/bIPg5s0hE6i2FyFaOPbvxuU3ZE4Wfol
vOoKk8j9LjRPTW42xz301IBzoc8kIyCMdxPjta9/0VosVVYP3qV+a+9mnEEG7Ud+
+N1ajXcJ9K4lbs02JDfniACbMIwWWMNeOiO38WQM5l55Zf93Ta7NLtw0cNfgjbOK
zgZ7DSi4ZCmKopdnohiPRmJcEMjxytVvrwn0z4jC99qI8pCNY54=
=0RKv
-----END PGP SIGNATURE-----

--dJ42aCNBiJKeL8rK--