Code review for ARC patch. Eliminate static variables, improve handling

of VACUUM cases so that VACUUM requests don't affect the ARC state at all,
avoid corner case where BufferSync would uselessly rewrite a buffer that
no longer contains the page that was to be flushed.  Make some minor
other cleanups in and around the bufmgr as well, such as moving PinBuffer
and UnpinBuffer into bufmgr.c where they really belong.

src/backend/storage/buffer/buf_init.c

@@ -8,35 +8,15 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/storage/buffer/buf_init.c,v 1.62 2004/02/12 15:06:56 wieck Exp $
+ *	  $PostgreSQL: pgsql/src/backend/storage/buffer/buf_init.c,v 1.63 2004/04/19 23:27:17 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
 #include "postgres.h"
 
-#include <sys/file.h>
-#include <math.h>
-#include <signal.h>
-
-#include "catalog/catalog.h"
-#include "executor/execdebug.h"
-#include "miscadmin.h"
-#include "storage/buf.h"
-#include "storage/buf_internals.h"
 #include "storage/bufmgr.h"
-#include "storage/fd.h"
-#include "storage/ipc.h"
-#include "storage/lmgr.h"
-#include "storage/shmem.h"
-#include "storage/smgr.h"
-#include "storage/lwlock.h"
-#include "utils/builtins.h"
-#include "utils/hsearch.h"
-#include "utils/memutils.h"
-
-int			ShowPinTrace = 0;
+#include "storage/buf_internals.h"
 
-int			Data_Descriptors;
 
 BufferDesc *BufferDescriptors;
 Block	   *BufferBlockPointers;
@@ -44,6 +24,14 @@ Block	   *BufferBlockPointers;
 long	   *PrivateRefCount;	/* also used in freelist.c */
 bits8	   *BufferLocks;		/* flag bits showing locks I have set */
 
+/* statistics counters */
+long int	ReadBufferCount;
+long int	ReadLocalBufferCount;
+long int	BufferHitCount;
+long int	LocalBufferHitCount;
+long int	BufferFlushCount;
+long int	LocalBufferFlushCount;
+
 
 /*
  * Data Structures:
@@ -61,48 +49,35 @@ bits8	   *BufferLocks;		/* flag bits showing locks I have set */
  *		see freelist.c.  A buffer cannot be replaced while in
  *		use either by data manager or during IO.
  *
- * WriteBufferBack:
- *		currently, a buffer is only written back at the time
- *		it is selected for replacement.  It should
- *		be done sooner if possible to reduce latency of
- *		BufferAlloc().	Maybe there should be a daemon process.
  *
  * Synchronization/Locking:
  *
  * BufMgrLock lock -- must be acquired before manipulating the
- *		buffer queues (lookup/freelist).  Must be released
+ *		buffer search datastructures (lookup/freelist, as well as the
+ *		flag bits of any buffer).  Must be released
  *		before exit and before doing any IO.
  *
  * IO_IN_PROGRESS -- this is a flag in the buffer descriptor.
  *		It must be set when an IO is initiated and cleared at
- *		the end of	the IO.  It is there to make sure that one
+ *		the end of the IO.  It is there to make sure that one
  *		process doesn't start to use a buffer while another is
  *		faulting it in.  see IOWait/IOSignal.
  *
- * refcount --	A buffer is pinned during IO and immediately
- *		after a BufferAlloc().	A buffer is always either pinned
- *		or on the freelist but never both.	The buffer must be
- *		released, written, or flushed before the end of
- *		transaction.
+ * refcount --	Counts the number of processes holding pins on a buffer.
+ *		A buffer is pinned during IO and immediately after a BufferAlloc().
+ *		Pins must be released before end of transaction.
  *
- * PrivateRefCount -- Each buffer also has a private refcount the keeps
+ * PrivateRefCount -- Each buffer also has a private refcount that keeps
  *		track of the number of times the buffer is pinned in the current
- *		processes.	This is used for two purposes, first, if we pin a
+ *		process.	This is used for two purposes: first, if we pin a
  *		a buffer more than once, we only need to change the shared refcount
- *		once, thus only lock the buffer pool once, second, when a transaction
+ *		once, thus only lock the shared state once; second, when a transaction
  *		aborts, it should only unpin the buffers exactly the number of times it
  *		has pinned them, so that it will not blow away buffers of another
  *		backend.
  *
  */
 
