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