local custom injection points do not work as expected - Mailing list pgsql-hackers

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