Re: BufMgr cleanup - Mailing list pgsql-patches

From Bruce Momjian
Subject Re: BufMgr cleanup
Date
Msg-id 200207020547.g625la225077@candle.pha.pa.us
Whole thread Raw
In response to BufMgr cleanup  (Manfred Koizar <mkoi-pg@aon.at>)
List pgsql-patches
Patch applied.  Thanks.

---------------------------------------------------------------------------


Manfred Koizar wrote:
> Minor code cleanup in bufmgr.c and bufmgr.h, mainly by moving repeated
> lines of code into internal routines (drop_relfilenode_buffers,
> release_buffer) and by hiding unused routines (PrintBufferDescs,
> PrintPinnedBufs) behind #ifdef NOT_USED.
> Remove AbortBufferIO() declaration from bufmgr.c (already declared
> in bufmgr.h)
>
> diff -ru ../orig/src/backend/storage/buffer/bufmgr.c src/backend/storage/buffer/bufmgr.c
> --- ../orig/src/backend/storage/buffer/bufmgr.c    2002-06-13 09:32:40.000000000 +0200
> +++ src/backend/storage/buffer/bufmgr.c    2002-06-13 11:02:47.000000000 +0200
> @@ -71,7 +71,6 @@
>  static void StartBufferIO(BufferDesc *buf, bool forInput);
>  static void TerminateBufferIO(BufferDesc *buf);
>  static void ContinueBufferIO(BufferDesc *buf, bool forInput);
> -extern void AbortBufferIO(void);
>
>  /*
>   * Macro : BUFFER_IS_BROKEN
> @@ -85,9 +84,14 @@
>              bool *foundPtr);
>  static int    ReleaseBufferWithBufferLock(Buffer buffer);
>  static int    BufferReplace(BufferDesc *bufHdr);
> +#ifdef NOT_USED
>  void        PrintBufferDescs(void);
> +#endif
>
>  static void write_buffer(Buffer buffer, bool unpin);
> +static void drop_relfilenode_buffers(RelFileNode rnode,
> +                                     bool do_local, bool do_both);
> +static int release_buffer(Buffer buffer, bool havelock);
>
>  /*
>   * ReadBuffer -- returns a buffer containing the requested
> @@ -1097,34 +1101,26 @@
>      return relation->rd_nblocks;
>  }
>
> -/* ---------------------------------------------------------------------
> - *        DropRelationBuffers
> - *
> - *        This function removes all the buffered pages for a relation
> - *        from the buffer pool.  Dirty pages are simply dropped, without
> - *        bothering to write them out first.    This is NOT rollback-able,
> - *        and so should be used only with extreme caution!
> - *
> - *        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.
> +/*
> + * drop_relfilenode_buffers -- common functionality for
> + *                             DropRelationBuffers and
> + *                             DropRelFileNodeBuffers
>   *
>   *        XXX currently it sequentially searches the buffer pool, should be
>   *        changed to more clever ways of searching.
> - * --------------------------------------------------------------------
>   */
> -void
> -DropRelationBuffers(Relation rel)
> +static void
> +drop_relfilenode_buffers(RelFileNode rnode, bool do_local, bool do_both)
>  {
>      int            i;
>      BufferDesc *bufHdr;
>
> -    if (rel->rd_myxactonly)
> +    if (do_local)
>      {
>          for (i = 0; i < NLocBuffer; i++)
>          {
>              bufHdr = &LocalBufferDescriptors[i];
> -            if (RelFileNodeEquals(bufHdr->tag.rnode, rel->rd_node))
> +            if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
>              {
>                  bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
>                  bufHdr->cntxDirty = false;
> @@ -1132,7 +1128,8 @@
>                  bufHdr->tag.rnode.relNode = InvalidOid;
>              }
>          }
> -        return;
> +        if (!do_both)
> +            return;
>      }
>
>      LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
> @@ -1141,7 +1138,7 @@
>      {
>          bufHdr = &BufferDescriptors[i - 1];
>  recheck:
> -        if (RelFileNodeEquals(bufHdr->tag.rnode, rel->rd_node))
> +        if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
>          {
>              /*
>               * If there is I/O in progress, better wait till it's done;
> @@ -1188,6 +1185,25 @@
>  }
>
>  /* ---------------------------------------------------------------------
> + *        DropRelationBuffers
> + *
> + *        This function removes all the buffered pages for a relation
> + *        from the buffer pool.  Dirty pages are simply dropped, without
> + *        bothering to write them out first.    This is NOT rollback-able,
> + *        and so should be used only with extreme caution!
> + *
> + *        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.
> + * --------------------------------------------------------------------
> + */
> +void
> +DropRelationBuffers(Relation rel)
> +{
> +    drop_relfilenode_buffers(rel->rd_node, rel->rd_myxactonly, false);
> +}
> +
> +/* ---------------------------------------------------------------------
>   *        DropRelFileNodeBuffers
>   *
>   *        This is the same as DropRelationBuffers, except that the target
> @@ -1201,73 +1217,8 @@
>  void
>  DropRelFileNodeBuffers(RelFileNode rnode)
>  {
> -    int            i;
> -    BufferDesc *bufHdr;
> -
>      /* We have to search both local and shared buffers... */
> -
> -    for (i = 0; i < NLocBuffer; i++)
> -    {
> -        bufHdr = &LocalBufferDescriptors[i];
> -        if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
> -        {
> -            bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
> -            bufHdr->cntxDirty = false;
> -            LocalRefCount[i] = 0;
> -            bufHdr->tag.rnode.relNode = InvalidOid;
> -        }
> -    }
> -
> -    LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
> -
> -    for (i = 1; i <= NBuffers; i++)
> -    {
> -        bufHdr = &BufferDescriptors[i - 1];
> -recheck:
> -        if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
> -        {
> -            /*
> -             * 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 (bufHdr->flags & BM_IO_IN_PROGRESS)
> -            {
> -                WaitIO(bufHdr);
> -
> -                /*
> -                 * 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 */
> -            bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
> -            bufHdr->cntxDirty = false;
> -
> -            /*
> -             * 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 (!(bufHdr->flags & BM_FREE))
> -            {
> -                /* Assert checks that buffer will actually get freed! */
> -                Assert(PrivateRefCount[i - 1] == 1 &&
> -                       bufHdr->refcount == 1);
> -                ReleaseBufferWithBufferLock(i);
> -            }
> -
> -            /*
> -             * And mark the buffer as no longer occupied by this rel.
> -             */
> -            BufTableDelete(bufHdr);
> -        }
> -    }
> -
> -    LWLockRelease(BufMgrLock);
> +    drop_relfilenode_buffers(rnode, true, true);
>  }
>
>  /* ---------------------------------------------------------------------
> @@ -1343,6 +1294,7 @@
>   *        use only.
>   * -----------------------------------------------------------------
>   */
> +#ifdef NOT_USED
>  void
>  PrintBufferDescs()
>  {
> @@ -1375,7 +1327,9 @@
>          }
>      }
>  }
> +#endif
>
> +#ifdef NOT_USED
>  void
>  PrintPinnedBufs()
>  {
> @@ -1395,6 +1349,7 @@
>      }
>      LWLockRelease(BufMgrLock);
>  }
> +#endif
>
>  /*
>   * BufferPoolBlowaway
> @@ -1589,14 +1544,12 @@
>      return 0;
>  }
>
> -#undef ReleaseBuffer
> -
>  /*
> - * ReleaseBuffer -- remove the pin on a buffer without
> - *        marking it dirty.
> + * release_buffer -- common functionality for
> + *                   ReleaseBuffer and ReleaseBufferWithBufferLock
>   */
> -int
> -ReleaseBuffer(Buffer buffer)
> +static int
> +release_buffer(Buffer buffer, bool havelock)
>  {
>      BufferDesc *bufHdr;
>
> @@ -1617,14 +1570,30 @@
>          PrivateRefCount[buffer - 1]--;
>      else
>      {
> -        LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
> +        if (!havelock)
> +            LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
> +
>          UnpinBuffer(bufHdr);
> -        LWLockRelease(BufMgrLock);
> +
> +        if (!havelock)
> +            LWLockRelease(BufMgrLock);
>      }
>
>      return STATUS_OK;
>  }
>
> +#undef ReleaseBuffer
> +
> +/*
> + * ReleaseBuffer -- remove the pin on a buffer without
> + *        marking it dirty.
> + */
> +int
> +ReleaseBuffer(Buffer buffer)
> +{
> +    return release_buffer(buffer, false);
> +}
> +
>  /*
>   * ReleaseBufferWithBufferLock
>   *        Same as ReleaseBuffer except we hold the bufmgr lock
> @@ -1632,27 +1601,7 @@
>  static int
>  ReleaseBufferWithBufferLock(Buffer buffer)
>  {
> -    BufferDesc *bufHdr;
> -
> -    if (BufferIsLocal(buffer))
> -    {
> -        Assert(LocalRefCount[-buffer - 1] > 0);
> -        LocalRefCount[-buffer - 1]--;
> -        return STATUS_OK;
> -    }
> -
> -    if (BAD_BUFFER_ID(buffer))
> -        return STATUS_ERROR;
> -
> -    bufHdr = &BufferDescriptors[buffer - 1];
> -
> -    Assert(PrivateRefCount[buffer - 1] > 0);
> -    if (PrivateRefCount[buffer - 1] > 1)
> -        PrivateRefCount[buffer - 1]--;
> -    else
> -        UnpinBuffer(bufHdr);
> -
> -    return STATUS_OK;
> +    return release_buffer(buffer, true);
>  }
>
>
> @@ -1695,7 +1644,7 @@
>  #endif
>
>  #ifdef NOT_USED
> -int
> +Buffer
>  ReleaseAndReadBuffer_Debug(char *file,
>                             int line,
>                             Buffer buffer,
> @@ -2117,7 +2066,7 @@
>  {
>      Assert(buf == InProgressBuf);
>      LWLockRelease(buf->io_in_progress_lock);
> -    InProgressBuf = (BufferDesc *) 0;
> +    InProgressBuf = (BufferDesc *) NULL;
>  }
>
>  /*
> @@ -2142,7 +2091,7 @@
>  void
>  InitBufferIO(void)
>  {
> -    InProgressBuf = (BufferDesc *) 0;
> +    InProgressBuf = (BufferDesc *) NULL;
>  }
>  #endif
>
> diff -ru ../orig/src/include/storage/bufmgr.h src/include/storage/bufmgr.h
> --- ../orig/src/include/storage/bufmgr.h    2002-06-13 09:22:59.000000000 +0200
> +++ src/include/storage/bufmgr.h    2002-06-13 10:57:22.000000000 +0200
> @@ -167,7 +167,9 @@
>  extern void DropRelationBuffers(Relation rel);
>  extern void DropRelFileNodeBuffers(RelFileNode rnode);
>  extern void DropBuffers(Oid dbid);
> +#ifdef NOT_USED
>  extern void PrintPinnedBufs(void);
> +#endif
>  extern int    BufferShmemSize(void);
>  extern RelFileNode BufferGetFileNode(Buffer buffer);
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/users-lounge/docs/faq.html
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026



pgsql-patches by date:

Previous
From: Bruce Momjian
Date:
Subject: Re: Reduce heap tuple header size
Next
From: Bruce Momjian
Date:
Subject: Re: Page access