Fix race condition in GIN posting tree page deletion.

If a page is deleted, and reused for something else, just as a search is
following a rightlink to it from its left sibling, the search would continue
scanning whatever the new contents of the page are. That could lead to
incorrect query results, or even something more curious if the page is
reused for a different kind of a page.

To fix, modify the search algorithm to lock the next page before releasing
the previous one, and refrain from deleting pages from the leftmost branch
of the tree.

Add a new Concurrency section to the README, explaining why this works.
There is a lot more one could say about concurrency in GIN, but that's for
another patch.

Backpatch to all supported versions.

1 files changed, 22 insertions, 29 deletions

diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c
index b84d3a30de3..e6241850e1f 100644
--- a/src/backend/access/gin/ginvacuum.c
+++ b/src/backend/access/gin/ginvacuum.c
@@ -237,6 +237,9 @@ ginVacuumPostingTreeLeaves(GinVacuumState *gvs, BlockNumber blkno, bool isRoot,
 	return hasVoidPage;
 }
 
+/*
+ * Delete a posting tree page.
+ */
 static void
 ginDeletePage(GinVacuumState *gvs, BlockNumber deleteBlkno, BlockNumber leftBlkno,
 			  BlockNumber parentBlkno, OffsetNumber myoff, bool isParentRoot)
@@ -246,39 +249,35 @@ ginDeletePage(GinVacuumState *gvs, BlockNumber deleteBlkno, BlockNumber leftBlkn
 	Buffer		pBuffer;
 	Page		page,
 				parentPage;
+	BlockNumber	rightlink;
 
+	/*
+	 * Lock the pages in the same order as an insertion would, to avoid
+	 * deadlocks: left, then right, then parent.
+	 */
+	lBuffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, leftBlkno,
+								 RBM_NORMAL, gvs->strategy);
 	dBuffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, deleteBlkno,
 								 RBM_NORMAL, gvs->strategy);
-
-	if (leftBlkno != InvalidBlockNumber)
-		lBuffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, leftBlkno,
-									 RBM_NORMAL, gvs->strategy);
-	else
-		lBuffer = InvalidBuffer;
-
 	pBuffer = ReadBufferExtended(gvs->index, MAIN_FORKNUM, parentBlkno,
 								 RBM_NORMAL, gvs->strategy);
 
+	LockBuffer(lBuffer, GIN_EXCLUSIVE);
 	LockBuffer(dBuffer, GIN_EXCLUSIVE);
 	if (!isParentRoot)			/* parent is already locked by
 								 * LockBufferForCleanup() */
 		LockBuffer(pBuffer, GIN_EXCLUSIVE);
-	if (leftBlkno != InvalidBlockNumber)
-		LockBuffer(lBuffer, GIN_EXCLUSIVE);
 
 	START_CRIT_SECTION();
 
-	if (leftBlkno != InvalidBlockNumber)
-	{
-		BlockNumber rightlink;
-
-		page = BufferGetPage(dBuffer);
-		rightlink = GinPageGetOpaque(page)->rightlink;
+	/* Unlink the page by changing left sibling's rightlink */
+	page = BufferGetPage(dBuffer);
+	rightlink = GinPageGetOpaque(page)->rightlink;
 
-		page = BufferGetPage(lBuffer);
-		GinPageGetOpaque(page)->rightlink = rightlink;
-	}
+	page = BufferGetPage(lBuffer);
+	GinPageGetOpaque(page)->rightlink = rightlink;
 
+	/* Delete downlink from parent */
 	parentPage = BufferGetPage(pBuffer);
 #ifdef USE_ASSERT_CHECKING
 	do
@@ -360,10 +359,7 @@ ginDeletePage(GinVacuumState *gvs, BlockNumber deleteBlkno, BlockNumber leftBlkn
 	if (!isParentRoot)
 		LockBuffer(pBuffer, GIN_UNLOCK);
 	ReleaseBuffer(pBuffer);
-
-	if (leftBlkno != InvalidBlockNumber)
-		UnlockReleaseBuffer(lBuffer);
-
+	UnlockReleaseBuffer(lBuffer);
 	UnlockReleaseBuffer(dBuffer);
 
 	END_CRIT_SECTION();
@@ -431,15 +427,12 @@ ginScanToDelete(GinVacuumState *gvs, BlockNumber blkno, bool isRoot, DataPageDel
 
 	if (GinPageGetOpaque(page)->maxoff < FirstOffsetNumber)
 	{
-		if (!(me->leftBlkno == InvalidBlockNumber && GinPageRightMost(page)))
+		/* we never delete the left- or rightmost branch */
+		if (me->leftBlkno != InvalidBlockNumber && !GinPageRightMost(page))
 		{
-			/* we never delete right most branch */
 			Assert(!isRoot);
-			if (GinPageGetOpaque(page)->maxoff < FirstOffsetNumber)
-			{
-				ginDeletePage(gvs, blkno, me->leftBlkno, me->parent->blkno, myoff, me->parent->isRoot);
-				meDelete = TRUE;
-			}
+			ginDeletePage(gvs, blkno, me->leftBlkno, me->parent->blkno, myoff, me->parent->isRoot);
+			meDelete = TRUE;
 		}
 	}