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

On Thu, Jan 11, 2024 at 9:42 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> >> Yeah, that's an intended design choice to keep the code simpler and
> >> faster as there is no need to track the library and function names in
> >> the local caches or implement something similar to invalidation
> >> messages for this facility because it would impact performance anyway
> >> in the call paths. In short, just don't do that, or use two distinct
> >> points.
> >
> > In practice the InjectionPointDetach() and InjectionPointAttach()
> > calls may not be close by and the user may not be able to figure out
> > why the injection points are behaviour weird. It may impact
> > performance but unexpected behaviour should be avoided, IMO.
> >
> > If nothing else this should be documented.
>
> In all the infrastructures I've looked at, folks did not really care
> about having an invalidation for the callbacks loaded. Still I'm OK
> to add something in the documentation about that, say among the lines
> of an extra sentence like:
> "The callbacks loaded by a process are cached within each process.
> There is no invalidation facility for the callbacks attached to
> injection points, hence updating a callback for an injection point
> requires a restart of the process to release its cache and the
> previous callbacks attached to it."

It doesn't behave exactly like that either. If the INJECTION_POINT is
run after detach (but before Attach), the local cache will be updated.
A subsequent attach and INJECTION_POINT call would fetch the new
callback.

>
> > I am ok with not populating the cache but checking with just
> > load_external_function(). This is again another ease of use scenario
> > where a silly mistake by user is caught earlier making user's life
> > easier. That at least should be the goal of the first cut.
>
> I don't really aim for complicated here, just useful.

It isn't complicated. Such simple error check improve user's
confidence on the feature and better be part of the 1st cut.

>
> > With v6 I could run the test when built with enable_injection_point
> > false. I just ran make check in that folder. Didn't test meson build.
>
> The CI has been failing because 041_invalid_checkpoint_after_promote
> was loading Time::HiRes::nanosleep and Windows does not support it.

Some miscommunication here. The SQL test under injection_point module
can be run in a build without injection_point and it fails. I think
it's better to have an alternate output for the same or prohibit the
test running itself.

--
Best Wishes,
Ashutosh Bapat