Date: Fri, 10 Nov 2023 14:44:25 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
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: <20231110204425.GA1315705@nathanxps13>
References: <ZTiV8tn_MIb_H2rE@paquier.xyz>
 <CAN55FZ1Hf_mWOsvqDGJA8Sf-7YPD1hKcX0gUMJG0sqT=VU12Pg@mail.gmail.com>
 <ZUnuzPimkZCOVEcz@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZUnuzPimkZCOVEcz@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/20231110204425.GA1315705%40nathanxps13>
Precedence: bulk

On Tue, Nov 07, 2023 at 05:01:16PM +0900, Michael Paquier wrote:
> On Mon, Nov 06, 2023 at 10:28:14PM +0300, Nazir Bilal Yavuz wrote:
>> I liked the idea; thanks for working on this!

+1, this seems very useful.

> +#ifdef USE_INJECTION_POINTS
> +#define INJECTION_POINT_RUN(name) InjectionPointRun(name)
> +#else
> +#define INJECTION_POINT_RUN(name) ((void) name)
> +#endif

nitpick: Why is the non-injection-point version "(void) name"?  I see
"(void) true" used elsewhere for this purpose.

> +    <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>

Why do we provide the name to the callback functions?

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 able
to change it easily in the future as needed.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com