Horton

Thread

Commits

  1. aio: Use runtime arguments with injections points in tests

  2. injection_points: Add support and tests for runtime arguments

  3. Add support for runtime arguments in injection points

  1. Support for runtime parameters in injection points, for AIO tests

    Michael Paquier <michael@paquier.xyz> — 2025-04-14T07:46:22Z

    Hi all,
(Andres in CC.)

While reading the AIO code I have noticed that the way it uses
injection points is limited by the fact that we don't have support for
runtime parameters in the existing injection point facility.  The code
is shaped with two set/get functions that set a static parameter that
the injection callback would reuse internally, using
pgaio_inj_io_get(), and pgaio_io_call_inj() and a static
pgaio_inj_cur_handle.  Relying on a static variable for that is not a
good idea, IMO, even if the stack is reset with a TRY/CATCH block on
error in the callback run.

Supporting runtime parameters in injection points is something that I
have mentioned as wanted a couple of times, but I was waiting for an
actual use-case in core before adding support for it, as mentioned
around here:
https://www.postgresql.org/message-id/Z_yN_gaLw-pE2ul-@paquier.xyz

test_aio is bringing one.

We are still in early April, and I'd like to propose a cleanup of the
AIO code on HEAD, even if we are post-feature freeze, to not release
this new code in this state, implying an open item.  Please note that
I'm OK to take responsibility for this patch set at the end, reviews
are welcome.

Anyway, I have spent some time with my mind on that, and finished
with the attached patch set:
- 0001 is the addition of runtime parameters in the backend code.  I
have made the choice of extending the existing INJECTION_POINT() and
INJECTION_POINT_CACHED() instead of introducing new macros.  That's a
matter of taste, perhaps, but increasing the number of these macros
leads to a confusing result.  This one is more invasive, but that's OK
for me.  The code is shaped so as we can rely on
InjectionPointCallback to define the shape of a callback.  It is 
possible to pass down a full structure if one wants, that the callback
is then responsible for translating back.  The AIO code only want an
AIO handle, which is simple.
- 0002 introduces a few updates to the module injection_points, adding
support for runtime parameters and some tests.
- 0003 cleans up the AOI test code, relying on 0001.

The CI is passing.  Thoughts and comments are welcome.
--
Michael

  2. Re: Support for runtime parameters in injection points, for AIO tests

    Andres Freund <andres@anarazel.de> — 2025-04-14T08:05:40Z

    Hi,

On 2025-04-14 16:46:22 +0900, Michael Paquier wrote:
> While reading the AIO code I have noticed that the way it uses
> injection points is limited by the fact that we don't have support for
> runtime parameters in the existing injection point facility.

Yep.


> The code is shaped with two set/get functions that set a static parameter
> that the injection callback would reuse internally, using
> pgaio_inj_io_get(), and pgaio_io_call_inj() and a static
> pgaio_inj_cur_handle.  Relying on a static variable for that is not a good
> idea, IMO, even if the stack is reset with a TRY/CATCH block on error in the
> callback run.

It's not great, I agree.  I was frankly rather surprised to see that injection
points don't support anything to pass parameters, since, without that, you
really can't actually inject different results or anything reliably.


> We are still in early April, and I'd like to propose a cleanup of the
> AIO code on HEAD, even if we are post-feature freeze, to not release
> this new code in this state, implying an open item.  Please note that
> I'm OK to take responsibility for this patch set at the end, reviews
> are welcome.

I'm pretty agnostic about this happening for 18 or not.  If we can do it,
good, but if not, the impact of needing to use static variable supporting
injection points is really rather small.

I'd say that the fact that injection variables are really hard to use in
critical sections, requiring to have attached to the injection point
beforehand, is worse.


> Anyway, I have spent some time with my mind on that, and finished
> with the attached patch set:
> - 0001 is the addition of runtime parameters in the backend code.  I
> have made the choice of extending the existing INJECTION_POINT() and
> INJECTION_POINT_CACHED() instead of introducing new macros.  That's a
> matter of taste, perhaps, but increasing the number of these macros
> leads to a confusing result.  This one is more invasive, but that's OK
> for me.

I can see arguments for going either way on that one.


> The code is shaped so as we can rely on InjectionPointCallback to define the
> shape of a callback.

I don't know what this means though?

Greetings,

Andres Freund

  3. Re: Support for runtime parameters in injection points, for AIO tests

    Michael Paquier <michael@paquier.xyz> — 2025-04-16T22:19:33Z

    On Mon, Apr 14, 2025 at 04:05:40AM -0400, Andres Freund wrote:
> On 2025-04-14 16:46:22 +0900, Michael Paquier wrote:
>> We are still in early April, and I'd like to propose a cleanup of the
>> AIO code on HEAD, even if we are post-feature freeze, to not release
>> this new code in this state, implying an open item.  Please note that
>> I'm OK to take responsibility for this patch set at the end, reviews
>> are welcome.
> 
> I'm pretty agnostic about this happening for 18 or not.  If we can do it,
> good, but if not, the impact of needing to use static variable supporting
> injection points is really rather small.

One thing that I'm afraid of is seeing this pattern grow in the code
even for code in the works, and as injection points are for tests
purposes, I'd like to think that we can be rather flexible with them.
We're still early in the post-freeze phase, so I'd say to just do it
at this point.  Of course, this won't happen without a clear consensus
and if folks are happy with what I've sent.

> I'd say that the fact that injection variables are really hard to use in
> critical sections, requiring to have attached to the injection point
> beforehand, is worse.

I've also looked at providing a new flavor of load_external_function()
that would not need to worry about the allocation, even if it means
relying on MAXPGPATH.  I've never come down to actually do that as
being able to load the callback in the cache beforehand is something I
think we'll require anyway.  So we don't really need this new flavor
API at this level.

>> The code is shaped so as we can rely on InjectionPointCallback to define the
>> shape of a callback.
> 
> I don't know what this means though?

I mean here to have all the callbacks rely on one single typedef.
I've looked some options, like having multiple typedef where some have
zero, one or mote arguments, but found that inelegant.

One thing that Jeff Davis (added in CC) has mentioned me upthread is
that he was looking at runtime arguments for injection points for a
new project of his, where he wanted to do some stack manipulation to
make a couple of corner cases cheap to emulate.  I've asked his
opinion about the interface I've proposed, as well.
--
Michael