ReleaseRelationBuffers() failed to check for I/O in progress on a buffer

it wants to release.  This leads to a race condition: does the backend
that's trying to flush the buffer do so before the one that's deleting the
relation does so?  Usually no problem, I expect, but on occasion this could
lead to hard-to-reproduce complaints from md.c, especially mdblindwrt.

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index bd5773e98e5345715f77d79b6b8bf77aa53eb9d6..8b71dc288c4272a9b137e8ce184ea77a7cd6a678 100644 (file)
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -7,7 +7,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v 1.66 1999/11/16 04:13:56 momjian Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v 1.67 1999/11/22 01:19:42 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1056,8 +1056,13 @@ BufferSync()
 
 
 /*
- * WaitIO -- Block until the IO_IN_PROGRESS flag on 'buf'
- *             is cleared.  Because IO_IN_PROGRESS conflicts are
+ * WaitIO -- Block until the IO_IN_PROGRESS flag on 'buf' is cleared.
+ *
+ * Should be entered with buffer manager spinlock held; releases it before
+ * waiting and re-acquires it afterwards.
+ *
+ * OLD NOTES:
+ *             Because IO_IN_PROGRESS conflicts are
  *             expected to be rare, there is only one BufferIO
  *             lock in the entire system.      All processes block
  *             on this semaphore when they try to use a buffer
@@ -1069,15 +1074,13 @@ BufferSync()
  *             is simple, but efficient enough if WaitIO is
  *             rarely called by multiple processes simultaneously.
  *
- *     ProcSleep atomically releases the spinlock and goes to
- *             sleep.
- *
- *     Note: there is an easy fix if the queue becomes long.
- *             save the id of the buffer we are waiting for in
- *             the queue structure.  That way signal can figure
- *             out which proc to wake up.
+ * NEW NOTES:
+ *             The above is true only on machines without test-and-set
+ *             semaphores (which we hope are few, these days).  On better
+ *             hardware, each buffer has a spinlock that we can wait on.
  */
 #ifdef HAS_TEST_AND_SET
+
 static void
 WaitIO(BufferDesc *buf, SPINLOCK spinlock)
 {
@@ -1087,7 +1090,8 @@ WaitIO(BufferDesc *buf, SPINLOCK spinlock)
        SpinAcquire(spinlock);
 }
 
-#else                                                  /* HAS_TEST_AND_SET */
+#else                                                  /* !HAS_TEST_AND_SET */
+
 IpcSemaphoreId WaitIOSemId;
 IpcSemaphoreId WaitCLSemId;
 
@@ -1387,7 +1391,11 @@ RelationGetNumberOfBlocks(Relation relation)
  *
  *             this function unmarks all the dirty pages of a relation
  *             in the buffer pool so that at the end of transaction
- *             these pages will not be flushed.
+ *             these pages will not be flushed.  This is used when the
+ *             relation is about to be deleted.  We assume that the caller
+ *             holds an exclusive lock on the relation, which should assure
+ *             that no new buffers will be acquired for the rel meanwhile.
+ *
  *             XXX currently it sequentially searches the buffer pool, should be
  *             changed to more clever ways of searching.
  * --------------------------------------------------------------------
@@ -1395,8 +1403,9 @@ RelationGetNumberOfBlocks(Relation relation)
 void
 ReleaseRelationBuffers(Relation rel)
 {
+       Oid                     relid = RelationGetRelid(rel);
+       bool            holding = false;
        int                     i;
-       int                     holding = 0;
        BufferDesc *buf;
 
        if (rel->rd_myxactonly)
@@ -1404,9 +1413,8 @@ ReleaseRelationBuffers(Relation rel)
                for (i = 0; i < NLocBuffer; i++)
                {
                        buf = &LocalBufferDescriptors[i];
-                       if ((buf->flags & BM_DIRTY) &&
-                               (buf->tag.relId.relId == RelationGetRelid(rel)))
-                               buf->flags &= ~BM_DIRTY;
+                       if (buf->tag.relId.relId == relid)
+                               buf->flags &= ~ ( BM_DIRTY | BM_JUST_DIRTIED);
                }
                return;
        }
@@ -1417,21 +1425,47 @@ ReleaseRelationBuffers(Relation rel)
                if (!holding)
                {
                        SpinAcquire(BufMgrLock);
-                       holding = 1;
+                       holding = true;
                }
-               if ((buf->flags & BM_DIRTY) &&
-                       (buf->tag.relId.dbId == MyDatabaseId) &&
-                       (buf->tag.relId.relId == RelationGetRelid(rel)))
+       recheck:
+               if (buf->tag.relId.dbId == MyDatabaseId &&
+                       buf->tag.relId.relId == relid)
                {
-                       buf->flags &= ~BM_DIRTY;
+                       /*
+                        * If there is I/O in progress, better wait till it's done;
+                        * don't want to delete the relation out from under someone
+                        * who's just trying to flush the buffer!
+                        */
+                       if (buf->flags & BM_IO_IN_PROGRESS)
+                       {
+                               WaitIO(buf, BufMgrLock);
+                               /* By now, the buffer very possibly belongs to some other
+                                * rel, so check again before proceeding.
+                                */
+                               goto recheck;
+                       }
+                       /* Now we can do what we came for */
+                       buf->flags &= ~ ( BM_DIRTY | BM_JUST_DIRTIED);
+                       CommitInfoNeedsSave[i - 1] = 0;
+                       /*
+                        * Release any refcount we may have.
+                        *
+                        * This is very probably dead code, and if it isn't then it's
+                        * probably wrong.  I added the Assert to find out --- tgl 11/99.
+                        */
                        if (!(buf->flags & BM_FREE))
                        {
+                               /* Assert checks that buffer will actually get freed! */
+                               Assert(PrivateRefCount[i - 1] == 1 &&
+                                          buf->refcount == 1);
+                               /* ReleaseBuffer expects we do not hold the lock at entry */
                                SpinRelease(BufMgrLock);
-                               holding = 0;
+                               holding = false;
                                ReleaseBuffer(i);
                        }
                }
        }
+
        if (holding)
                SpinRelease(BufMgrLock);
 }