Thread: bufmgr code cleanup

bufmgr code cleanup

From
Neil Conway
Date:
This patch cleans up some of the bufmgr code:

- replace uses of

        LockBuffer(buf, BUFFER_LOCK_UNLOCK);
        ReleaseBuffer(buf);

  with the equivalent, but more concise:

    UnlockAndReleaseBuffer(buf);

- analogous changes were made by replacing LockBuffer() + WriteBuffer()
with UnlockAndWriteBuffer()

- remove a bunch of #ifdef BMTRACE code, since it was ugly and broken
anyway

- remove an unused buffer descriptor bit flag (BM_PRIVATE)

- move the definition of INVALID_DESCRIPTOR to out of bufmgr.h and into
freelist.c, since it is the only file that uses it

- remove another unused function, and fix a few comments

Please apply to the CVS HEAD.

-Neil


Attachment

Re: bufmgr code cleanup

From
Jan Wieck
Date:
Neil Conway wrote:
> This patch cleans up some of the bufmgr code:
>
> - replace uses of
>
>         LockBuffer(buf, BUFFER_LOCK_UNLOCK);
>         ReleaseBuffer(buf);
>
>   with the equivalent, but more concise:
>
>     UnlockAndReleaseBuffer(buf);
>
> - analogous changes were made by replacing LockBuffer() + WriteBuffer()
> with UnlockAndWriteBuffer()
>
> - remove a bunch of #ifdef BMTRACE code, since it was ugly and broken
> anyway
>
> - remove an unused buffer descriptor bit flag (BM_PRIVATE)
>
> - move the definition of INVALID_DESCRIPTOR to out of bufmgr.h and into
> freelist.c, since it is the only file that uses it
>
> - remove another unused function, and fix a few comments
>
> Please apply to the CVS HEAD.

Can this be held off a little while we're experimenting with
improvements to the buffer algorithms?


Jan