-long int	ReadBufferCount;
-long int	ReadLocalBufferCount;
-long int	BufferHitCount;
-long int	LocalBufferHitCount;
-long int	BufferFlushCount;
-long int	LocalBufferFlushCount;
-
 
 /*
  * Initialize shared buffer pool
@@ -118,8 +93,6 @@ InitBufferPool(void)
 				foundDescs;
 	int			i;
 
-	Data_Descriptors = NBuffers;
-
 	/*
 	 * It's probably not really necessary to grab the lock --- if there's
 	 * anyone else attached to the shmem at this point, we've got
@@ -131,7 +104,7 @@ InitBufferPool(void)
 
 	BufferDescriptors = (BufferDesc *)
 		ShmemInitStruct("Buffer Descriptors",
-					  Data_Descriptors * sizeof(BufferDesc), &foundDescs);
+						NBuffers * sizeof(BufferDesc), &foundDescs);
 
 	BufferBlocks = (char *)
 		ShmemInitStruct("Buffer Blocks",
@@ -152,9 +125,9 @@ InitBufferPool(void)
 
 		/*
 		 * link the buffers into a single linked list. This will become the
-		 * LiFo list of unused buffers returned by StragegyGetBuffer().
+		 * LIFO list of unused buffers returned by StrategyGetBuffer().
 		 */
-		for (i = 0; i < Data_Descriptors; block += BLCKSZ, buf++, i++)
+		for (i = 0; i < NBuffers; block += BLCKSZ, buf++, i++)
 		{
 			Assert(ShmemIsValid((unsigned long) block));
 
@@ -173,7 +146,7 @@ InitBufferPool(void)
 		}
 
 		/* Correct last entry */
-		BufferDescriptors[Data_Descriptors - 1].bufNext = -1;
+		BufferDescriptors[NBuffers - 1].bufNext = -1;
 	}
 
 	/* Init other shared buffer-management stuff */
@@ -215,35 +188,31 @@ InitBufferPoolAccess(void)
 		BufferBlockPointers[i] = (Block) MAKE_PTR(BufferDescriptors[i].data);
 }
 
-/* -----------------------------------------------------
+/*
  * BufferShmemSize
  *
  * compute the size of shared memory for the buffer pool including
  * data pages, buffer descriptors, hash tables, etc.
- * ----------------------------------------------------
  */
 int
 BufferShmemSize(void)
 {
 	int			size = 0;
 
-	/* size of shmem index hash table */
-	size += hash_estimate_size(SHMEM_INDEX_SIZE, sizeof(ShmemIndexEnt));
-
 	/* size of buffer descriptors */
 	size += MAXALIGN(NBuffers * sizeof(BufferDesc));
 
-	/* size of the shared replacement strategy control block */
-	size += MAXALIGN(sizeof(BufferStrategyControl));
-
-	/* size of the ARC directory blocks */
-	size += MAXALIGN(NBuffers * 2 * sizeof(BufferStrategyCDB));
-
 	/* size of data pages */
 	size += NBuffers * MAXALIGN(BLCKSZ);
 
 	/* size of buffer hash table */
 	size += hash_estimate_size(NBuffers * 2, sizeof(BufferLookupEnt));
 
+	/* size of the shared replacement strategy control block */
+	size += MAXALIGN(sizeof(BufferStrategyControl));
+
+	/* size of the ARC directory blocks */
+	size += MAXALIGN(NBuffers * 2 * sizeof(BufferStrategyCDB));
+
 	return size;
 }

src/backend/storage/buffer/buf_table.c

@@ -3,46 +3,42 @@
  * buf_table.c
  *	  routines for finding buffers in the buffer pool.
  *
+ * NOTE: these days, what this table actually provides is a mapping from
+ * BufferTags to CDB indexes, not directly to buffers.  The function names
+ * are thus slight misnomers.
+ *
+ * Note: all routines in this file assume that the BufMgrLock is held
+ * by the caller, so no synchronization is needed.
+ *
+ *
  * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/storage/buffer/buf_table.c,v 1.34 2003/12/14 00:34:47 neilc Exp $
+ *	  $PostgreSQL: pgsql/src/backend/storage/buffer/buf_table.c,v 1.35 2004/04/19 23:27:17 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
-/*
- * OLD COMMENTS
- *
- * Data Structures:
- *
- *		Buffers are identified by their BufferTag (buf.h).	This
- * file contains routines for allocating a shmem hash table to
- * map buffer tags to buffer descriptors.
- *
- * Synchronization:
- *
- *	All routines in this file assume BufMgrLock is held by their caller.
- */
-
 #include "postgres.h"
 
 #include "storage/buf_internals.h"
 #include "storage/bufmgr.h"
 
