Обсуждение: local custom injection points do not work as expected

Hi All,
While writing a test for shared buffer resizing work, I noticed that
injection_points_attach() with a custom function does not work as
expected when locally attached. See new isolation test
local_custom_injection.spec in the attached patch for a reproducer. In
that test, even if we call injection_points_set_local() before calling
injection_points_attach() with a custom injection point, calling
injection_points_run() from another session still invokes the
injection callback (but should not). This happens because unlike
injection_points_attach(), injection_points_attach_func() does not
construct and pass an InjectionPointCondition object to
InjectionPointAttach(). injection_points_attach() uses the
private_data argument to pass InjectionPointCondition, whereas
injection_points_attach_func() uses private_data to pass the
user-provided attach-time argument.

I think InjectionPointAttach needs two different arguments: one to
pass the InjectionPointCondition() and other to pass user argument at
the time of attach. Attached is a patch to do it. The patch changes
the signature of InjectionPointAttach() and adds code to save the
parameters passed at attach time in look up tables, caches and then
retrieve those when the injection point is run.

While doing so, I noticed that test_aio code didn't notice the change
in the definition of InjectionPointCallback, which led to test_aio
tests failing. The reason is that the callbacks were not declared
using the InjectionPointCallback typedef. The patch now declares
callbacks as `InjectionPointCallback callback_name;` so compilation
fails if function definitions don't match the expected signature. To
enable this, I changed the typedef from a function pointer (`typedef
void (*InjectionPointCallback)(...)`) to a function type (`typedef
void (InjectionPointCallback)(...)`). Irrespective of what happens to
the rest of the patch, I think this change is worth considering for
commit by itself.

Suggestions on new names of the arguments, structure members are welcome.

I have changed the injection_notice() function to print both the
attach parameter and the run-time argument. This has caused a lot of
expected output changes. I think it's better to print (null) when
either of them is not passed, to be clearer. But I am ok if we don't
want to print an argument when one was not passed.

The patch still needs some work as follows:
o. Once we have two separate arguments, the condition_data argument
can be declared as InjectionPointCondition as well as saved in the
InjectionPoint(Cache)Entry as such. I haven't done that right now
since it's a structure local to injection_points.c. We will need to
export it. If we do that, extensions writing a custom injection point
will be able to handle local injection points inside a custom
injection probe. It's not possible to do that now. This limits use of
custom injection points severely, as I faced while writing a test for
shared buffer resizing.

o. Right now non-custom variant injection_point_attach() only takes
two arguments. We could make it to accept three arguments - the third
being the data to be passed at the attach time - just like a custom
variant of the injection_point_attach() function.

o. We depend upon the first byte of the attach parameter being '\0' to
decide whether the user has passed the attach time argument or not. I
think we need a better way to do it; maybe a flag in the
InjectionPoint(Cache)Entry.

o. Given that there are now two arguments one at run time and one at
attach time, the current last argument in InjectionPointCallback
signature should be changed to run_time_arg or some such.

o. I have not updated injection_points_error() and
injection_points_wait() functions to use the attach-time argument. If
we can agree on something, I will do it. For example, when the attach
parameter is passed and its value is true, the respective function
throws an error or waits, otherwise not. When no attach parameter is
passed, the respective function always waits or throws an error. Or if
both the parameters are passed, the respective function waits or
throws error only when both of them are true. I am open to suggestions
on this.

Before proceeding with it, I wanted to check whether the approach looks good.

-- 
Best Wishes,
Ashutosh Bapat

On Fri, Feb 06, 2026 at 06:22:00PM +0530, Ashutosh Bapat wrote:
> While writing a test for shared buffer resizing work, I noticed that
> injection_points_attach() with a custom function does not work as
> expected when locally attached. See new isolation test
> local_custom_injection.spec in the attached patch for a reproducer. In
> that test, even if we call injection_points_set_local() before calling
> injection_points_attach() with a custom injection point, calling
> injection_points_run() from another session still invokes the
> injection callback (but should not). This happens because unlike
> injection_points_attach(), injection_points_attach_func() does not
> construct and pass an InjectionPointCondition object to
> InjectionPointAttach(). injection_points_attach() uses the
> private_data argument to pass InjectionPointCondition, whereas
> injection_points_attach_func() uses private_data to pass the
> user-provided attach-time argument.

injection_points_attach_func() ignores local conditions on purpose.
It does not make sense to push a local condition into its private data
stored in shared memory *because* it is possible to push down a custom
private_data area with the function call.  It's still a useful wrapper
for other modules who don't want to duplicate an attach() call.

> Before proceeding with it, I wanted to check whether the approach looks good.

A local injection point is a concept proper to the module
injection_points.

In short, I don't really want to expand this API more than what we
already have, sticking its philosophy and the footprint of the backend
code with two key concepts:
1) We need to be able to register some data when attached, which is
what the private_data area stored in shmem is about.  This area can be
defined by the caller of InjectionPointAttach(), with private_data and
private_data_size.  It's a kind of static state, that does not change
for the time a point is defined.
2) We need to be able to pass down to a callback data at runtime.
That's what the *arg argument of InjectionPointRun() and
InjectionPointCached() is about, and what AIO tests rely on.  This is
a kind of dynamic state, that changes depending on what you have on
your stack when a point is run.

If you think about it, one does not really need more than that: it is
already possible to push down data to the callbacks for a static
state, depending on when a point is atached, and a dynamic state,
which is what the argument pointer is about.  That's also enough for
stack manipulations within a callback, something that the AIO test
module does.

The concept introduced in your patch makes the backend API bloated for
no gain: the new arguments are just a way to duplicate what concept 1)
does by pushing a static condition at attach.  This is demonstrated in
the patch by the complexity of using two variables and four arguments
that serve the same purposes.

For your case with the shared buffer resizing, I'd suggest to disable
install_check on it anyway if you manipulate the buffer pool directly
on a cluster, due to its potential cluster-wide effect.  This also
means that local points are moot to have in a SQL test, either
isolation of direct queries in sql/: they would never run
concurrently.  If you really need something that's allowed to run
under installcheck with a SQL or isolation test, I would suggest to
add a test directly in  src/test/modules/injection_points/ with a
dedicated callback.  If you want to do something more complex, like
what AIO does, you can just disable install_check and have one or more
complicated callbacks that do what you want.  I cannot say which one
is more adapted than the other for your case, but I suspect that the
second option is preferable, especially if you want to test various
tricky scenarios in isolation of each other.  injection_points has a
kind of good balance now, with only a minimal set of callbacks, even
if these can mean more footprint in the backend code when it comes to
manipulate the stack (see f1e251be80a0 as a funky case to speed up
tests).  The same balance exists for the backend code APIs.
--
Michael