Fix deadlock between ginDeletePage() and ginStepRight()

When ginDeletePage() is about to delete page it locks its left sibling to revise
the rightlink.  So, it locks pages in right to left manner.  Int he same time
ginStepRight() locks pages in left to right manner, and that could cause a
deadlock.

This commit makes ginScanToDelete() keep exclusive lock on left siblings of
currently investigated path.  That elimites need to relock left sibling in
ginDeletePage().  Thus, deadlock with ginStepRight() can't happen anymore.

Reported-by: Chen Huajun
Discussion: https://postgr.es/m/5c332bd1.87b6.16d7c17aa98.Coremail.chjischj%40163.com
Author: Alexander Korotkov
Reviewed-by: Peter Geoghegan
Backpatch-through: 10

diff --git a/src/backend/access/gin/README b/src/backend/access/gin/README
index 76c12ae2f6b30e54261600f54539cb76b3f53b5d..e4152500cc6c03cb37c5b3106ccd38938bf9255a 100644 (file)
--- a/src/backend/access/gin/README
+++ b/src/backend/access/gin/README
@@ -277,50 +277,63 @@ followed by the packed items.
 Concurrency
 -----------
 
-The entry tree and each posting tree is a B-tree, with right-links connecting
+The entry tree and each posting tree are B-trees, with right-links connecting
 sibling pages at the same level. This is the same structure that is used in
 the regular B-tree indexam (invented by Lehman & Yao), but we don't support
 scanning a GIN trees backwards, so we don't need left-links.
 
 To avoid deadlocks, B-tree pages must always be locked in the same order:
-left to right, and bottom to top. When searching, the tree is traversed from
-top to bottom, so the lock on the parent page must be released before
+left to right, and bottom to top. The exception is page deletion during vacuum,
+which would be considered separately.  When searching, the tree is traversed
+from top to bottom, so the lock on the parent page must be released before
 descending to the next level. Concurrent page splits move the keyspace to
 right, so after following a downlink, the page actually containing the key
 we're looking for might be somewhere to the right of the page we landed on.
 In that case, we follow the right-links until we find the page we're looking
-for.
-
-To delete a page, the page's left sibling, the target page, and its parent,
-are locked in that order, and the page is marked as deleted. However, a
-concurrent search might already have read a pointer to the page, and might be
-just about to follow it. A page can be reached via the right-link of its left
-sibling, or via its downlink in the parent.
+for. In spite of searches, insertions keeps pins on all pages in path from
+root to the current page. These pages potentially requies downlink insertion,
+while pins prevent them from being deleted.
+
+Vacuum never deletes tuples or pages from the entry tree. It traverses entry
+tree leafs in logical order by rightlinks and removes deletable TIDs from
+posting lists. Posting trees are processed by links from entry tree leafs. They
+are vacuumed in two stages. At first stage, deletable TIDs are removed from
+leafs. If first stage detects at least one empty page, then at the second stage
+ginScanToDelete() deletes empty pages.
+
+ginScanToDelete() scans posting tree to delete empty pages, while vacuum holds
+cleanup lock on the posting tree root. This lock prevent concurrent inserts,
+because inserters hold a pin on the root page. In spite of inserters searches
+don't hold pin on root page. So, while new searches cannot begin while root page
+is locked, any already-in-progress scans can continue concurrently with vacuum.
+
+ginScanToDelete() does depth-first tree scanning while keeping each page in path
+from root to current page exclusively locked.  It also keeps left sibling of
+each page in the path locked.  Thus, if current page is to be removed, all
+required pages to remove both downlink and rightlink are already locked.
+Therefore, page deletion locks pages top to bottom and left to right breaking
+our general rule. But assuming there is no concurrent insertions, this can't
+cause a deadlock.
+
+During replay od page deletion at standby, the page's left sibling, the target
+page, and its parent, are locked in that order. So it follows bottom to top and
+left to right rule.
+
+A search concurrent to page deletion might already have read a pointer to the
+page to be deleted, and might be just about to follow it. A page can be reached
+via the right-link of its left sibling, or via its downlink in the parent.
 
 To prevent a backend from reaching a deleted page via a right-link, when
-following a right-link the lock on the previous page is not released until
-the lock on next page has been acquired.
-
-The downlink is more tricky. A search descending the tree must release the
-lock on the parent page before locking the child, or it could deadlock with
-a concurrent split of the child page; a page split locks the parent, while
-already holding a lock on the child page. So, deleted page cannot be reclaimed
-immediately. Instead, we have to wait for every transaction, which might wait
-to reference this page, to finish. Corresponding processes must observe that
-the page is marked deleted and recover accordingly.
-
-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 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
-deleted pages around with the right-link intact until all concurrent scans
-have finished.)
+following a right-link the lock on the previous page is not released until the
+lock on next page has been acquired.
+
+The downlink is more tricky. A search descending the tree must release the lock
+on the parent page before locking the child, or it could deadlock with a
+concurrent split of the child page; a page split locks the parent, while already
+holding a lock on the child page. So, deleted page cannot be reclaimed
+immediately. Instead, we have to wait for every transaction, which might wait to
+reference this page, to finish. Corresponding processes must observe that the
+page is marked deleted and recover accordingly.
 
 Predicate Locking
 -----------------

diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c
index dc46f2460e262a31eb051680362403d9359076e7..85acddc76b3d038a769e33f8ceb43eb42ed9f3e2 100644 (file)
--- a/src/backend/access/gin/ginvacuum.c
+++ b/src/backend/access/gin/ginvacuum.c
@@ -117,7 +117,8 @@ typedef struct DataPageDeleteStack
    struct DataPageDeleteStack *parent;
 
    BlockNumber blkno;          /* current block number */
