Hi!
> 11 марта 2019 г., в 20:03, Heikki Linnakangas <hlinnaka@iki.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
updatedthe 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,
forevery 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
separatepatches, for easier review, but they should of course be squashed into yours. For convenience, I'm including
yourpatches 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
everythingis recycled already.
Thanks!
Best regards, Andrey Borodin.