Fix ReadBuffer() to correctly handle the case where it's trying to extend

the relation but it finds a pre-existing valid buffer.  The buffer does not
correspond to any page known to the kernel, so we *must* do smgrextend to
ensure that the space becomes allocated.  The 7.x branches all do this
correctly, but the corner case got lost somewhere during 8.0 bufmgr rewrites.
(My fault no doubt :-( ... I think I assumed that such a buffer must be
not-BM_VALID, which is not so.)

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index a8a37e5ca3cfa5f2ccba3ec2bacf6aae1441da5e..11e83812d0c0712053995fd7347a04e29f7a9912 100644 (file)
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/storage/buffer/bufmgr.c,v 1.201 2005/12/29 18:08:05 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/storage/buffer/bufmgr.c,v 1.202 2006/01/06 00:04:20 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -164,13 +164,47 @@ ReadBuffer(Relation reln, BlockNumber blockNum)
    /* if it was already in the buffer pool, we're done */
    if (found)
    {
-       /* Just need to update stats before we exit */
-       pgstat_count_buffer_hit(&reln->pgstat_info, reln);
+       if (!isExtend)
+       {
+           /* Just need to update stats before we exit */
+           pgstat_count_buffer_hit(&reln->pgstat_info, reln);
+
+           if (VacuumCostActive)
+               VacuumCostBalance += VacuumCostPageHit;
 
-       if (VacuumCostActive)
-           VacuumCostBalance += VacuumCostPageHit;
+           return BufferDescriptorGetBuffer(bufHdr);
+       }
 
-       return BufferDescriptorGetBuffer(bufHdr);
+       /*
+        * We get here only in the corner case where we are trying to extend
+        * the relation but we found a pre-existing buffer marked BM_VALID.
+        * (This can happen because mdread doesn't complain about reads
+        * beyond EOF --- which is arguably bogus, but changing it seems
+        * tricky.)  We *must* do smgrextend before succeeding, else the
+        * page will not be reserved by the kernel, and the next P_NEW call
+        * will decide to return the same page.  Clear the BM_VALID bit,
+        * do the StartBufferIO call that BufferAlloc didn't, and proceed.
+        */
+       if (isLocalBuf)
+       {
+           /* Only need to adjust flags */
+           Assert(bufHdr->flags & BM_VALID);
+           bufHdr->flags &= ~BM_VALID;
+       }
+       else
+       {
+           /*
+            * Loop to handle the very small possibility that someone
+            * re-sets BM_VALID between our clearing it and StartBufferIO
+            * inspecting it.
+            */
+           do {
+               LockBufHdr(bufHdr);
+               Assert(bufHdr->flags & BM_VALID);
+               bufHdr->flags &= ~BM_VALID;
+               UnlockBufHdr(bufHdr);
+           } while (!StartBufferIO(bufHdr, true));
+       }
    }
 
    /*