Message-ID: <f5eb802a-e98d-438e-9fa8-1694f767e5ea@iki.fi>
Date: Tue, 23 Jan 2024 09:12:25 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when
 ginFinishSplit
Content-Language: en-US
To: Michael Paquier <michael@paquier.xyz>
Cc: feichanghong <feichanghong@qq.com>,
 pgsql-bugs <pgsql-bugs@lists.postgresql.org>
References: <tencent_A3CE810F59132D8E230475A5F0F7A08C8307@qq.com>
 <053bc845-3cb5-445e-8ea4-00f6c678ee25@iki.fi>
 <tencent_0F9D2F03B4F3E2C3DE4483E31B4147C9F605@qq.com>
 <d8f0b068-0e6e-4b2c-8932-62507eb7e1c6@iki.fi>
 <tencent_BEFB72867BA7CF879B492487284296D2F606@qq.com>
 <f8fa079c-c88c-4a4f-9c36-3854d12ddf70@iki.fi> <Za78YVcqNA8nAnn-@paquier.xyz>
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <Za78YVcqNA8nAnn-@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/f5eb802a-e98d-438e-9fa8-1694f767e5ea%40iki.fi>
Precedence: bulk

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)