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

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.

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..

If you're interested to work on this, Tender, maybe you can figure that out?

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