From fd83c83d0943a6717dbe521efa10da9dce94e4cd Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Thu, 13 Dec 2018 06:12:11 +0300 Subject: [PATCH] Fix deadlock in GIN vacuum introduced by 218f51584d5 Before 218f51584d5 if posting tree page is about to be deleted, then the whole posting tree is locked by LockBufferForCleanup() on root preventing all the concurrent inserts. 218f51584d5 reduced locking to the subtree containing page to be deleted. However, due to concurrent parent split, inserter doesn't always holds pins on all the pages constituting path from root to the target leaf page. That could cause a deadlock between GIN vacuum process and GIN inserter. And we didn't find non-invasive way to fix this. This commit reverts VACUUM behavior to lock the whole posting tree before delete any page. However, we keep another useful change by 218f51584d5: the tree is locked only if there are pages to be deleted. Reported-by: Chen Huajun Diagnosed-by: Chen Huajun, Andrey Borodin, Peter Geoghegan Discussion: https://postgr.es/m/31a702a.14dd.166c1366ac1.Coremail.chjischj%40163.com Author: Alexander Korotkov, based on ideas from Andrey Borodin and Peter Geoghegan Reviewed-by: Andrey Borodin Backpatch-through: 10 --- src/backend/access/gin/README | 15 +-- src/backend/access/gin/ginvacuum.c | 162 ++++++++++++++--------------- 2 files changed, 80 insertions(+), 97 deletions(-) diff --git a/src/backend/access/gin/README b/src/backend/access/gin/README index cc434b1feb..421b5b26d5 100644 --- a/src/backend/access/gin/README +++ b/src/backend/access/gin/README @@ -314,17 +314,10 @@ deleted. The previous paragraph's reasoning only applies to searches, and only to posting trees. To protect from inserters following a downlink to a deleted page, vacuum simply locks out all concurrent insertions to the posting tree, -by holding a super-exclusive lock on the parent page of subtree with deletable -pages. Inserters hold a pin on the root page, but searches do not, so while -new searches cannot begin while root page is locked, any already-in-progress -scans can continue concurrently with vacuum in corresponding subtree of -posting tree. To exclude interference with readers vacuum takes exclusive -locks in a depth-first scan in left-to-right order of page tuples. Leftmost -page is never deleted. Thus before deleting any page we obtain exclusive -lock on any left page, effectively excluding deadlock with any reader, despite -taking parent lock before current and left lock after current. We take left -lock not for a concurrency reasons, but rather in need to mark page dirty. -In the entry tree, we never delete pages. +by holding a super-exclusive lock on the posting tree root. Inserters hold a +pin on the root page, but searches do not, so while new searches cannot begin +while root page is locked, any already-in-progress scans can continue +concurrently with vacuum. In the entry tree, we never delete pages. (This is quite different from the mechanism the btree indexam uses to make page-deletions safe; it stamps the deleted pages with an XID and keeps the diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c index 3104bc12b6..bdeb0bf4f5 100644 --- a/src/backend/access/gin/ginvacuum.c +++ b/src/backend/access/gin/ginvacuum.c @@ -315,122 +315,112 @@ ginScanToDelete(GinVacuumState *gvs, BlockNumber blkno, bool isRoot, /* - * Scan through posting tree, delete empty tuples from leaf pages. - * Also, this function collects empty subtrees (with all empty leafs). - * For parents of these subtrees CleanUp lock is taken, then we call - * ScanToDelete. This is done for every inner page, which points to - * empty subtree. + * Scan through posting tree leafs, delete empty tuples. Returns true if there + * is at least one empty page. */ static bool -ginVacuumPostingTreeLeaves(GinVacuumState *gvs, BlockNumber blkno, bool isRoot) +ginVacuumPostingTreeLeaves(GinVacuumState *gvs, BlockNumber blkno) { Buffer buffer; Page page; bool hasVoidPage = false; MemoryContext oldCxt; - buffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, blkno, - RBM_NORMAL, gvs->strategy); - page = BufferGetPage(buffer); + /* Find leftmost leaf page of posting tree and lock it in exclusive mode */ + while (true) + { + PostingItem *pitem; - ginTraverseLock(buffer, false); + buffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, blkno, + RBM_NORMAL, gvs->strategy); + LockBuffer(buffer, GIN_SHARE); + page = BufferGetPage(buffer); - Assert(GinPageIsData(page)); + Assert(GinPageIsData(page)); - if (GinPageIsLeaf(page)) + if (GinPageIsLeaf(page)) + { + LockBuffer(buffer, GIN_UNLOCK); + LockBuffer(buffer, GIN_EXCLUSIVE); + break; + } + + Assert(PageGetMaxOffsetNumber(page) >= FirstOffsetNumber); + + pitem = GinDataPageGetPostingItem(page, FirstOffsetNumber); + blkno = PostingItemGetBlockNumber(pitem); + Assert(blkno != InvalidBlockNumber); + + UnlockReleaseBuffer(buffer); + } + + /* Iterate all posting tree leaves using rightlinks and vacuum them */ + while (true) { oldCxt = MemoryContextSwitchTo(gvs->tmpCxt); ginVacuumPostingTreeLeaf(gvs->index, buffer, gvs); MemoryContextSwitchTo(oldCxt); MemoryContextReset(gvs->tmpCxt); - /* if root is a leaf page, we don't desire further processing */ if (GinDataLeafPageIsEmpty(page)) hasVoidPage = true; - UnlockReleaseBuffer(buffer); - - return hasVoidPage; - } - else - { - OffsetNumber i; - bool hasEmptyChild = false; - bool hasNonEmptyChild = false; - OffsetNumber maxoff = GinPageGetOpaque(page)->maxoff; - BlockNumber *children = palloc(sizeof(BlockNumber) * (maxoff + 1)); - - /* - * Read all children BlockNumbers. Not sure it is safe if there are - * many concurrent vacuums. - */ - - for (i = FirstOffsetNumber; i <= maxoff; i++) - { - PostingItem *pitem = GinDataPageGetPostingItem(page, i); - - children[i] = PostingItemGetBlockNumber(pitem); - } + blkno = GinPageGetOpaque(page)->rightlink; UnlockReleaseBuffer(buffer); - for (i = FirstOffsetNumber; i <= maxoff; i++) - { - if (ginVacuumPostingTreeLeaves(gvs, children[i], false)) - hasEmptyChild = true; - else - hasNonEmptyChild = true; - } + if (blkno == InvalidBlockNumber) + break; - pfree(children); - - vacuum_delay_point(); - - /* - * All subtree is empty - just return true to indicate that parent - * must do a cleanup, unless we are ROOT and there is way to go upper. - */ - - if (hasEmptyChild && !hasNonEmptyChild && !isRoot) - return true; - - if (hasEmptyChild) - { - DataPageDeleteStack root, - *ptr, - *tmp; - - buffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, blkno, - RBM_NORMAL, gvs->strategy); - LockBufferForCleanup(buffer); - - memset(&root, 0, sizeof(DataPageDeleteStack)); - root.leftBlkno = InvalidBlockNumber; - root.isRoot = true; - - ginScanToDelete(gvs, blkno, true, &root, InvalidOffsetNumber); - - ptr = root.child; - - while (ptr) - { - tmp = ptr->child; - pfree(ptr); - ptr = tmp; - } - - UnlockReleaseBuffer(buffer); - } - - /* Here we have deleted all empty subtrees */ - return false; + buffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, blkno, + RBM_NORMAL, gvs->strategy); + LockBuffer(buffer, GIN_EXCLUSIVE); + page = BufferGetPage(buffer); } + + return hasVoidPage; } static void ginVacuumPostingTree(GinVacuumState *gvs, BlockNumber rootBlkno) { - ginVacuumPostingTreeLeaves(gvs, rootBlkno, true); + if (ginVacuumPostingTreeLeaves(gvs, rootBlkno)) + { + /* + * There is at least one empty page. So we have to rescan the tree + * deleting empty pages. + */ + Buffer buffer; + DataPageDeleteStack root, + *ptr, + *tmp; + + buffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, rootBlkno, + RBM_NORMAL, gvs->strategy); + + /* + * Lock posting tree root for cleanup to ensure there are no concurrent + * inserts. + */ + LockBufferForCleanup(buffer); + + memset(&root, 0, sizeof(DataPageDeleteStack)); + root.leftBlkno = InvalidBlockNumber; + root.isRoot = true; + + ginScanToDelete(gvs, rootBlkno, true, &root, InvalidOffsetNumber); + + ptr = root.child; + + while (ptr) + { + tmp = ptr->child; + pfree(ptr); + ptr = tmp; + } + + UnlockReleaseBuffer(buffer); + } } /*