Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 22 Aug 2024 09:29:30 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: "Andrey M. Borodin" <x4mmm@yandex-team.ru>,
	vignesh C <vignesh21@gmail.com>,
	Andrew Borodin <amborodin86@gmail.com>, i.lazarev@postgrespro.ru,
	Yura Sokolov <y.sokolov@postgrespro.ru>,
	Andres Freund <andres@anarazel.de>,
	Thomas Munro <thomas.munro@gmail.com>,
	Gilles Darold <gilles@darold.net>,
	Alexander Korotkov <aekorotkov@gmail.com>,
	Anastasia Lubennikova <a.lubennikova@postgrespro.ru>,
	Daniel Gustafsson <daniel@yesql.se>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers mailing list <pgsql-hackers@postgresql.org>
Subject: Re: MultiXact\SLRU buffers configuration
Message-ID: <ZsaGaqKV7ghWhG57@paquier.xyz>
References: <ZsWH4HgIitscS6RE@paquier.xyz>
 <202408211755.j5v7w2ez46no@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="rn+mzuTsYi+bx9au"
Content-Disposition: inline
In-Reply-To: <202408211755.j5v7w2ez46no@alvherre.pgsql>
Archived-At: <https://www.postgresql.org/message-id/ZsaGaqKV7ghWhG57%40paquier.xyz>
Precedence: bulk


--rn+mzuTsYi+bx9au
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Aug 21, 2024 at 01:55:06PM -0400, Alvaro Herrera wrote:
> I find it's strange that the information that stats cannot be used with
> injection points that have dependency on critical sections (?), is only
> in the commit message and not in the code.

A comment close to where inj_stats_enabled is declared in
injection_points.c may be adapted for that, say:
"This GUC is useful to control if statistics should be enabled or not
during a test with injection points, like for example if a test relies
on a callback run in a critical section where no allocation should
happen."

> Also, maybe it'd make sense for stats to be globally enabled, and that
> only the tests that require it would disable them?  (It's probably easy
> enough to have a value "injection_points.stats=3Dauto" which means, if the
> module is loaded in shared_preload_libraries them set stats on,
> otherwise turn them off.)

I'm not sure that we need to get down to that until somebody has a
case where they want to rely on stats of injection points for their
stuff.  At this stage, I only want the stats to be enabled to provide
automated checks for the custom pgstats APIs, so disabling it by
default and enabling it only in the stats test of the module
injection_points sounds kind of enough to me for now.  The module
could always be tweaked to do that in the future, if there's a case.

> TBH I don't understand why the issue that
> stats require shared_preload_libraries only comes up now ...

Because there was no need to, simply.  It is the first test that
relies on a critical section, and we need allocations if we want to
use a wait condition.=20

> Maybe another approach is to say that if an injection point is loaded via
> _LOAD() rather than the normal way, then stats are disabled for that one
> rather than globally?

One trick would be to force the GUC to be false for the duration of
the callback based on a check of CritSectionCount, a second one would
be to just skip the stats if are under CritSectionCount.  A third
option, that I find actually interesting, would be to call
MemoryContextAllowInCriticalSection in some strategic code paths of
the test module injection_points because we're OK to live with this
restriction in the module.

> Or give the _LOAD() macro a boolean argument to
> indicate whether to collect stats for that injection point or not.

Sticking some knowledge about the stats in the backend part of
injection points does not sound like a good idea to me.

> Lastly, it's not clear to me what does it mean that the test has a
> "dependency" on a critical section.  Do you mean to say that the
> injection point runs inside a critical section?

Yes.

> No issues with this, feel free to go ahead.

Cool, thanks.
--
Michael

--rn+mzuTsYi+bx9au
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmbGhmoACgkQnvQgOdby
QH36Yw/+Ke5fHse9dqZ4FrMj2/1CPV3X9smQXihdx8cXfkA70qHEYhbo2JOv44+D
ol0uWi85Pm7yDMRnmFq5j3gwjJzXZO1wq6kn1s6g36EtKSpLB7FYhVnFZBSsXV3W
vS2bckIb4gG/V2GZumSUPdi6TXKTN+zjKQOsAVCoaxbwkcZVc3mTgPSch0i/QbZ+
UlbJBvsnTXbIXMrBMSWk16yfk7Saj9kYHTcHdRaOBSUsl0fBAgyeXp7BXpIj96zo
NtetvIFzJdfTZW1iKcl4ouPONtJCoSCI+iwM5RucUbvaKmyvAGJME2n+owJ497lq
3TPzVCfvI7lhILb8Cw4zSH/65s7tKp0uvGav+IHCUfkbj/Gfb2vNJd/yi4o2P2+J
VkTplB7X67825K+AuHI+wR7wU1xZtxUaYGxDYFk+KtlIZlRmDop+wwh+quJ4wS0J
tZ9ZD6/RX4JL2/hEXmqa8sq7ONsKPV08icSt7jQkoLYYcIOWnvAYsGmfk14Wk4NR
0BKYoz06Kt3FqG/gPnmxr0mDdq2xWjT0mqHEZ77YNHlb2ib2KGmIYg01FvFsDSG7
kIBqLxSCdhKVsUb7n5jKngo+KpROnVdA9A/q8L/Rd+ruJTUHS/sMR6QjJiRa0BTG
qQO9ngfvqOTgVZopGrflSBP9Z5cwx6viFrAHaQNyqkguhvhbn/w=
=bQzm
-----END PGP SIGNATURE-----

--rn+mzuTsYi+bx9au--