Fix race condition between relation extension and vacuum.

anarazel · anarazel · commit fc02e6724f3c · 2019-01-28T15:44:12.000-08:00
In e6799d5 I removed vacuumlazy.c trickery around re-checking whether a page is actually empty after acquiring an extension lock on the relation, because the page is not PageInit()ed anymore, and entries in the FSM ought not to lead to user-visible errors. As reported by various buildfarm animals that is not correct, given the way to code currently stands: If vacuum processes a page that's just been newly added by either RelationGetBufferForTuple() or RelationAddExtraBlocks(), it could add that page to the FSM and it could be reused by other backends, before those two functions check whether the newly added page is actually new. That's a relatively narrow race, but several buildfarm machines appear to be able to hit it. While it seems wrong that the FSM, given it's lack of durability and approximative nature, can trigger errors like this, that seems better fixed in a separate commit. Especially given that a good portion of the buildfarm is red, and this is just re-introducing logic that existed a few hours ago. Author: Andres Freund Discussion: https://postgr.es/m/20190128222259.zhi7ovzgtkft6em6@alap3.anarazel.de
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
@@ -860,6 +860,8 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
 
 		if (PageIsNew(page))
 		{
+			bool		still_new;
+
 			/*
 			 * All-zeroes pages can be left over if either a backend extends
 			 * the relation by a single page, but crashes before the newly
@@ -877,21 +879,45 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
 			 * promoted standby. The harm of repeated checking ought to
 			 * normally not be too bad - the space usually should be used at
 			 * some point, otherwise there wouldn't be any regular vacuums.
+			 *
+			 * We have to be careful here because we could be looking at a
+			 * page that someone has just added to the relation and the
+			 * extending backend might not yet have been able to lock the page
+			 * (see RelationGetBufferForTuple), which is problematic because
+			 * of cross-checks that new pages are actually new. If we add this
+			 * page to the FSM, this page could be reused, and such
+			 * crosschecks could fail. To protect against that, release the
+			 * buffer lock, grab the relation extension lock momentarily, and
+			 * re-lock the buffer. If the page is still empty and not in the
+			 * FSM by then, it must be left over from a from a crashed
+			 * backend, and we can record the free space.
+			 *
+			 * Note: the comparable code in vacuum.c need not worry because
+			 * it's got an exclusive lock on the whole relation.
 			 */
-			empty_pages++;
+			LockBuffer(buf, BUFFER_LOCK_UNLOCK);
+			LockRelationForExtension(onerel, ExclusiveLock);
+			UnlockRelationForExtension(onerel, ExclusiveLock);
+			LockBufferForCleanup(buf);
 
 			/*
 			 * Perform checking of FSM after releasing lock, the fsm is
 			 * approximate, after all.
 			 */
+			still_new = PageIsNew(page);
 			UnlockReleaseBuffer(buf);
 
-			if (GetRecordedFreeSpace(onerel, blkno) == 0)
+			if (still_new)
 			{
-				Size		freespace;
+				empty_pages++;
+
+				if (GetRecordedFreeSpace(onerel, blkno) == 0)
+				{
+					Size		freespace;
 
-				freespace = BufferGetPageSize(buf) - SizeOfPageHeaderData;
-				RecordPageWithFreeSpace(onerel, blkno, freespace);
+					freespace = BufferGetPageSize(buf) - SizeOfPageHeaderData;
+					RecordPageWithFreeSpace(onerel, blkno, freespace);
+				}
 			}
 			continue;
 		}