>
> -Neil
>
>
>
> ------------------------------------------------------------------------
>
> Index: src/backend/access/hash/hashpage.c
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/backend/access/hash/hashpage.c,v
> retrieving revision 1.42
> diff -c -r1.42 hashpage.c
> *** src/backend/access/hash/hashpage.c    4 Sep 2003 22:06:27 -0000    1.42
> --- src/backend/access/hash/hashpage.c    31 Oct 2003 22:55:59 -0000
> ***************
> *** 135,142 ****
>   void
>   _hash_relbuf(Relation rel, Buffer buf)
>   {
> !     LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !     ReleaseBuffer(buf);
>   }
>
>   /*
> --- 135,141 ----
>   void
>   _hash_relbuf(Relation rel, Buffer buf)
>   {
> !     UnlockAndReleaseBuffer(buf);
>   }
>
>   /*
> ***************
> *** 166,173 ****
>   void
>   _hash_wrtbuf(Relation rel, Buffer buf)
>   {
> !     LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !     WriteBuffer(buf);
>   }
>
>   /*
> --- 165,171 ----
>   void
>   _hash_wrtbuf(Relation rel, Buffer buf)
>   {
> !     UnlockAndWriteBuffer(buf);
>   }
>
>   /*
> Index: src/backend/access/heap/heapam.c
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/backend/access/heap/heapam.c,v
> retrieving revision 1.157
> diff -c -r1.157 heapam.c
> *** src/backend/access/heap/heapam.c    1 Oct 2003 21:30:52 -0000    1.157
> --- src/backend/access/heap/heapam.c    31 Oct 2003 22:59:14 -0000
> ***************
> *** 899,906 ****
>        */
>       if (!ItemIdIsUsed(lp))
>       {
> !         LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
> !         ReleaseBuffer(buffer);
>           *userbuf = InvalidBuffer;
>           tuple->t_datamcxt = NULL;
>           tuple->t_data = NULL;
> --- 899,905 ----
>        */
>       if (!ItemIdIsUsed(lp))
>       {
> !         UnlockAndReleaseBuffer(buffer);
>           *userbuf = InvalidBuffer;
>           tuple->t_datamcxt = NULL;
>           tuple->t_data = NULL;
> ***************
> *** 1006,1013 ****
>       }
>       if (invalidBlock)
>       {
> !         LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
> !         ReleaseBuffer(buffer);
>           return NULL;
>       }
>
> --- 1005,1011 ----
>       }
>       if (invalidBlock)
>       {
> !         UnlockAndReleaseBuffer(buffer);
>           return NULL;
>       }
>
> ***************
> *** 1033,1040 ****
>           !ItemPointerEquals(tid, &ctid))
>           linkend = false;
>
> !     LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
> !     ReleaseBuffer(buffer);
>
>       if (!valid)
>       {
> --- 1031,1037 ----
>           !ItemPointerEquals(tid, &ctid))
>           linkend = false;
>
> !     UnlockAndReleaseBuffer(buffer);
>
>       if (!valid)
>       {
> ***************
> *** 1174,1181 ****
>
>       END_CRIT_SECTION();
>
> !     LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
> !     WriteBuffer(buffer);
>
>       /*
>        * If tuple is cachable, mark it for invalidation from the caches in
> --- 1171,1177 ----
>
>       END_CRIT_SECTION();
>
> !     UnlockAndWriteBuffer(buffer);
>
>       /*
>        * If tuple is cachable, mark it for invalidation from the caches in
> ***************
> *** 1253,1260 ****
>
>       if (result == HeapTupleInvisible)
>       {
> !         LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
> !         ReleaseBuffer(buffer);
>           elog(ERROR, "attempted to delete invisible tuple");
>       }
>       else if (result == HeapTupleBeingUpdated && wait)
> --- 1249,1255 ----
>
>       if (result == HeapTupleInvisible)
>       {
> !         UnlockAndReleaseBuffer(buffer);
>           elog(ERROR, "attempted to delete invisible tuple");
>       }
>       else if (result == HeapTupleBeingUpdated && wait)
> ***************
> *** 1301,1308 ****
>                  result == HeapTupleUpdated ||
>                  result == HeapTupleBeingUpdated);
>           *ctid = tp.t_data->t_ctid;
> !         LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
> !         ReleaseBuffer(buffer);
>           return result;
>       }
>
> --- 1296,1302 ----
>                  result == HeapTupleUpdated ||
>                  result == HeapTupleBeingUpdated);
>           *ctid = tp.t_data->t_ctid;
> !         UnlockAndReleaseBuffer(buffer);
>           return result;
>       }
>
> ***************
> *** 1483,1490 ****
>
>       if (result == HeapTupleInvisible)
>       {
> !         LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
> !         ReleaseBuffer(buffer);
>           elog(ERROR, "attempted to update invisible tuple");
>       }
>       else if (result == HeapTupleBeingUpdated && wait)
> --- 1477,1483 ----
>
>       if (result == HeapTupleInvisible)
>       {
> !         UnlockAndReleaseBuffer(buffer);
>           elog(ERROR, "attempted to update invisible tuple");
>       }
>       else if (result == HeapTupleBeingUpdated && wait)
> ***************
> *** 1531,1538 ****
>                  result == HeapTupleUpdated ||
>                  result == HeapTupleBeingUpdated);
>           *ctid = oldtup.t_data->t_ctid;
> !         LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
> !         ReleaseBuffer(buffer);
>           return result;
>       }
>
> --- 1524,1530 ----
>                  result == HeapTupleUpdated ||
>                  result == HeapTupleBeingUpdated);
>           *ctid = oldtup.t_data->t_ctid;
> !         UnlockAndReleaseBuffer(buffer);
>           return result;
>       }
>
> ***************
> *** 1808,1815 ****
>
>       if (result == HeapTupleInvisible)
>       {
> !         LockBuffer(*buffer, BUFFER_LOCK_UNLOCK);
> !         ReleaseBuffer(*buffer);
>           elog(ERROR, "attempted to mark4update invisible tuple");
>       }
>       else if (result == HeapTupleBeingUpdated)
> --- 1800,1806 ----
>
>       if (result == HeapTupleInvisible)
>       {
> !         UnlockAndReleaseBuffer(*buffer);
>           elog(ERROR, "attempted to mark4update invisible tuple");
>       }
>       else if (result == HeapTupleBeingUpdated)
> Index: src/backend/access/index/indexam.c
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/backend/access/index/indexam.c,v
> retrieving revision 1.71
> diff -c -r1.71 indexam.c
> *** src/backend/access/index/indexam.c    25 Sep 2003 06:57:57 -0000    1.71
> --- src/backend/access/index/indexam.c    31 Oct 2003 23:00:00 -0000
> ***************
> *** 541,548 ****
>
>           if (sv_infomask != heapTuple->t_data->t_infomask)
>               SetBufferCommitInfoNeedsSave(scan->xs_cbuf);
> !         LockBuffer(scan->xs_cbuf, BUFFER_LOCK_UNLOCK);
> !         ReleaseBuffer(scan->xs_cbuf);
>           scan->xs_cbuf = InvalidBuffer;
>       }
>
> --- 541,547 ----
>
>           if (sv_infomask != heapTuple->t_data->t_infomask)
>               SetBufferCommitInfoNeedsSave(scan->xs_cbuf);
> !         UnlockAndReleaseBuffer(scan->xs_cbuf);
>           scan->xs_cbuf = InvalidBuffer;
>       }
>
> Index: src/backend/access/nbtree/nbtinsert.c
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/backend/access/nbtree/nbtinsert.c,v
> retrieving revision 1.106
> diff -c -r1.106 nbtinsert.c
> *** src/backend/access/nbtree/nbtinsert.c    25 Sep 2003 06:57:57 -0000    1.106
> --- src/backend/access/nbtree/nbtinsert.c    31 Oct 2003 23:00:35 -0000
> ***************
> *** 274,281 ****
>                       }
>                       if (sv_infomask != htup.t_data->t_infomask)
>                           SetBufferCommitInfoNeedsSave(hbuffer);
> !                     LockBuffer(hbuffer, BUFFER_LOCK_UNLOCK);
> !                     ReleaseBuffer(hbuffer);
>                   }
>               }
>           }
> --- 274,280 ----
>                       }
>                       if (sv_infomask != htup.t_data->t_infomask)
>                           SetBufferCommitInfoNeedsSave(hbuffer);
> !                     UnlockAndReleaseBuffer(hbuffer);
>                   }
>               }
>           }
> Index: src/backend/access/nbtree/nbtpage.c
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/backend/access/nbtree/nbtpage.c,v
> retrieving revision 1.72
> diff -c -r1.72 nbtpage.c
> *** src/backend/access/nbtree/nbtpage.c    29 Sep 2003 23:40:26 -0000    1.72
> --- src/backend/access/nbtree/nbtpage.c    31 Oct 2003 23:01:05 -0000
> ***************
> *** 502,509 ****
>   void
>   _bt_relbuf(Relation rel, Buffer buf)
>   {
> !     LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !     ReleaseBuffer(buf);
>   }
>
>   /*
> --- 502,508 ----
>   void
>   _bt_relbuf(Relation rel, Buffer buf)
>   {
> !     UnlockAndReleaseBuffer(buf);
>   }
>
>   /*
> ***************
> *** 521,528 ****
>   void
>   _bt_wrtbuf(Relation rel, Buffer buf)
>   {
> !     LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !     WriteBuffer(buf);
>   }
>
>   /*
> --- 520,526 ----
>   void
>   _bt_wrtbuf(Relation rel, Buffer buf)
>   {
> !     UnlockAndWriteBuffer(buf);
>   }
>
>   /*
> Index: src/backend/commands/sequence.c
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/backend/commands/sequence.c,v
> retrieving revision 1.103
> diff -c -r1.103 sequence.c
> *** src/backend/commands/sequence.c    25 Sep 2003 06:57:58 -0000    1.103
> --- src/backend/commands/sequence.c    31 Oct 2003 23:02:15 -0000
> ***************
> *** 291,298 ****
>
>       END_CRIT_SECTION();
>
> !     LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !     WriteBuffer(buf);
>       heap_close(rel, NoLock);
>   }
>
> --- 291,297 ----
>
>       END_CRIT_SECTION();
>
> !     UnlockAndWriteBuffer(buf);
>       heap_close(rel, NoLock);
>   }
>
> ***************
> *** 379,388 ****
>
>       END_CRIT_SECTION();
>
> !     LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !
> !     WriteBuffer(buf);
> !
>       relation_close(seqrel, NoLock);
>   }
>
> --- 378,384 ----
>
>       END_CRIT_SECTION();
>
> !     UnlockAndWriteBuffer(buf);
>       relation_close(seqrel, NoLock);
>   }
>
> ***************
> *** 583,592 ****
>
>       END_CRIT_SECTION();
>
> !     LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !
> !     WriteBuffer(buf);
> !
>       relation_close(seqrel, NoLock);
>
>       PG_RETURN_INT64(result);
> --- 579,585 ----
>
>       END_CRIT_SECTION();
>
> !     UnlockAndWriteBuffer(buf);
>       relation_close(seqrel, NoLock);
>
>       PG_RETURN_INT64(result);
> ***************
> *** 719,728 ****
>
>       END_CRIT_SECTION();
>
> !     LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !
> !     WriteBuffer(buf);
> !
>       relation_close(seqrel, NoLock);
>   }
>
> --- 712,718 ----
>
>       END_CRIT_SECTION();
>
> !     UnlockAndWriteBuffer(buf);
>       relation_close(seqrel, NoLock);
>   }
>
> Index: src/backend/commands/vacuum.c
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/backend/commands/vacuum.c,v
> retrieving revision 1.263
> diff -c -r1.263 vacuum.c
> *** src/backend/commands/vacuum.c    2 Oct 2003 23:19:44 -0000    1.263
> --- src/backend/commands/vacuum.c    31 Oct 2003 23:04:31 -0000
> ***************
> *** 2299,2306 ****
>               page = BufferGetPage(buf);
>               if (!PageIsEmpty(page))
>                   vacuum_page(onerel, buf, *curpage);
> !             LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !             WriteBuffer(buf);
>           }
>       }
>
> --- 2299,2305 ----
>               page = BufferGetPage(buf);
>               if (!PageIsEmpty(page))
>                   vacuum_page(onerel, buf, *curpage);
> !             UnlockAndWriteBuffer(buf);
>           }
>       }
>
> ***************
> *** 2356,2363 ****
>                       tuple.t_data->t_infomask |= HEAP_XMIN_INVALID;
>               }
>           }
> !         LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !         WriteBuffer(buf);
>           Assert((*curpage)->offsets_used == num_tuples);
>           checked_moved += num_tuples;
>       }
> --- 2355,2361 ----
>                       tuple.t_data->t_infomask |= HEAP_XMIN_INVALID;
>               }
>           }
> !         UnlockAndWriteBuffer(buf);
>           Assert((*curpage)->offsets_used == num_tuples);
>           checked_moved += num_tuples;
>       }
> ***************
> *** 2467,2474 ****
>
>               END_CRIT_SECTION();
>
> !             LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !             WriteBuffer(buf);
>           }
>
>           /* now - free new list of reaped pages */
> --- 2465,2471 ----
>
>               END_CRIT_SECTION();
>
> !             UnlockAndWriteBuffer(buf);
>           }
>
>           /* now - free new list of reaped pages */
> ***************
> *** 2535,2542 ****
>               buf = ReadBuffer(onerel, (*vacpage)->blkno);
>               LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
>               vacuum_page(onerel, buf, *vacpage);
> !             LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !             WriteBuffer(buf);
>           }
>       }
>
> --- 2532,2538 ----
>               buf = ReadBuffer(onerel, (*vacpage)->blkno);
>               LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
>               vacuum_page(onerel, buf, *vacpage);
> !             UnlockAndWriteBuffer(buf);
>           }
>       }
>
> Index: src/backend/commands/vacuumlazy.c
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/backend/commands/vacuumlazy.c,v
> retrieving revision 1.32
> diff -c -r1.32 vacuumlazy.c
> *** src/backend/commands/vacuumlazy.c    25 Sep 2003 06:57:59 -0000    1.32
> --- src/backend/commands/vacuumlazy.c    31 Oct 2003 23:03:10 -0000
> ***************
> *** 267,274 ****
>                   lazy_record_free_space(vacrelstats, blkno,
>                                          PageGetFreeSpace(page));
>               }
> !             LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !             WriteBuffer(buf);
>               continue;
>           }
>
> --- 267,273 ----
>                   lazy_record_free_space(vacrelstats, blkno,
>                                          PageGetFreeSpace(page));
>               }
> !             UnlockAndWriteBuffer(buf);
>               continue;
>           }
>
> ***************
> *** 277,284 ****
>               empty_pages++;
>               lazy_record_free_space(vacrelstats, blkno,
>                                      PageGetFreeSpace(page));
> !             LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !             ReleaseBuffer(buf);
>               continue;
>           }
>
> --- 276,282 ----
>               empty_pages++;
>               lazy_record_free_space(vacrelstats, blkno,
>                                      PageGetFreeSpace(page));
> !             UnlockAndReleaseBuffer(buf);
>               continue;
>           }
>
> ***************
> *** 477,484 ****
>           page = BufferGetPage(buf);
>           lazy_record_free_space(vacrelstats, tblk,
>                                  PageGetFreeSpace(page));
> !         LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !         WriteBuffer(buf);
>           npages++;
>       }
>
> --- 475,481 ----
>           page = BufferGetPage(buf);
>           lazy_record_free_space(vacrelstats, tblk,
>                                  PageGetFreeSpace(page));
> !         UnlockAndWriteBuffer(buf);
>           npages++;
>       }
>
> ***************
> *** 812,819 ****
>           if (PageIsNew(page) || PageIsEmpty(page))
>           {
>               /* PageIsNew robably shouldn't happen... */
> !             LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> !             ReleaseBuffer(buf);
>               continue;
>           }
>
> --- 809,815 ----
>           if (PageIsNew(page) || PageIsEmpty(page))
>           {
>               /* PageIsNew robably shouldn't happen... */
> !             UnlockAndReleaseBuffer(buf);
>               continue;
>           }
>
> Index: src/backend/storage/buffer/buf_init.c
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/backend/storage/buffer/buf_init.c,v
> retrieving revision 1.54
> diff -c -r1.54 buf_init.c
> *** src/backend/storage/buffer/buf_init.c    4 Aug 2003 02:40:03 -0000    1.54
> --- src/backend/storage/buffer/buf_init.c    31 Oct 2003 22:38:16 -0000
> ***************
> *** 34,50 ****
>   #include "utils/hsearch.h"
>   #include "utils/memutils.h"
>
> -
> - /*
> -  *    if BMTRACE is defined, we trace the last 200 buffer allocations and
> -  *    deallocations in a circular buffer in shared memory.
> -  */
> - #ifdef    BMTRACE
> - bmtrace    *TraceBuf;
> - long       *CurTraceBuf;
> -
> - #define BMT_LIMIT        200
> - #endif   /* BMTRACE */
>   int            ShowPinTrace = 0;
>
>   int            Data_Descriptors;
> --- 34,39 ----
> ***************
> *** 144,159 ****
>        */
>       LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
>
> - #ifdef BMTRACE
> -     CurTraceBuf = (long *) ShmemInitStruct("Buffer trace",
> -                             (BMT_LIMIT * sizeof(bmtrace)) + sizeof(long),
> -                                            &foundDescs);
> -     if (!foundDescs)
> -         MemSet(CurTraceBuf, 0, (BMT_LIMIT * sizeof(bmtrace)) + sizeof(long));
> -
> -     TraceBuf = (bmtrace *) & (CurTraceBuf[1]);
> - #endif
> -
>       BufferDescriptors = (BufferDesc *)
>           ShmemInitStruct("Buffer Descriptors",
>                         Num_Descriptors * sizeof(BufferDesc), &foundDescs);
> --- 133,138 ----
> ***************
> *** 266,274 ****
>       /* size of buffer hash table */
>       size += hash_estimate_size(NBuffers, sizeof(BufferLookupEnt));
>
> - #ifdef BMTRACE
> -     size += (BMT_LIMIT * sizeof(bmtrace)) + sizeof(long);
> - #endif
> -
>       return size;
>   }
> --- 245,249 ----
> Index: src/backend/storage/buffer/buf_table.c
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/backend/storage/buffer/buf_table.c,v
> retrieving revision 1.29
> diff -c -r1.29 buf_table.c
> *** src/backend/storage/buffer/buf_table.c    4 Aug 2003 02:40:03 -0000    1.29
> --- src/backend/storage/buffer/buf_table.c    31 Oct 2003 22:30:34 -0000
> ***************
> *** 133,147 ****
>       result->id = buf->buf_id;
>       return TRUE;
>   }
> -
> - /* prints out collision stats for the buf table */
> - #ifdef NOT_USED
> - void
> - DBG_LookupListCheck(int nlookup)
> - {
> -     nlookup = 10;
> -
> -     hash_stats("Shared", SharedBufHash);
> - }
> -
> - #endif
> --- 133,135 ----
> Index: src/backend/storage/buffer/bufmgr.c
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/backend/storage/buffer/bufmgr.c,v
> retrieving revision 1.141
> diff -c -r1.141 bufmgr.c
> *** src/backend/storage/buffer/bufmgr.c    25 Sep 2003 06:58:01 -0000    1.141
> --- src/backend/storage/buffer/bufmgr.c    31 Oct 2003 22:46:35 -0000
> ***************
> *** 105,117 ****
>    *
>    * Note: a side effect of a P_NEW call is to update reln->rd_nblocks.
>    */
> -
> - #undef ReadBuffer                /* conflicts with macro when BUFMGR_DEBUG
> -                                  * defined */
> -
> - /*
> -  * ReadBuffer
> -  */
>   Buffer
>   ReadBuffer(Relation reln, BlockNumber blockNum)
>   {
> --- 105,110 ----
> ***************
> *** 358,366 ****
>                */
>               *foundPtr = FALSE;
>           }
> - #ifdef BMTRACE
> -         _bm_trace((reln->rd_rel->relisshared ? 0 : MyDatabaseId), RelationGetRelid(reln), blockNum,
BufferDescriptorGetBuffer(buf),BMT_ALLOCFND); 
> - #endif   /* BMTRACE */
>
>           if (!(*foundPtr))
>               StartBufferIO(buf, true);
> --- 351,356 ----
> ***************
> *** 569,578 ****
>       else
>           ContinueBufferIO(buf, true);
>
> - #ifdef BMTRACE
> -     _bm_trace((reln->rd_rel->relisshared ? 0 : MyDatabaseId), RelationGetRelid(reln), blockNum,
BufferDescriptorGetBuffer(buf),BMT_ALLOCNOTFND); 
> - #endif   /* BMTRACE */
> -
>       LWLockRelease(BufMgrLock);
>
>       return buf;
> --- 559,564 ----
> ***************
> *** 619,627 ****
>    * Side Effects:
>    *        Pin count is decremented.
>    */
> -
> - #undef WriteBuffer
> -
>   void
>   WriteBuffer(Buffer buffer)
>   {
> --- 605,610 ----
> ***************
> *** 638,645 ****
>       write_buffer(buffer, false);
>   }
>
> -
> - #undef ReleaseAndReadBuffer
>   /*
>    * ReleaseAndReadBuffer -- combine ReleaseBuffer() and ReadBuffer()
>    *        to save a lock release/acquire.
> --- 621,626 ----
> ***************
> *** 1099,1105 ****
>        * new or temp, because no one else should be modifying it.  Otherwise
>        * we need to ask the smgr for the current physical file length.
>        *
> !      * Don't call smgr on a view, either.
>        */
>       if (relation->rd_rel->relkind == RELKIND_VIEW)
>           relation->rd_nblocks = 0;
> --- 1080,1086 ----
>        * new or temp, because no one else should be modifying it.  Otherwise
>        * we need to ask the smgr for the current physical file length.
>        *
> !      * Don't call smgr on a view or a composite type, either.
>        */
>       if (relation->rd_rel->relkind == RELKIND_VIEW)
>           relation->rd_nblocks = 0;
> ***************
> *** 1107,1112 ****
> --- 1088,1094 ----
>           relation->rd_nblocks = 0;
>       else if (!relation->rd_isnew && !relation->rd_istemp)
>           relation->rd_nblocks = smgrnblocks(DEFAULT_SMGR, relation);
> +
>       return relation->rd_nblocks;
>   }
>
> ***************
> *** 1555,1562 ****
>       return 0;
>   }
>
> - #undef ReleaseBuffer
> -
>   /*
>    * ReleaseBuffer -- remove the pin on a buffer without
>    *        marking it dirty.
> --- 1537,1542 ----
> ***************
> *** 1669,1819 ****
>   }
>   #endif
>
> - #ifdef BMTRACE
> -
> - /*
> -  *    trace allocations and deallocations in a circular buffer in
> -  *    shared memory.    check the buffer before doing the allocation,
> -  *    and die if there's anything fishy.
> -  */
> -
> - void
> - _bm_trace(Oid dbId, Oid relId, int blkNo, int bufNo, int allocType)
> - {
> -     long        start,
> -                 cur;
> -     bmtrace    *tb;
> -
> -     start = *CurTraceBuf;
> -
> -     if (start > 0)
> -         cur = start - 1;
> -     else
> -         cur = BMT_LIMIT - 1;
> -
> -     for (;;)
> -     {
> -         tb = &TraceBuf[cur];
> -         if (tb->bmt_op != BMT_NOTUSED)
> -         {
> -             if (tb->bmt_buf == bufNo)
> -             {
> -                 if ((tb->bmt_op == BMT_DEALLOC)
> -                     || (tb->bmt_dbid == dbId && tb->bmt_relid == relId
> -                         && tb->bmt_blkno == blkNo))
> -                     goto okay;
> -
> -                 /* die holding the buffer lock */
> -                 _bm_die(dbId, relId, blkNo, bufNo, allocType, start, cur);
> -             }
> -         }
> -
> -         if (cur == start)
> -             goto okay;
> -
> -         if (cur == 0)
> -             cur = BMT_LIMIT - 1;
> -         else
> -             cur--;
> -     }
> -
> - okay:
> -     tb = &TraceBuf[start];
> -     tb->bmt_pid = MyProcPid;
> -     tb->bmt_buf = bufNo;
> -     tb->bmt_dbid = dbId;
> -     tb->bmt_relid = relId;
> -     tb->bmt_blkno = blkNo;
> -     tb->bmt_op = allocType;
> -
> -     *CurTraceBuf = (start + 1) % BMT_LIMIT;
> - }
> -
> - void
> - _bm_die(Oid dbId, Oid relId, int blkNo, int bufNo,
> -         int allocType, long start, long cur)
> - {
> -     FILE       *fp;
> -     bmtrace    *tb;
> -     int            i;
> -
> -     tb = &TraceBuf[cur];
> -
> -     if ((fp = AllocateFile("/tmp/death_notice", "w")) == NULL)
> -         elog(FATAL, "buffer alloc trace error and can't open log file");
> -
> -     fprintf(fp, "buffer alloc trace detected the following error:\n\n");
> -     fprintf(fp, "    buffer %d being %s inconsistently with a previous %s\n\n",
> -          bufNo, (allocType == BMT_DEALLOC ? "deallocated" : "allocated"),
> -             (tb->bmt_op == BMT_DEALLOC ? "deallocation" : "allocation"));
> -
> -     fprintf(fp, "the trace buffer contains:\n");
> -
> -     i = start;
> -     for (;;)
> -     {
> -         tb = &TraceBuf[i];
> -         if (tb->bmt_op != BMT_NOTUSED)
> -         {
> -             fprintf(fp, "     [%3d]%spid %d buf %2d for <%u,%u,%u> ",
> -                     i, (i == cur ? " ---> " : "\t"),
> -                     tb->bmt_pid, tb->bmt_buf,
> -                     tb->bmt_dbid, tb->bmt_relid, tb->bmt_blkno);
> -
> -             switch (tb->bmt_op)
> -             {
> -                 case BMT_ALLOCFND:
> -                     fprintf(fp, "allocate (found)\n");
> -                     break;
> -
> -                 case BMT_ALLOCNOTFND:
> -                     fprintf(fp, "allocate (not found)\n");
> -                     break;
> -
> -                 case BMT_DEALLOC:
> -                     fprintf(fp, "deallocate\n");
> -                     break;
> -
> -                 default:
> -                     fprintf(fp, "unknown op type %d\n", tb->bmt_op);
> -                     break;
> -             }
> -         }
> -
> -         i = (i + 1) % BMT_LIMIT;
> -         if (i == start)
> -             break;
> -     }
> -
> -     fprintf(fp, "\noperation causing error:\n");
> -     fprintf(fp, "\tpid %d buf %d for <%d,%u,%d> ",
> -             getpid(), bufNo, dbId, relId, blkNo);
> -
> -     switch (allocType)
> -     {
> -         case BMT_ALLOCFND:
> -             fprintf(fp, "allocate (found)\n");
> -             break;
> -
> -         case BMT_ALLOCNOTFND:
> -             fprintf(fp, "allocate (not found)\n");
> -             break;
> -
> -         case BMT_DEALLOC:
> -             fprintf(fp, "deallocate\n");
> -             break;
> -
> -         default:
> -             fprintf(fp, "unknown op type %d\n", allocType);
> -             break;
> -     }
> -
> -     FreeFile(fp);
> -
> -     kill(getpid(), SIGILL);
> - }
> - #endif   /* BMTRACE */
> -
>   /*
>    * SetBufferCommitInfoNeedsSave
>    *
> --- 1649,1654 ----
> Index: src/backend/storage/buffer/freelist.c
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/backend/storage/buffer/freelist.c,v
> retrieving revision 1.31
> diff -c -r1.31 freelist.c
> *** src/backend/storage/buffer/freelist.c    4 Aug 2003 02:40:03 -0000    1.31
> --- src/backend/storage/buffer/freelist.c    31 Oct 2003 22:50:11 -0000
> ***************
> *** 32,40 ****
>   #include "storage/ipc.h"
>   #include "storage/proc.h"
>
> -
>   static BufferDesc *SharedFreeList;
>
>   /*
>    * State-checking macros
>    */
> --- 32,41 ----
>   #include "storage/ipc.h"
>   #include "storage/proc.h"
>
>   static BufferDesc *SharedFreeList;
>
> + #define INVALID_DESCRIPTOR (-3)
> +
>   /*
>    * State-checking macros
>    */
> ***************
> *** 65,74 ****
>   static void
>   AddBufferToFreelist(BufferDesc *bf)
>   {
> - #ifdef BMTRACE
> -     _bm_trace(bf->tag.relId.dbId, bf->tag.relId.relId, bf->tag.blockNum,
> -               BufferDescriptorGetBuffer(bf), BMT_DEALLOC);
> - #endif   /* BMTRACE */
>       IsNotInQueue(bf);
>
>       /* change bf so it points to inFrontOfNew and its successor */
> --- 66,71 ----
> Index: src/include/storage/buf_internals.h
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/include/storage/buf_internals.h,v
> retrieving revision 1.61
> diff -c -r1.61 buf_internals.h
> *** src/include/storage/buf_internals.h    4 Aug 2003 02:40:14 -0000    1.61
> --- src/include/storage/buf_internals.h    31 Oct 2003 22:37:19 -0000
> ***************
> *** 33,46 ****
>    * Flags for buffer descriptors
>    */
>   #define BM_DIRTY                (1 << 0)
> ! #define BM_PRIVATE                (1 << 1)
> ! #define BM_VALID                (1 << 2)
> ! #define BM_DELETED                (1 << 3)
> ! #define BM_FREE                    (1 << 4)
> ! #define BM_IO_IN_PROGRESS        (1 << 5)
> ! #define BM_IO_ERROR                (1 << 6)
> ! #define BM_JUST_DIRTIED            (1 << 7)
> ! #define BM_PIN_COUNT_WAITER        (1 << 8)
>
>   typedef bits16 BufFlags;
>
> --- 33,45 ----
>    * Flags for buffer descriptors
>    */
>   #define BM_DIRTY                (1 << 0)
> ! #define BM_VALID                (1 << 1)
> ! #define BM_DELETED                (1 << 2)
> ! #define BM_FREE                    (1 << 3)
> ! #define BM_IO_IN_PROGRESS        (1 << 4)
> ! #define BM_IO_ERROR                (1 << 5)
> ! #define BM_JUST_DIRTIED            (1 << 6)
> ! #define BM_PIN_COUNT_WAITER        (1 << 7)
>
>   typedef bits16 BufFlags;
>
> ***************
> *** 123,154 ****
>       Buffer        id;
>   } BufferLookupEnt;
>
> - /*
> -  *    mao tracing buffer allocation
> -  */
> -
> - /*#define BMTRACE*/
> -
> - #ifdef BMTRACE
> -
> - typedef struct _bmtrace
> - {
> -     int            bmt_pid;
> -     int            bmt_buf;
> -     Oid            bmt_dbid;
> -     Oid            bmt_relid;
> -     BlockNumber bmt_blkno;
> -     int            bmt_op;
> -
> - #define BMT_NOTUSED        0
> - #define BMT_ALLOCFND    1
> - #define BMT_ALLOCNOTFND 2
> - #define BMT_DEALLOC        3
> -
> - }    bmtrace;
> - #endif   /* BMTRACE */
> -
> -
>   /* counters in buf_init.c */
>   extern long int ReadBufferCount;
>   extern long int ReadLocalBufferCount;
> --- 122,127 ----
> Index: src/include/storage/bufmgr.h
> ===================================================================
> RCS file: /var/lib/cvs/pgsql-server/src/include/storage/bufmgr.h,v
> retrieving revision 1.70
> diff -c -r1.70 bufmgr.h
> *** src/include/storage/bufmgr.h    10 Aug 2003 19:48:08 -0000    1.70
> --- src/include/storage/bufmgr.h    31 Oct 2003 22:50:16 -0000
> ***************
> *** 52,58 ****
>    */
>
>   #define BAD_BUFFER_ID(bid) ((bid) < 1 || (bid) > NBuffers)
> - #define INVALID_DESCRIPTOR (-3)
>
>   #define UnlockAndReleaseBuffer(buffer)    \
>   ( \
> --- 52,57 ----
>
>
> ------------------------------------------------------------------------
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
>       subscribe-nomail command to majordomo@postgresql.org so that your
>       message can get through to the mailing list cleanly


--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #


Re: bufmgr code cleanup

From
Tom Lane
Date:
Jan Wieck <JanWieck@Yahoo.com> writes:
> Neil Conway wrote:
>> This patch cleans up some of the bufmgr code:

> Can this be held off a little while we're experimenting with
> improvements to the buffer algorithms?

I do not actually agree with the "UnlockAndReleaseBuffer" changes
anyway.  I think this obscures the code by making resource grabbing
and resource releasing code unsymmetrical, not to mention incompatible
with code branches where the unlock and the buffer release can't be
merged because other things are done in between.

As for removing the BM_TRACE code, what's broken about it?  Shouldn't we
be more interested in fixing it than removing it?

            regards, tom lane

Re: bufmgr code cleanup

From
Neil Conway
Date:
On Mon, 2003-11-03 at 10:00, Tom Lane wrote:
> I do not actually agree with the "UnlockAndReleaseBuffer" changes
> anyway.  I think this obscures the code by making resource grabbing
> and resource releasing code unsymmetrical

Hmm... fair enough, I see your point. In that case, should I remove the
UnlockAndReleaseBuffer macro and change all the places that use it to
just do a LockBuffer() followed by ReleaseBuffer()?

(Similarly for UnlockAndWriteBuffer())

> not to mention incompatible
> with code branches where the unlock and the buffer release can't be
> merged because other things are done in between.

This makes no sense to me: the macro is inapplicable in this case, but I
don't see how this makes anything "incompatible". Can you elaborate?

> As for removing the BM_TRACE code, what's broken about it?  Shouldn't we
> be more interested in fixing it than removing it?

Well, it doesn't compile, and probably hasn't for years, so I took that
as a sign that there wasn't much interest in it. We need proper buffer
tracing, but if anyone wants to implement that, the first thing they'd
need to do is rip out the existing, broken BMTRACE code. In the unlikely
event that someone would like to resurrect it or use it for reference,
that's what CVS is for...

-Neil



Re: bufmgr code cleanup

From
Tom Lane
Date:
Neil Conway <neilc@samurai.com> writes:
> On Mon, 2003-11-03 at 10:00, Tom Lane wrote:
>> I do not actually agree with the "UnlockAndReleaseBuffer" changes
>> anyway.  I think this obscures the code by making resource grabbing
>> and resource releasing code unsymmetrical

> Hmm... fair enough, I see your point. In that case, should I remove the
> UnlockAndReleaseBuffer macro and change all the places that use it to
> just do a LockBuffer() followed by ReleaseBuffer()?

Didn't realize we had one, actually.  Wonder when it was put in?  I
agree with removing it --- seems like it obscures the code to little
purpose.

>> not to mention incompatible
>> with code branches where the unlock and the buffer release can't be
>> merged because other things are done in between.

> This makes no sense to me: the macro is inapplicable in this case, but I
> don't see how this makes anything "incompatible". Can you elaborate?

Incompatible in the sense that it looks different.  If you see in one
place

    foo = ReadBuffer(...);
    do some stuff;
    LockBuffer(foo, LOCK);
    do some stuff;
    LockBuffer(foo, UNLOCK);
    do some stuff;
    ReleaseBuffer(foo);

then it's reasonably apparent that the LockBuffers pair together and so
do ReadBuffer/ReleaseBuffer.  If other places replace this pattern by

    foo = ReadBuffer(...);
    do some stuff;
    LockBuffer(foo, LOCK);
    do some stuff;
    UnLockAndReleaseBuffer(foo);

it looks visually different and the pairing of operations is lost.
ISTM this adds potential for confusion without really buying anything.

One could argue that LockBuffer(foo, UNLOCK) ought to be written
UnLockBuffer(foo) for symmetry with ReleaseBuffer.  I'm not sure if
there are any places that really depend on the way it's coded now...


>> As for removing the BM_TRACE code, what's broken about it?

> Well, it doesn't compile, and probably hasn't for years, so I took that
> as a sign that there wasn't much interest in it.

Ah.  Fair point I guess ...

            regards, tom lane