Re: BUG #18396: Assert in gistFindCorrectParent() fails on inserting large tuples into gist index

Heikki Linnakangas <hlinnaka@iki.fi> 于2024年3月19日周二 22:26写道：

> On 19/03/2024 14:07, Tender Wang wrote:
> > Thanks for your report. I can reproduce this issue.
> > I try to delete the Assert, no coredump anymore.
> > I need some time to learn GiST to find the root cause.
> At first glance, I think the assertion is too strict. In
> gistFindCorrectParent(), if we walk right, we update the parent's block
> number and reset its memorized downlinkoffnum to InvalidOffsetNumber. If
> we later call gistFindCorrectParent() again with the same stack, because
> the parent also needs to be split, we hit that assertion. But it's OK in
> that case, we don't know the downlink because we had moved right.
> Attached patch relaxes that.
>
>  I tested the code with attached patch many times. No failure anymore.
+1

> But now I'm having some second thoughts. gistFindCorrectParent() looks
> like this:
>
> >       /*
> >        * Scan the page to re-find the downlink. If the page was split,
> it might
> >        * have moved to a different page, so follow the right links until
> we find
> >        * it.
> >        */
> >       while (true)
> >       {
> >               OffsetNumber i;
> >
> >               maxoff = PageGetMaxOffsetNumber(parent->page);
> >               for (i = FirstOffsetNumber; i <= maxoff; i =
> OffsetNumberNext(i))
> >               {
> >                       iid = PageGetItemId(parent->page, i);
> >                       idxtuple = (IndexTuple) PageGetItem(parent->page,
> iid);
> >                       if (ItemPointerGetBlockNumber(&(idxtuple->t_tid))
> == child->blkno)
> >                       {
> >                               /* yes!!, found */
> >                               child->downlinkoffnum = i;
> >                               return;
> >                       }
> >               }
> >
> >               parent->blkno = GistPageGetOpaque(parent->page)->rightlink;
> >               parent->downlinkoffnum = InvalidOffsetNumber;
> >               UnlockReleaseBuffer(parent->buffer);
> >               if (parent->blkno == InvalidBlockNumber)
> >               {
> >                       /*
> >                        * End of chain and still didn't find parent. It's
> a very-very
> >                        * rare situation when the root was split.
> >                        */
> >                       break;
> >               }
> >               parent->buffer = ReadBuffer(r, parent->blkno);
> >               LockBuffer(parent->buffer, GIST_EXCLUSIVE);
> >               gistcheckpage(r, parent->buffer);
> >               parent->page = (Page) BufferGetPage(parent->buffer);
> >       }
>
> When we step right and update parent->blkno, shouldn't we also update
> parent->lsn? Otherwise, we might fail to detect a concurrent page split
> with the LSN-NSN interlock checks. Or maybe it's correct, and we should
> indeed not update the memorized LSN. Or maybe it doesn't matter because
> we retry from the parent anyway, thanks to the 'retry_from_parent' flag?
> I'm not sure..
>

I go through the code many times. I found that updating parent->lsn or not
seemd to have no impact. Sorry, I'm not 100% sure.


>
> If you're interested to work on this, Tender, maybe you can figure that
> out?
>
> --
> Heikki Linnakangas
> Neon (https://neon.tech)
>


-- 
Tender Wang
OpenPie:  https://en.openpie.com/