Reproducible GIST index corruption under concurrent updates

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Reproducible GIST index corruption under concurrent updates

Duncan Sands
To reproduce, run the attached python3 script after adjusting CONNECTION_STRING
appropriately.  The target database should have the btree_gist extension
installed.  If it fails due to making too many connections to the database, try
reducing NUM_THREADS (or allowing more connections).

Example bad output (may take a minute or two):

$ ./gist_issue.py

inconsistent: 9286


Each "inconsistent" line means that looking up by "id" in the "sections" table
returns a row but looking up by "file" doesn't, even though "id" and "file" are
always the same:

duncan=> SELECT * FROM sections WHERE id=9286;

   id  | file |     valid

------+------+---------------

  9286 | 9286 | (,1970-01-01)

(1 row)



duncan=> SELECT * FROM sections WHERE file=9286;

  id | file | valid

----+------+-------

(0 rows)


What is going on here is that querying by "file" uses the GIST index but this
index is corrupt:

duncan=> EXPLAIN SELECT * FROM sections WHERE file=9286;

                                         QUERY PLAN


------------------------------------------------------------------------------------------

  Index Scan using sections_file_valid_excl on sections  (cost=0.15..2.37 rows=1
width=18)

    Index Cond: (file = 9286)

(2 rows)



If the "gist_issue.py" script runs forever then it failed to reproduce the problem.


Reproduces with postgres 13.1 on Linux (ubuntu groovy, package
13.1-1.pgdg20.10+1, x86-64).  I reproduced it with default database settings and
with a tuned database, and on multiple Linux machines.

Enjoy!

Best wishes, Duncan.

gist_issue.py (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Reproducible GIST index corruption under concurrent updates

Andrey Borodin-2


> 19 янв. 2021 г., в 16:15, Duncan Sands <[hidden email]> написал(а):
>
> 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.

Best regards, Andrey Borodin.

Reply | Threaded
Open this post in threaded view
|

Re: Reproducible GIST index corruption under concurrent updates

Heikki Linnakangas
On 19/01/2021 19:12, Andrey Borodin wrote:
>> 19 янв. 2021 г., в 16:15, Duncan Sands <[hidden email]> написал(а):
>>
>> 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 <[hidden email]>
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.

- Heikki


Reply | Threaded
Open this post in threaded view
|

Re: Reproducible GIST index corruption under concurrent updates

Heikki Linnakangas
On 19/01/2021 23:53, Heikki Linnakangas wrote:

> On 19/01/2021 19:12, Andrey Borodin wrote:
>>> 19 янв. 2021 г., в 16:15, Duncan Sands <[hidden email]> написал(а):
>>>
>>> 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 <[hidden email]>
> 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

fix-gist-corruption-bug.patch (830 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Reproducible GIST index corruption under concurrent updates

Duncan Sands
> 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!

No problem Heikki, thanks for the quick fix.

Best wishes, Duncan.


Reply | Threaded
Open this post in threaded view
|

Re: Reproducible GIST index corruption under concurrent updates

Heikki Linnakangas
On 20/01/2021 10:23, Duncan Sands wrote:

>> 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!
>
> No problem Heikki, thanks for the quick fix.

Pushed as commit 6b4d3046f4, and backpatched down to version 12, where
this bug was introduced. It will appear in the next minor release.

- Heikki


Reply | Threaded
Open this post in threaded view
|

Re: Reproducible GIST index corruption under concurrent updates

Tom Lane-2
Heikki Linnakangas <[hidden email]> writes:
> Pushed as commit 6b4d3046f4, and backpatched down to version 12, where
> this bug was introduced. It will appear in the next minor release.

I see you noted that REINDEX is required to repair any corrupted indexes.
For the purposes of the release notes, can we characterize which indexes
might be affected, or do we just have to say "better reindex all GIST
indexes"?

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Reproducible GIST index corruption under concurrent updates

Heikki Linnakangas
On 20/01/2021 17:25, Tom Lane wrote:
> Heikki Linnakangas <[hidden email]> writes:
>> Pushed as commit 6b4d3046f4, and backpatched down to version 12, where
>> this bug was introduced. It will appear in the next minor release.
>
> I see you noted that REINDEX is required to repair any corrupted indexes.
> For the purposes of the release notes, can we characterize which indexes
> might be affected, or do we just have to say "better reindex all GIST
> indexes"?

Any GiST index that's been concurrently inserted might be corrupt.
Better reindex all GiST indexes.

- Heikki

P.S. The GiST amcheck extension that's been discussed at [1] would catch
this corruption, so if that was already committed, we could suggest
using it. But that doesn't help us now.

[1]
https://www.postgresql.org/message-id/CAF3eApa07-BajjG8+RYx-Dr_cq28ZA0GsZmUQrGu5b2ayRhB5A@... 
would catch this corruption