+
 static HTAB *SharedBufHash;
 
 
 /*
  * Initialize shmem hash table for mapping buffers
+ *		size is the desired hash table size (2*NBuffers for ARC algorithm)
  */
 void
 InitBufTable(int size)
 {
 	HASHCTL		info;
 
-	/* assume lock is held */
+	/* assume no locking is needed yet */
 
 	/* BufferTag maps to Buffer */
 	info.keysize = sizeof(BufferTag);
@@ -60,6 +56,7 @@ InitBufTable(int size)
 
 /*
  * BufTableLookup
+ *		Lookup the given BufferTag; return CDB index, or -1 if not found
  */
 int
 BufTableLookup(BufferTag *tagPtr)
@@ -78,10 +75,11 @@ BufTableLookup(BufferTag *tagPtr)
 }
 
 /*
- * BufTableDelete
+ * BufTableInsert
+ *		Insert a hashtable entry for given tag and CDB index
  */
-bool
-BufTableInsert(BufferTag *tagPtr, Buffer buf_id)
+void
+BufTableInsert(BufferTag *tagPtr, int cdb_id)
 {
 	BufferLookupEnt *result;
 	bool		found;
@@ -97,14 +95,14 @@ BufTableInsert(BufferTag *tagPtr, Buffer buf_id)
 	if (found)					/* found something else in the table? */
 		elog(ERROR, "shared buffer hash table corrupted");
 
-	result->id = buf_id;
-	return TRUE;
+	result->id = cdb_id;
 }
 
 /*
  * BufTableDelete
+ *		Delete the hashtable entry for given tag
  */
-bool
+void
 BufTableDelete(BufferTag *tagPtr)
 {
 	BufferLookupEnt *result;
@@ -114,6 +112,4 @@ BufTableDelete(BufferTag *tagPtr)
 
 	if (!result)				/* shouldn't happen */
 		elog(ERROR, "shared buffer hash table corrupted");
-
-	return TRUE;
 }

src/backend/storage/ipc/ipci.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/storage/ipc/ipci.c,v 1.65 2004/02/25 19:41:22 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/storage/ipc/ipci.c,v 1.66 2004/04/19 23:27:17 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -60,7 +60,8 @@ CreateSharedMemoryAndSemaphores(bool makePrivate,
 		 * moderately-accurate estimates for the big hogs, plus 100K for the
 		 * stuff that's too small to bother with estimating.
 		 */
-		size = BufferShmemSize();
+		size = hash_estimate_size(SHMEM_INDEX_SIZE, sizeof(ShmemIndexEnt));
+		size += BufferShmemSize();
 		size += LockShmemSize(maxBackends);
 		size += XLOGShmemSize();
 		size += CLOGShmemSize();

src/include/storage/buf_internals.h

@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/storage/buf_internals.h,v 1.68 2004/02/12 15:06:56 wieck Exp $
+ * $PostgreSQL: pgsql/src/include/storage/buf_internals.h,v 1.69 2004/04/19 23:27:17 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -21,15 +21,6 @@
 #include "storage/lwlock.h"
 
 
-/* Buf Mgr constants */
-/* in bufmgr.c */
-extern int	Data_Descriptors;
-extern int	Free_List_Descriptor;
-extern int	Lookup_List_Descriptor;
-extern int	Num_Descriptors;
-
-extern int	ShowPinTrace;
-
 /*
  * Flags for buffer descriptors
  */
@@ -51,10 +42,13 @@ typedef bits16 BufFlags;
  * that the backend flushing the buffer doesn't even believe the relation is
  * visible yet (its xact may have started before the xact that created the
  * rel).  The storage manager must be able to cope anyway.
+ *
+ * Note: if there's any pad bytes in the struct, INIT_BUFFERTAG will have
+ * to be fixed to zero them, since this struct is used as a hash key.
  */
 typedef struct buftag
 {
-	RelFileNode rnode;
+	RelFileNode rnode;			/* physical relation identifier */
 	BlockNumber blockNum;		/* blknum relative to begin of reln */
 } BufferTag;
 
@@ -71,12 +65,6 @@ typedef struct buftag
 	(a)->rnode = (xx_reln)->rd_node \
 )
 
