Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit

On 22/01/2024 09:59, feichanghong wrote:
> In the function ginFindLeafPage, if we encounter a page marked with
> GIN_INCOMPLETE_SPLIT, we call ginFinishSplit to finish the incomplete 
> split and
> remove the GIN_INCOMPLETE_SPLIT flag from that page. ginFinishSplit requires
> that "On entry, stack->buffer is exclusively locked," as explained in its
> comments.
> 
> The function ginFindLeafPage determines the type of lock held by 
> ginTraverseLock:
> leaf pages hold GIN_EXCLUSIVE, and internal page hold GIN_SHARE. If 
> ginFindLeafPage
> traverses to an internal page with an incomplete split, it will only hold a
> GIN_SHARE lock, which does not meet the requirements of ginFinishSplit. If a
> crash occurs when an internal page is split, but no downlink is inserted 
> in the
> parent page, this problem may occur.
> 
> I injected the following code into the ginbtree.c file to trigger a crash
> during the splitting of an internal page:
> ```
> --- a/src/backend/access/gin/ginbtree.c
> +++ b/src/backend/access/gin/ginbtree.c
> @@ -621,6 +621,12 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
>                   PageSetLSN(BufferGetPage(lbuffer), recptr);
>               if (BufferIsValid(childbuf))
>                   PageSetLSN(childpage, recptr);
> +
> +            if (stack->parent != NULL && !GinPageIsLeaf(newlpage))
> +            {
> +                XLogFlush(recptr);
> +                elog(PANIC, "internal page split for block: %u", 
> stack->blkno);
> +            }
>           }
>           END_CRIT_SECTION();
> ```
> 
> With the modifications above, the problem can be reproduced with the 
> following
> steps:
> ```
> alter system set autovacuum to off;
> alter system set gin_pending_list_limit to 64;
> select pg_reload_conf();
> create table t (a text);
> create extension btree_gin;
> create index on t USING gin (a);
> -- PANIC crash
> insert into t select generate_series(1, 100000);
> 
> -- Assert fail
> insert into t select 1;
> ```
> 
> I reviewed all places where ginFinishSplit is called, and only in two 
> instances
> in ginFindLeafPage might it be possible to not hold an exclusive lock on
> stack->buffer.
> 
> The patch I provided in the attachment has been verified to fix the issue
> mentioned above.
> However, it may not be perfect: passing GIN_EXCLUSIVE to ginStepRight might
> affect performance. Do we need to add a parameter to ginStepRight, and 
> within
> the function ginStepRight, elevate the lock level for an incomplete 
> split page?
> 
> Looking forward to suggestions from developers!

Thanks, I'll look into this. The fix seems fine at a quick glance, but 
I'll think about the performance aspect a bit more.

I'm thinking of adding tests for this using the new fault-injection 
facility that Michael just merged in commit d86d20f0ba. For GIN, but 
also B-tree which has a similar incomplete-split mechanism.

Another way to create a scenario with incomplete splits, which doesn't 
involve any crashes or errors, would be to perform PITR to just between 
the insert and the finish-split records. But the fault-injection seems 
easier.

-- 
Heikki Linnakangas
Neon (https://neon.tech)