Hi! > 11 марта 2019 г., в 20:03, Heikki Linnakangas <hlinn...@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 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.
0001-Add-block-set-data-structure-v22.patch
Description: Binary data
0002-Delete-pages-during-GiST-VACUUM-v22.patch
Description: Binary data
0003-Minor-cleanup-v22.patch
Description: Binary data
0004-Move-the-page-deletion-logic-to-separate-function-v2.patch
Description: Binary data
0005-Remove-numEmptyPages-.-it-s-not-really-needed-v22.patch
Description: Binary data
0006-Misc-cleanup-v22.patch
Description: Binary data