-   BlockNumber leftBlkno;      /* rightest non-deleted page on left */
+   Buffer      leftBuffer;     /* pinned and locked rightest non-deleted page
+                                * on left */
    bool        isRoot;
 } DataPageDeleteStack;
 
@@ -139,11 +140,8 @@ ginDeletePage(GinVacuumState *gvs, BlockNumber deleteBlkno, BlockNumber leftBlkn
    /*
     * This function MUST be called only if someone of parent pages hold
     * exclusive cleanup lock. This guarantees that no insertions currently
-    * happen in this subtree. Caller also acquire Exclusive lock on deletable
-    * page and is acquiring and releasing exclusive lock on left page before.
-    * Left page was locked and released. Then parent and this page are
-    * locked. We acquire left page lock here only to mark page dirty after
-    * changing right pointer.
+    * happen in this subtree. Caller also acquires Exclusive locks on
+    * deletable, parent and left pages.
     */
    lBuffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, leftBlkno,
                                 RBM_NORMAL, gvs->strategy);
@@ -152,8 +150,6 @@ ginDeletePage(GinVacuumState *gvs, BlockNumber deleteBlkno, BlockNumber leftBlkn
    pBuffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, parentBlkno,
                                 RBM_NORMAL, gvs->strategy);
 
-   LockBuffer(lBuffer, GIN_EXCLUSIVE);
-
    page = BufferGetPage(dBuffer);
    rightlink = GinPageGetOpaque(page)->rightlink;
 
@@ -227,7 +223,7 @@ ginDeletePage(GinVacuumState *gvs, BlockNumber deleteBlkno, BlockNumber leftBlkn
    }
 
    ReleaseBuffer(pBuffer);
-   UnlockReleaseBuffer(lBuffer);
+   ReleaseBuffer(lBuffer);
    ReleaseBuffer(dBuffer);
 
    END_CRIT_SECTION();
@@ -237,7 +233,11 @@ ginDeletePage(GinVacuumState *gvs, BlockNumber deleteBlkno, BlockNumber leftBlkn
 
 
 /*
- * scans posting tree and deletes empty pages
+ * Scans posting tree and deletes empty pages.  Caller must lock root page for
+ * cleanup.  During scan path from root to current page is kept exclusively
+ * locked.  Also keep left page exclusively locked, because ginDeletePage()
+ * needs it.  If we try to relock left page later, it could deadlock with
+ * ginStepRight().
  */
 static bool
 ginScanToDelete(GinVacuumState *gvs, BlockNumber blkno, bool isRoot,
@@ -260,7 +260,7 @@ ginScanToDelete(GinVacuumState *gvs, BlockNumber blkno, bool isRoot,
            me = (DataPageDeleteStack *) palloc0(sizeof(DataPageDeleteStack));
            me->parent = parent;
            parent->child = me;
-           me->leftBlkno = InvalidBlockNumber;
+           me->leftBuffer = InvalidBuffer;
        }
        else
            me = parent->child;
@@ -288,6 +288,12 @@ ginScanToDelete(GinVacuumState *gvs, BlockNumber blkno, bool isRoot,
            if (ginScanToDelete(gvs, PostingItemGetBlockNumber(pitem), false, me, i))
                i--;
        }
+
+       if (GinPageRightMost(page) && BufferIsValid(me->child->leftBuffer))
+       {
+           UnlockReleaseBuffer(me->child->leftBuffer);
+           me->child->leftBuffer = InvalidBuffer;
+       }
    }
 
    if (GinPageIsLeaf(page))
@@ -298,21 +304,31 @@ ginScanToDelete(GinVacuumState *gvs, BlockNumber blkno, bool isRoot,
    if (isempty)
    {
        /* we never delete the left- or rightmost branch */
-       if (me->leftBlkno != InvalidBlockNumber && !GinPageRightMost(page))
+       if (BufferIsValid(me->leftBuffer) && !GinPageRightMost(page))
        {
            Assert(!isRoot);
-           ginDeletePage(gvs, blkno, me->leftBlkno, me->parent->blkno, myoff, me->parent->isRoot);
+           ginDeletePage(gvs, blkno, BufferGetBlockNumber(me->leftBuffer),
+                         me->parent->blkno, myoff, me->parent->isRoot);
            meDelete = true;
        }
    }
 
-   if (!isRoot)
-       LockBuffer(buffer, GIN_UNLOCK);
+   if (!meDelete)
+   {
+       if (BufferIsValid(me->leftBuffer))
+           UnlockReleaseBuffer(me->leftBuffer);
+       me->leftBuffer = buffer;
+   }
+   else
+   {
+       if (!isRoot)
+           LockBuffer(buffer, GIN_UNLOCK);
 
-   ReleaseBuffer(buffer);
+       ReleaseBuffer(buffer);
+   }
 
-   if (!meDelete)
-       me->leftBlkno = blkno;
+   if (isRoot)
+       ReleaseBuffer(buffer);
 
    return meDelete;
 }
@@ -409,7 +425,7 @@ ginVacuumPostingTree(GinVacuumState *gvs, BlockNumber rootBlkno)
        LockBufferForCleanup(buffer);
 
        memset(&root, 0, sizeof(DataPageDeleteStack));
-       root.leftBlkno = InvalidBlockNumber;
+       root.leftBuffer = InvalidBuffer;
        root.isRoot = true;
 
        ginScanToDelete(gvs, rootBlkno, true, &root, InvalidOffsetNumber);