Re: Reproducible GIST index corruption under concurrent updates

On 19/01/2021 23:53, Heikki Linnakangas wrote:
> On 19/01/2021 19:12, Andrey Borodin wrote:
>>> 19 янв. 2021 г., в 16:15, Duncan Sands <duncan(dot)sands(at)deepbluecap(dot)com> написал(а):
>>>
>>> Enjoy!
>>
>> Thanks!
>> It reproduces on my machine. Seems like all tuples are indexed, but some root downlinks are not adjusted properly. Probably, after concurrent split.
>
> This is reproducable down to v12. I bisected it to this commit:
>
> commit e2e992c93145cfc0e3563fb84efd25b390a84bb9 (refs/bisect/bad)
> Author: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Wed Jul 24 20:24:05 2019 +0300
>
> Refactor checks for deleted GiST pages.
>
> The explicit check in gistScanPage() isn't currently really
> necessary, as
> a deleted page is always empty, so the loop would fall through without
> doing anything, anyway. But it's a marginal optimization, and it
> gives a
> nice place to attach a comment to explain how it works.
>
> Backpatch to v12, where GiST page deletion was introduced.
>
> Reviewed-by: Andrey Borodin
> Discussion:
> https://www.postgresql.org/message-id/835A15A5-F1B4-4446-A711-BF48357EB602%40yandex-team.ru
>
> I'll dig deeper tomorrow.

The culprit is this change:

> @@ -858,12 +856,13 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace,
> * leaf/inner is enough to recognize split for root
> */
> }
> - else if (GistFollowRight(stack->page) ||
> - stack->parent->lsn < GistPageGetNSN(stack->page))
> + else if ((GistFollowRight(stack->page) ||
> + stack->parent->lsn < GistPageGetNSN(stack->page)) &&
> + GistPageIsDeleted(stack->page))
> {
> /*
> - * The page was split while we momentarily unlocked the
> - * page. Go back to parent.
> + * The page was split or deleted while we momentarily
> + * unlocked the page. Go back to parent.
> */
> UnlockReleaseBuffer(stack->buffer);
> xlocked = false;

The comment change was correct, but the condition used &&. Should've
been ||. There is another copy of basically the same condition earlier
in the function that was changed correctly, but I blundered this one. Oops.

The attached patch fixes this. I also added an assertion to the
gistplacetopage() function, to check that we never try to insert on a
deleted page. This bug could've made that happen too, although in this
case the problem was a concurrent split, not a deletion. I'll backpatch
and push this tomorrow.

Many thanks for the easy reproducer script, Duncan!

- Heikki