On 23/01/2024 01:38, Michael Paquier wrote:
> On Mon, Jan 22, 2024 at 10:47:17PM +0200, Heikki Linnakangas wrote:
>> Michael, it was a pleasure to write this test with the injection points,
>> compared to trying to set up PITR at just the right moment. Thank you! Since
>> this is the first test that uses it, I didn't have any precedence to
>> copy-paste; can you take a look and verify if this is how you imagined the
>> facility to be used?
>
> I was wondering how many weeks it would take for somebody to begin
> playing with that, and here you are 12 hours later..
:-D
> +EXTRA_INSTALL = src/test/modules/injection_points
> [...]
> +#ifdef USE_INJECTION_POINTS
> + if (GinPageIsLeaf(BufferGetPage(stack->buffer)))
> + INJECTION_POINT("gin-leave-leaf-split-incomplete");
> + else
> + INJECTION_POINT("gin-leave-internal-split-incomplete");
> +#endif
>
> Yes, that would be one way of doing it. It makes the code a bit more
> invasive but that would work. Now, you expect this point to be
> conditional depending on the state of the buffer. One thing I was
> thinking of was to extend the APIs in injection_point.c to be able to
> handle a set of arguments as a set of (void*) so as callbacks could
> internally decide what they want to do depending on the running state.
> I didn't have a use for it, but well, you do, so perhaps we could just
> bite the bullet and do it.
>
> That reduces the footprint on the backend code, moving more logic
> behind USE_INJECTION_POINTS in injection_point.c. At the end, you
> should be able to patch the core backend with something like that,
> without using some ifdefs on USE_INJECTION_POINTS:
> INJECTION_POINT_1ARG("gin-leave-leaf-split", (void *) buffer)
>
> And INJECTION_POINT_1ARG() would be just a wrapper around something
> like:
> extern void InjectionPointRun1(const char *name, void *arg1);
I can see that being useful in general. It requires a bespoken callback
function, though. One nice thing about this test was that I didn't need
to write a new C module, only that snippet at the injection point and
some SQL. In more complicated tests a new C module is warranted, but I
think a lot of tests - like this gin test - can be written without it.
Perhaps if the argument was uint64 instead of a pointer, so that the
'injection_point' module could contain ready-made functions that do
things with the argument.
Overall I feel we should not bother with injection point arguments for
now, though. Let's wait until we have a few more tests that would
benefit from that, and then we'll have more information on what the
ideal interface would look like.
--
Heikki Linnakangas
Neon (https://neon.tech)
