Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 13 Nov 2023 14:48:13 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Nazir Bilal Yavuz <byavuz81@gmail.com>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Adding facility for injection points (or probe points?) for more
 advanced tests
Message-ID: <ZVG4nf6Is2pnJgU8@paquier.xyz>
References: <ZTiV8tn_MIb_H2rE@paquier.xyz>
 <CAN55FZ1Hf_mWOsvqDGJA8Sf-7YPD1hKcX0gUMJG0sqT=VU12Pg@mail.gmail.com>
 <ZUnuzPimkZCOVEcz@paquier.xyz>
 <20231110204425.GA1315705@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="TzHCXRGPYc1YFMjQ"
Content-Disposition: inline
In-Reply-To: <20231110204425.GA1315705@nathanxps13>
Archived-At: <https://www.postgresql.org/message-id/ZVG4nf6Is2pnJgU8%40paquier.xyz>
Precedence: bulk


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

On Fri, Nov 10, 2023 at 02:44:25PM -0600, Nathan Bossart wrote:
> On Tue, Nov 07, 2023 at 05:01:16PM +0900, Michael Paquier wrote:
>> +#ifdef USE_INJECTION_POINTS
>> +#define INJECTION_POINT_RUN(name) InjectionPointRun(name)
>> +#else
>> +#define INJECTION_POINT_RUN(name) ((void) name)
>> +#endif
>=20
> nitpick: Why is the non-injection-point version "(void) name"?  I see
> "(void) true" used elsewhere for this purpose.

Or (void) 0.

>> +    <para>
>> +     Here is an example of callback for
>> +     <literal>InjectionPointCallback</literal>:
>> +<programlisting>
>> +static void
>> +custom_injection_callback(const char *name)
>> +{
>> +    elog(NOTICE, "%s: executed custom callback", name);
>> +}
>> +</programlisting>
>=20
> Why do we provide the name to the callback functions?

This is for the use of the same callback across multiple points, and
tracking the name of the event happening was making sense to me to
know which code path is being taken when a callback is called.  One
thing that I got in mind as well here is to be able to register custom
wait events based on the name of the callback taken, for example on a=20
condition variable, a latch or a named LWLock.

> Overall, the design looks pretty good to me.  I think it's a good idea to
> keep it simple to start with.  Since this is really only intended for
> special tests that run in special builds, it seems like we ought to be ab=
le
> to change it easily in the future as needed.

Yes, my first idea is to keep the initial design minimal and take the
temperature.  As far as I can see, there seem to not be any strong
objection with this basic design, still I agree that I need to show a
bit more code about its usability.  I have some SQL and recovery cases
where this is handy and these have piled over time, including at least
two/three of them with more basic APIs in the test module may make
sense in the initial batch of what I am proposing here.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmVRuJ0ACgkQnvQgOdby
QH3s1A/8CJ7XPGhzCMnl6w/AyYt7Vlt7Iat6IvVDG6FT5rvHTmD33MK/eI/z6B0Z
zSvqkMfM0D3t2YrDMxl5eGWLJsgnqyFzM5Ythbm07RjTX1NAbY0OIFPQyI8fv039
eRum57oGYHpHbDJ8dK2PJId4NcEJhoBYviuuKNfnYlJL7yF4k3zR/kOsDQlHV72U
e2v7+ap7OtDVdGHkknX+cASAM03eGOz+poQgYnUTRs0aJZoHQRDQ2Ir8kL+3sYED
j04rknPjBt6d6Z0oIu0+WQTz5s3hLipHW8LqTig7mye6EbzYL9089DQ+e/x7GUb2
jL/4kTUYRBEcaTEGPquOUMUgs//IGEqQGnZpR/hTAb9KpHmaBUZViFIJh/e7AqOg
GvBPU2Wvu3Hdv7pTseY1bEJsE1QeDKdbkzP7IzauUoP5yhaDX+3OEgAS3UKRFnyM
LVCP2fjggYsET3LUXVxwa59/fEmDux6MtH6tZcxLuySE28YSKS9fHwxufxbJJ5aj
07anbCXOL3PEtvEZS3u2HG/FdqeKVqTotgYTIWXi3nVFX3CG7KOf2xPUhqOG29as
R+I0YjyCX1lNpLdYpV4BnfUTF5u2vE6Q4AvOYypw9fvIK1Tj5YltboF+FwQkakJm
dn1ChPZvGsndjLi/c5bEumn25p+odI3571rPsayzjENFJIqg1P4=
=ebQ6
-----END PGP SIGNATURE-----

--TzHCXRGPYc1YFMjQ--