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)