Re: Adding facility for injection points (or probe points?) for more advanced tests

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