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

Heikki Linnakangas <hlinnaka(at)iki(dot)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/