Repair very-low-probability race condition between relation extension

and VACUUM: in the interval between adding a new page to the relation
and formatting it, it was possible for VACUUM to come along and decide
it should format the page too.  Though not harmful in itself, this would
cause data loss if a third transaction were able to insert tuples into
the vacuumed page before the original extender got control back.

2 files changed, 34 insertions, 7 deletions

diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 3e2a9010a44..378c5f9d36f 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtpage.c,v 1.81 2004/12/31 21:59:22 pgsql Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtpage.c,v 1.81.4.1 2005/05/07 21:32:53 tgl Exp $
  *
  *	NOTES
  *	   Postgres btree pages look like ordinary relation pages.	The opaque
@@ -491,18 +491,21 @@ _bt_getbuf(Relation rel, BlockNumber blkno, int access)
 
 		buf = ReadBuffer(rel, P_NEW);
 
+		/* Acquire buffer lock on new page */
+		LockBuffer(buf, BT_WRITE);
+
 		/*
-		 * Release the file-extension lock; it's now OK for someone else
-		 * to extend the relation some more.
+		 * Release the file-extension lock; it's now OK for someone else to
+		 * extend the relation some more.  Note that we cannot release this
+		 * lock before we have buffer lock on the new page, or we risk a
+		 * race condition against btvacuumcleanup --- see comments therein.
 		 */
 		if (needLock)
 			UnlockPage(rel, 0, ExclusiveLock);
 
-		/* Acquire appropriate buffer lock on new page */
-		LockBuffer(buf, access);
-
 		/* Initialize the new page before returning it */
 		page = BufferGetPage(buf);
+		Assert(PageIsNew((PageHeader) page));
 		_bt_pageinit(page, BufferGetPageSize(buf));
 	}
 
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index ba31abcbbfd..988cbc2a218 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -12,7 +12,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtree.c,v 1.124 2004/12/31 21:59:22 pgsql Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtree.c,v 1.124.4.1 2005/05/07 21:32:53 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -670,11 +670,35 @@ btvacuumcleanup(PG_FUNCTION_ARGS)
 	BlockNumber pages_deleted = 0;
 	MemoryContext mycontext;
 	MemoryContext oldcontext;
+	bool		needLock;
 
 	Assert(stats != NULL);
 
+	/*
+	 * First find out the number of pages in the index.  We must acquire
+	 * the relation-extension lock while doing this to avoid a race
+	 * condition: if someone else is extending the relation, there is
+	 * a window where bufmgr/smgr have created a new all-zero page but
+	 * it hasn't yet been write-locked by _bt_getbuf().  If we manage to
+	 * scan such a page here, we'll improperly assume it can be recycled.
+	 * Taking the lock synchronizes things enough to prevent a problem:
+	 * either num_pages won't include the new page, or _bt_getbuf already
+	 * has write lock on the buffer and it will be fully initialized before
+	 * we can examine it.  (See also vacuumlazy.c, which has the same issue.)
+	 *
+	 * We can skip locking for new or temp relations,
+	 * however, since no one else could be accessing them.
+	 */
+	needLock = !RELATION_IS_LOCAL(rel);
+
+	if (needLock)
+		LockPage(rel, 0, ExclusiveLock);
+
 	num_pages = RelationGetNumberOfBlocks(rel);
 
+	if (needLock)
+		UnlockPage(rel, 0, ExclusiveLock);
+
 	/* No point in remembering more than MaxFSMPages pages */
 	maxFreePages = MaxFSMPages;
 	if ((BlockNumber) maxFreePages > num_pages)