Обсуждение: Extend injection_points_attach() to accept a user-defined function

Hi,

I would like to propose providing a sql interface to link a
user-defined function to an injection point.
Currently, if a user wants an injection point to invoke a custom
function, they must first define an SQL
function that attaches the injection point to the target/custom
function. This SQL function can then be called
in sql tests to attach to the injection point before running the tests.

The attached patch simplifies this by extending the
injection_points_attach() function to support a new
action type called "func".
The new "func" action enables users to attach a user-defined function
to an injection point.
When "func" is specified as action, the caller must provide:

1. the module name

2. the function name

as additional arguments to injection_points_attach(). These arguments
default to NULL when the
action is not "func".
This approach aims to simplify the process of assigning user-defined
functions to injection points,
eliminating the need to create separate SQL functions to attach each
injection point individually.

Thank you,
Rahila Syed

On Tue, Oct 28, 2025 at 06:11:25PM +0530, Rahila Syed wrote:
> I would like to propose providing a sql interface to link a
> user-defined function to an injection point.
> Currently, if a user wants an injection point to invoke a custom
> function, they must first define an SQL
> function that attaches the injection point to the target/custom
> function. This SQL function can then be called
> in sql tests to attach to the injection point before running the tests.
> The attached patch simplifies this by extending the
> injection_points_attach() function to support a new
> action type called "func".

@@ -354,6 +354,7 @@ injection_points_attach(PG_FUNCTION_ARGS)
+    char       *mod_name;
[...]
@@ -362,6 +363,15 @@ injection_points_attach(PG_FUNCTION_ARGS)
         function = "injection_notice";
     else if (strcmp(action, "wait") == 0)
         function = "injection_wait";
+    else if (strcmp(action, "func") == 0)

How about a simpler injection_points_attach(point_name text, func
text, module text) with a second SQL function, but a different number
of arguments?   Using a new hardcoded action for this purpose is
confusing as your point is to introduce a SQL wrapper on top of
InjectionPointAttach(), and using input arguments that match with the
C function is an attractive option.
--
Michael

Hi,

> How about a simpler injection_points_attach(point_name text, func
> text, module text) with a second SQL function, but a different number
> of arguments?   Using a new hardcoded action for this purpose is
> confusing as your point is to introduce a SQL wrapper on top of
> InjectionPointAttach(), and using input arguments that match with the
> C function is an attractive option.

Thank you for the suggestion.
I agree that having a separate SQL function for this would make the design
easier to understand.

Please find attached a patch that implements this.

Thank you,
Rahila Syed

Hello!

I thought it may help me to implement some kind of notice+wait
required for [1] in order to stabilize the tests.

