Re: Adding facility for injection points (or probe points?) for more advanced tests - Mailing list pgsql-hackers

On Tue, Nov 14, 2023 at 02:11:50PM +0100, Alvaro Herrera wrote:
> Good stuff here, I also have a bunch of bugfix commits that ended up not
> having a test because of the need for a debugger or other interaction,
> so let's move forward.
>
> I think the docs (and the macro/function naming) describe things
> backwards.  In my mind, it is INJECTION_POINT_RUN() that creates the
> injection point; then InjectionPointCreate() attaches something to it.
> So I would rename the macro to just INJECTION_POINT() and the function
> to InjectionPointAttach().  This way you're saying "attach function FN
> from library L to the injection point P"; where P is an entity that is
> being created by the INJECTION_POINT() call in the code.

Okay.  I am not strongly attached to the terms used by the patch.  The
first WIP I wrote used completely different terms.

> You named the hash table InjectionPointHashByName, which seems weird.
> Is there any *other* way to locate an injection point that is not by
> name?

I am not sure what you mean here.  Names are kind of the most
portable and simplest thing I could think of.  Is there something else
you have in mind that would allow a mapping between a code path and
what should be run?  Perhaps that's useful in some cases, but you were
also thinking about an in-core API where it is possible to retrieve a
list of callbacks based on a library name and/or a function name?  I
didn't see a use for it, but why not.

> In this patch, injection points are instance-wide (because the hash
> table is in shmem).  As soon as you install a callback to one point,
> that callback will be fired in every session.  Maybe for some tests this
> is OK (and in particular your TAP tests have them attached in one
> ->safe_psql call and then they hit a completely different session, which
> wouldn't work if the attachments were process-local), but maybe one
> would want them limited to some specific process.  Maybe give an
> optional PID so that if any other process hits that injection point,
> nothing happens?

Yes, still not something that's required in the core APIs or an
initial batch.  This is something I've seen used and a central place
where the callbacks are registered allows that because the callback is
triggered based on a global state like a MyProcPid or a getpid(), so
it is possible to pass a condition to a callback when it is created
(or attached per your wording), with the condition maintained in a
shmem area that can be part of an extension module that defines the
callbacks (in test_injection_points).  One trick sometimes is to know
the PID beforehand, which may need a second wait point (for example)
to make a test deterministic so as a test script has the time to get
the PID of a running session (bgworkers included) before the process
has time to do anything critical for the scenario tested.

An extra thing is that this design can be extended so as it could be
possible to pass down to the callback execution a private pointer of
data, though that's bound to the code path running the injection
point (not in the initial patch).  Then it's up to the callback to
decide if it needs to do something or not (say, I don't want to run
this callback except if I am manipulating page N in an access method,
etc.).  The conditional complexity is pushed to the injection
callbacks, not the core routines in charge is finding a callback or
attaching/creating one.  I am not sure that it is a good idea to
enforce a specific conditional logic in the backend core code.
--
Michael