-#define BUFFERTAG_EQUALS(a,xx_reln,xx_blockNum) \
-( \
-	(a)->rnode.tblNode == (xx_reln)->rd_node.tblNode && \
-	(a)->rnode.relNode == (xx_reln)->rd_node.relNode && \
-	(a)->blockNum == (xx_blockNum) \
-)
 #define BUFFERTAGS_EQUAL(a,b) \
 ( \
 	(a)->rnode.tblNode == (b)->rnode.tblNode && \
@@ -93,7 +81,7 @@ typedef struct sbufdesc
 	Buffer		bufNext;		/* link in freelist chain */
 	SHMEM_OFFSET data;			/* pointer to data in buf pool */
 
-	/* tag and id must be together for table lookup */
+	/* tag and id must be together for table lookup (still true?) */
 	BufferTag	tag;			/* file/block identifier */
 	int			buf_id;			/* buffer's index number (from 0) */
 
@@ -108,7 +96,7 @@ typedef struct sbufdesc
 	/*
 	 * We can't physically remove items from a disk page if another
 	 * backend has the buffer pinned.  Hence, a backend may need to wait
-	 * for all other pins to go away.  This is signaled by setting its own
+	 * for all other pins to go away.  This is signaled by storing its own
 	 * backend ID into wait_backend_id and setting flag bit
 	 * BM_PIN_COUNT_WAITER. At present, there can be only one such waiter
 	 * per buffer.
@@ -128,17 +116,17 @@ typedef struct sbufdesc
 #define BL_IO_IN_PROGRESS	(1 << 0)	/* unimplemented */
 #define BL_PIN_COUNT_LOCK	(1 << 1)
 
-/* entry for buffer hashtable */
+/* entry for buffer lookup hashtable */
 typedef struct
 {
-	BufferTag	key;
-	Buffer		id;
+	BufferTag	key;			/* Tag of a disk page */
+	int			id;				/* CDB id of associated CDB */
 } BufferLookupEnt;
 
 /*
  * Definitions for the buffer replacement strategy
  */
-#define STRAT_LIST_UNUSED	-1
+#define STRAT_LIST_UNUSED	(-1)
 #define STRAT_LIST_B1		0
 #define STRAT_LIST_T1		1
 #define STRAT_LIST_T2		2
@@ -150,12 +138,13 @@ typedef struct
  */
 typedef struct
 {
-	int				prev;		/* links in the queue */
+	int				prev;		/* list links */
 	int				next;
-	int				list;		/* current list */
-	BufferTag		buf_tag;	/* buffer key */
-	Buffer			buf_id;		/* currently assigned data buffer */
+	short			list;		/* ID of list it is currently in */
+	bool			t1_vacuum;	/* t => present only because of VACUUM */
 	TransactionId	t1_xid;		/* the xid this entry went onto T1 */
+	BufferTag		buf_tag;	/* page identifier */
+	int				buf_id;		/* currently assigned data buffer, or -1 */
 } BufferStrategyCDB;
 
 /*
@@ -163,7 +152,6 @@ typedef struct
  */
 typedef struct
 {
-
 	int		target_T1_size;				/* What T1 size are we aiming for */
 	int		listUnusedCDB;				/* All unused StrategyCDB */
 	int		listHead[STRAT_NUM_LISTS];	/* ARC lists B1, T1, T2 and B2 */
@@ -175,8 +163,10 @@ typedef struct
 	long	num_hit[STRAT_NUM_LISTS];
 	time_t	stat_report;
 
-	BufferStrategyCDB	cdb[1];			/* The cache directory */
+	/* Array of CDB's starts here */
+	BufferStrategyCDB	cdb[1];			/* VARIABLE SIZE ARRAY */
 } BufferStrategyControl;
+
  
 /* counters in buf_init.c */
 extern long int ReadBufferCount;
@@ -191,24 +181,25 @@ extern long int LocalBufferFlushCount;
  * Bufmgr Interface:
  */
 
-/* Internal routines: only called by buf.c */
+/* Internal routines: only called by bufmgr */
 
-/*freelist.c*/
-extern void PinBuffer(BufferDesc *buf);
-extern void UnpinBuffer(BufferDesc *buf);
-extern BufferDesc *StrategyBufferLookup(BufferTag *tagPtr, bool recheck);
-extern BufferDesc *StrategyGetBuffer(void);
-extern void StrategyReplaceBuffer(BufferDesc *buf, Relation rnode, BlockNumber blockNum);
+/* freelist.c */
+extern BufferDesc *StrategyBufferLookup(BufferTag *tagPtr, bool recheck,
+										int *cdb_found_index);
+extern BufferDesc *StrategyGetBuffer(int *cdb_replace_index);
+extern void StrategyReplaceBuffer(BufferDesc *buf, BufferTag *newTag,
+								  int cdb_found_index, int cdb_replace_index);
 extern void StrategyInvalidateBuffer(BufferDesc *buf);
 extern void StrategyHintVacuum(bool vacuum_active);
-extern int StrategyDirtyBufferList(int *buffer_dirty, int max_buffers);
+extern int StrategyDirtyBufferList(BufferDesc **buffers, BufferTag *buftags,
+								   int max_buffers);
 extern void StrategyInitialize(bool init);
 
 /* buf_table.c */
 extern void InitBufTable(int size);
 extern int BufTableLookup(BufferTag *tagPtr);
-extern bool BufTableInsert(BufferTag *tagPtr, Buffer buf_id);
-extern bool BufTableDelete(BufferTag *tagPtr);
+extern void BufTableInsert(BufferTag *tagPtr, int cdb_id);
+extern void BufTableDelete(BufferTag *tagPtr);
 
 /* bufmgr.c */
 extern BufferDesc *BufferDescriptors;