Re: BUG #16162: create index using gist_trgm_ops leads to panic

On 15/12/2019 19:57, Jeff Janes wrote:
> On Sat, Dec 14, 2019 at 2:00 PM Alexander Lakhin <exclusion@gmail.com 
> <mailto:exclusion@gmail.com>> wrote:
> 
>     14.12.2019 1:02, Heikki Linnakangas wrote:
>      >
>      > Committed a fix. Many thanks for the reproducer scripts, Andreas and
>      > Alexander!
>     The script that I presented in [0] still reproduces assertion failure
>     for me on REL_12_STABLE (fd005e1a):
> 
> I've verified that this was introduced by 9155580f and that Heikki's 
> latest commit a7ee7c851 has not fixed it.  I've reproduced it without 
> cassert.

So there was still one instance of basically the same bug. In the same 
function, there are two calls to gistinserttuples():

> 	/*
> 	 * insert downlinks for the siblings from right to left, until there are
> 	 * only two siblings left.
> 	 */
> 	for (int pos = list_length(splitinfo) - 1; pos > 1; pos--)
> 	{
> 		right = (GISTPageSplitInfo *) list_nth(splitinfo, pos);
> 		left = (GISTPageSplitInfo *) list_nth(splitinfo, pos - 1);
> 
> 		if (gistinserttuples(state, stack->parent, giststate,
> 							 &right->downlink, 1,
> 							 InvalidOffsetNumber,
> 							 left->buf, right->buf, false, false))
> 		{
> 			/*
> 			 * If the parent page was split, need to relocate the original
> 			 * parent pointer.
> 			 */
> 			stack->downlinkoffnum = InvalidOffsetNumber;
> 			gistFindCorrectParent(state->r, stack);
> 		}
> 		/* gistinserttuples() released the lock on right->buf. */
> 	}
> 
> 	right = (GISTPageSplitInfo *) lsecond(splitinfo);
> 	left = (GISTPageSplitInfo *) linitial(splitinfo);
> 
> 	/*
> 	 * Finally insert downlink for the remaining right page and update the
> 	 * downlink for the original page to not contain the tuples that were
> 	 * moved to the new pages.
> 	 */
> 	tuples[0] = left->downlink;
> 	tuples[1] = right->downlink;
> 	gistinserttuples(state, stack->parent, giststate,
> 					 tuples, 2,
> 					 stack->downlinkoffnum,
> 					 left->buf, right->buf,
> 					 true,		/* Unlock parent */
> 					 unlockbuf	/* Unlock stack->buffer if caller wants that */
> 		);
> 	Assert(left->buf == stack->buffer);

I added the "stack-downlinkoffnum = InvalidOffsetNumber" to the first 
call on Friday. But the second call needs the same treatment, because 
it's possible that we reuse the same stack on a later call to 
gistfinishsplit(), if a grandchild page is split twice, for the same 
leaf insert. I feel pretty dumb, because I looked at that second call on 
Friday too, thinking if it also needs to reset 'downlinkoffnum', but 
somehow I concluded it does not. Clearly it does, as this test case 
demonstrates.

Fix committed. Thanks again for the report!

- Heikki