Re: GiST VACUUM

From: Andrey Borodin <x4mmm(at)yandex-team(dot)ru>
To: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Костя Кузнецов <chapaev28(at)ya(dot)ru>
Subject: Re: GiST VACUUM
Date: 2019-03-15 18:25:27
Message-ID: B1E4DF12-6CD3-4706-BDBD-BF3283328F60@yandex-team.ru
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Hi!

> 11 марта 2019 г., в 20:03, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> написал(а):
>
> On 10/03/2019 18:40, Andrey Borodin wrote:
>> Here's new version of the patch for actual page deletion.
>> Changes:
>> 1. Fixed possible concurrency issue:
>> We were locking child page while holding lock on internal page. Notes
>> in GiST README recommend locking child before parent. Thus now we
>> unlock internal page before locking children for deletion, and lock
>> it again, check that everything is still on it's place and delete
>> only then.
>
> Good catch. The implementation is a bit broken, though. This segfaults:
Sorry for the noise. Few lines of old code leaked into the patch between testing and mailing.

> BTW, we don't seem to have test coverage for this in the regression tests. The tests that delete some rows, where you updated the comment, doesn't actually seem to produce any empty pages that could be deleted.
I've updated test to delete more rows. But it did not trigger previous bug anyway, we had to delete everything for this case.

>
>> One thing still bothers me. Let's assume that we have internal page
>> with 2 deletable leaves. We lock these leaves in order of items on
>> internal page. Is it possible that 2nd page have follow-right link on
>> 1st and someone will lock 2nd page, try to lock 1st and deadlock with
>> VACUUM?
>
> Hmm. If the follow-right flag is set on a page, it means that its right sibling doesn't have a downlink in the parent yet. Nevertheless, I think I'd sleep better, if we acquired the locks in left-to-right order, to be safe.
Actually, I did not found lock coupling in GiST code. But I decided to lock just two pages at once (leaf, then parent, for every pair). PFA v22 with this concurrency logic.

>
> An easy cop-out would be to use LWLockConditionalAcquire, and bail out if we can't get the lock. But if it's not too complicated, it'd be nice to acquire the locks in the correct order to begin with.
>
> I did a round of cleanup and moving things around, before I bumped into the above issue. I'm including them here as separate patches, for easier review, but they should of course be squashed into yours. For convenience, I'm including your patches here as well, so that all the patches you need are in the same place, but they are identical to what you posted.
I've rebased all these patches so that VACUUM should work on every commit. Let's just squash them for the next iteration, it was quite a messy rebase.
BTW, you deleted numEmptyPages, this makes code cleaner, but this variable could stop gistvacuum_recycle_pages() when everything is recycled already.

Thanks!

Best regards, Andrey Borodin.

Attachment Content-Type Size
0001-Add-block-set-data-structure-v22.patch application/octet-stream 16.1 KB
0002-Delete-pages-during-GiST-VACUUM-v22.patch application/octet-stream 19.8 KB
0003-Minor-cleanup-v22.patch application/octet-stream 8.2 KB
0004-Move-the-page-deletion-logic-to-separate-function-v2.patch application/octet-stream 15.3 KB
0005-Remove-numEmptyPages-.-it-s-not-really-needed-v22.patch application/octet-stream 2.5 KB
0006-Misc-cleanup-v22.patch application/octet-stream 1.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2019-03-15 18:39:54 Re: hyrax vs. RelationBuildPartitionDesc
Previous Message Pavel Stehule 2019-03-15 18:19:38 Re: string_to_array, array_to_string function without separator