Fix race condition between relation extension and vacuum.

In e6799d5a5301 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
index 7f60d6886862dc8381254100fad187c8672f12ad..53c72c14c3d4b0fa6ffe1fd35c8ed23d4f9320fa 100644 (file)
--- 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;
                }