Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit

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)