Is it possible to do something like this in the attached function?

    RAISE NOTICE 'going to wait';
    SELECT injection_points_run(some_point_with_wait"); -- wait called
inside injection point handler

Also, I think it is a good idea to add some tests to injection_points.sql.

Best regards,
Mikhail.

[1]:
https://www.postgresql.org/message-id/flat/CADzfLwWRVj7wDy4Qj3CJTuWy6fvv9TTDBTHsUjC7F1SAN0LpeA%40mail.gmail.com#52c7659bf9c8c5ff484e3b470a4cfb8a

Hi Mihail,

Thank you for looking into this thread.

>
> I thought it may help me to implement some kind of notice+wait
> required for [1] in order to stabilize the tests.
>
> Is it possible to do something like this in the attached function?
>
>     RAISE NOTICE 'going to wait';
>     SELECT injection_points_run(some_point_with_wait"); -- wait called
> inside injection point handler
>

One way to achieve this using the proposed SQL function is to create a
C function in a module like injection_points, which combines injection_notice
and injection_wait. You can then pass this combined function as an argument
to the proposed injection_points_attach() function.

Something as follows:

SELECT injection_points_attach('TestInjectionNoticeFunc',
'injection_points', 'injection_notice_and_wait');

> Also, I think it is a good idea to add some tests to injection_points.sql.
>

PFA a rebased patch that contains the test.
The tests use the newly added SQL function to attach the injection_notice
function to an injection point


Thank you,
Rahila Syed

On Tue, Nov 04, 2025 at 02:17:44PM +0530, Rahila Syed wrote:
> PFA a rebased patch that contains the test.
> The tests use the newly added SQL function to attach the injection_notice
> function to an injection point

+    if (injection_point_local)
+    {
+        condition.type = INJ_CONDITION_PID;
+        condition.pid = MyProcPid;
+    }

Hmm.  Is there a point in registering a condition that's linked to
the shared library injection_points?  The automatic drop is kind of
nice to have, I guess, but it gives the illusion that an attached
callback will not be run.  However, a callback from an entirely
different library *will* run anyway because it cannot look at the
condition registered, or the other library has an equivalent able to
treat a local condition with the same format and same structure as
what's in injection_points.c.

Different idea: we could allow one to pass a bytea that could be given
directly to InjectionPointAttach()?  Without a use-case, I cannot get
much excited about that yet, but if someone has a use for it, why not.

Maybe we should also discard the pgstat_create_inj() call in this
path?  The existing injection_points_detach() would be able to deal
with a point attached with a callback from a different library anyway.
Keeping pgstat_report_inj_fixed() feels OK in this new attach path.
--
Michael

Hi,

Thank you for your review.

>
> +       if (injection_point_local)
> +       {
> +               condition.type = INJ_CONDITION_PID;
> +               condition.pid = MyProcPid;
> +       }
>
> Hmm.  Is there a point in registering a condition that's linked to
> the shared library injection_points?  The automatic drop is kind of
> nice to have, I guess, but it gives the illusion that an attached
> callback will not be run.  However, a callback from an entirely
> different library *will* run anyway because it cannot look at the
> condition registered, or the other library has an equivalent able to
> treat a local condition with the same format and same structure as
> what's in injection_points.c.
>
The intention was to keep the implementation consistent across all
versions of injection_points_attach(). I agree that the conditional
implementation
is only feasible if the library defining the new function also supports the same
structure as injection_points.c. Therefore, I am okay with not adding
this support
for custom functions, since it would not be complete without the new function
being able to read the same structure.

> Different idea: we could allow one to pass a bytea that could be given
> directly to InjectionPointAttach()?  Without a use-case, I cannot get
> much excited about that yet, but if someone has a use for it, why not.
>

This seems helpful for situations where each module needs to provide its
own custom data to the injection point.  This idea is implemented in the
attached patch.

> Maybe we should also discard the pgstat_create_inj() call in this
> path?  The existing injection_points_detach() would be able to deal
> with a point attached with a callback from a different library anyway.
> Keeping pgstat_report_inj_fixed() feels OK in this new attach path.

OK, it makes sense to leave it out of this function for now. Since
pgstat_create_inj() currently only tracks the number of runs, it also
depends on any callback using the appropriate pgstat_report_* API
from the injection_point module. Without this, setting up the stats
infrastructure wouldn't be useful.

PFA the updated and rebased patch.

Thank you,
Rahila Syed

On Fri, Nov 07, 2025 at 05:39:57PM +0530, Rahila Syed wrote:
> OK, it makes sense to leave it out of this function for now. Since
> pgstat_create_inj() currently only tracks the number of runs, it also
> depends on any callback using the appropriate pgstat_report_* API
> from the injection_point module. Without this, setting up the stats
> infrastructure wouldn't be useful.

Yeah.  What you are doing would be enough on simplicity ground.  The
test added is also fine enough, it's safe to run even under an
installcheck.  So LGTM to use a minimal implementation.  Any thoughts
from others?
--
Michael

On Sun, Nov 09, 2025 at 08:35:55AM +0900, Michael Paquier wrote:
> Yeah.  What you are doing would be enough on simplicity ground.  The
> test added is also fine enough, it's safe to run even under an
> installcheck.  So LGTM to use a minimal implementation.

The patch had a one problem other than style.  Contrary to its
existing cousin, the new function is not strict.  Hence, it would
crash if given a NULL value for the point name, the library name or
the function name.

Fixed all that, adjusted a few comments, then applied the result.
--
Michael