Code review for FILLFACTOR patch. Change WITH grammar as per earlier

discussion (including making def_arg allow reserved words), add missed
opt_definition for UNIQUE case.  Put the reloptions support code in a less
random place (I chose to make a new file access/common/reloptions.c).
Eliminate header inclusion creep.  Make the index options functions safely
user-callable (seems like client apps might like to be able to test validity
of options before trying to make an index).  Reduce overhead for normal case
with no options by allowing rd_options to be NULL.  Fix some unmaintainably
klugy code, including getting rid of Natts_pg_class_fixed at long last.
Some stylistic cleanup too, and pay attention to keeping comments in sync
with code.

Documentation still needs work, though I did fix the omissions in
catalogs.sgml and indexam.sgml.

7 files changed, 62 insertions, 49 deletions

diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index c7ebae9931..9a8828a33a 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -7,14 +7,13 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/access/genam.h,v 1.61 2006/07/02 02:23:22 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/access/genam.h,v 1.62 2006/07/03 22:45:39 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
 #ifndef GENAM_H
 #define GENAM_H
 
-#include "access/heapam.h"
 #include "access/itup.h"
 #include "access/relscan.h"
 #include "access/sdir.h"
@@ -145,13 +144,4 @@ extern SysScanDesc systable_beginscan(Relation heapRelation,
 extern HeapTuple systable_getnext(SysScanDesc sysscan);
 extern void systable_endscan(SysScanDesc sysscan);
 
-
-typedef HeapOption	IndexOption;
-
-extern bytea *genam_option(ArrayType *options,
-                           int minFillfactor, int defaultFillfactor);
-
-#define IndexGetFillFactor(relation)	HeapGetFillFactor(relation)
-#define IndexGetPageFreeSpace(relation)	HeapGetPageFreeSpace(relation)
-
 #endif   /* GENAM_H */
diff --git a/src/include/access/gin.h b/src/include/access/gin.h
index 4749eca192..de3100c4ca 100644
--- a/src/include/access/gin.h
+++ b/src/include/access/gin.h
@@ -3,7 +3,7 @@
  *    header file for postgres inverted index access method implementation.
  *
  *  Copyright (c) 2006, PostgreSQL Global Development Group
- *  $PostgreSQL: pgsql/src/include/access/gin.h,v 1.2 2006/07/02 02:23:22 momjian Exp $
+ *  $PostgreSQL: pgsql/src/include/access/gin.h,v 1.3 2006/07/03 22:45:39 tgl Exp $
  *--------------------------------------------------------------------------
  */
 
@@ -213,7 +213,7 @@ typedef struct ginxlogDeletePage {
 } ginxlogDeletePage;
 
 /* ginutil.c */
-extern Datum ginoption(PG_FUNCTION_ARGS);
+extern Datum ginoptions(PG_FUNCTION_ARGS);
 extern void initGinState( GinState *state, Relation index );
 extern Buffer GinNewBuffer(Relation index);
 extern void GinInitBuffer(Buffer b, uint32 f);
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 5be99712b0..bf9dd11e21 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/access/gist_private.h,v 1.19 2006/07/02 02:23:22 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/access/gist_private.h,v 1.20 2006/07/03 22:45:39 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -272,7 +272,10 @@ extern Datum gistgetmulti(PG_FUNCTION_ARGS);
 #define GiSTPageSize   \
     ( BLCKSZ - SizeOfPageHeaderData - MAXALIGN(sizeof(GISTPageOpaqueData)) ) 
 
-extern Datum gistoption(PG_FUNCTION_ARGS);
+#define GIST_MIN_FILLFACTOR			50
+#define GIST_DEFAULT_FILLFACTOR		90
+
+extern Datum gistoptions(PG_FUNCTION_ARGS);
 extern bool gistfitpage(IndexTuple *itvec, int len);
 extern bool gistnospace(Page page, IndexTuple *itvec, int len, OffsetNumber todelete, Size freespace);
 extern void gistcheckpage(Relation rel, Buffer buf);
@@ -330,4 +333,5 @@ extern void gistSplitByKey(Relation r, Page page, IndexTuple *itup,
 							int len, GISTSTATE *giststate,
 							GistSplitVector *v, GistEntryVector *entryvec, 
 							int attno);
+
 #endif   /* GIST_PRIVATE_H */
diff --git a/src/include/access/hash.h b/src/include/access/hash.h
index 59cf5c9961..10646522d9 100644
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/access/hash.h,v 1.70 2006/07/02 02:23:22 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/access/hash.h,v 1.71 2006/07/03 22:45:39 tgl Exp $
  *
  * NOTES
  *		modeled after Margo Seltzer's hash implementation for unix.
@@ -167,6 +167,9 @@ typedef HashMetaPageData *HashMetaPage;
 	 MAXALIGN(sizeof(HashPageOpaqueData)) - \
 	 sizeof(ItemIdData))
 
+#define HASH_MIN_FILLFACTOR			50
+#define HASH_DEFAULT_FILLFACTOR		75
+
 /*
  * Constants
  */
@@ -234,7 +237,7 @@ extern Datum hashmarkpos(PG_FUNCTION_ARGS);
 extern Datum hashrestrpos(PG_FUNCTION_ARGS);
 extern Datum hashbulkdelete(PG_FUNCTION_ARGS);
 extern Datum hashvacuumcleanup(PG_FUNCTION_ARGS);
-extern Datum hashoption(PG_FUNCTION_ARGS);
+extern Datum hashoptions(PG_FUNCTION_ARGS);
 
 /*
  * Datatype-specific hash functions in hashfunc.c.
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 085501be7d..a2ca20bddd 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/access/heapam.h,v 1.113 2006/07/02 02:23:22 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/access/heapam.h,v 1.114 2006/07/03 22:45:39 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -22,7 +22,6 @@
 #include "nodes/primnodes.h"
 #include "storage/block.h"
 #include "storage/lmgr.h"
-#include "utils/array.h"
 #include "utils/rel.h"
 #include "utils/tqual.h"
 
@@ -228,32 +227,4 @@ extern MinimalTuple minimal_tuple_from_heap_tuple(HeapTuple htup);
 extern HeapTuple heap_addheader(int natts, bool withoid,
 			   Size structlen, void *structure);
 
-extern HeapTuple build_class_tuple(Form_pg_class pgclass, ArrayType *options);
-
-/*
- * HeapOption
- *	Internal data of heaps.
- */
-typedef struct HeapOption
-{
-	int32	vl_len;
-	int		fillfactor;
-} HeapOption;
-
-extern bytea *heap_option(char relkind, ArrayType *options);
-
-/*
- * HeapGetFillFactor
- *		Returns the heap's fillfactor.
- */
-#define HeapGetFillFactor(relation) \
-	(((HeapOption*)(relation)->rd_options)->fillfactor)
-
-/*
- * HeapGetPageFreeSpace
- *		Returns the heap's freespace per page in bytes.
- */
-#define HeapGetPageFreeSpace(relation) \
-	(BLCKSZ * (100 - HeapGetFillFactor(relation)) / 100)
-
 #endif   /* HEAPAM_H */
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index e5981d092c..43bd49a7bb 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/access/nbtree.h,v 1.99 2006/07/02 02:23:22 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/access/nbtree.h,v 1.100 2006/07/03 22:45:39 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -107,6 +107,13 @@ typedef struct BTMetaPageData
 	  MAXALIGN(sizeof(BTPageOpaqueData))) / 3 - sizeof(ItemIdData))
 
 /*
+ * Because of above, min fillfactor can't be less than 2/3rds; see notes in
+ * nbtsort.c before you change these!
+ */
+#define BTREE_MIN_FILLFACTOR		70
+#define BTREE_DEFAULT_FILLFACTOR	90
+
+/*
  *	Test whether two btree entries are "the same".
  *
  *	Old comments:
@@ -453,7 +460,7 @@ extern Datum btmarkpos(PG_FUNCTION_ARGS);
 extern Datum btrestrpos(PG_FUNCTION_ARGS);
 extern Datum btbulkdelete(PG_FUNCTION_ARGS);
 extern Datum btvacuumcleanup(PG_FUNCTION_ARGS);
-extern Datum btoption(PG_FUNCTION_ARGS);
+extern Datum btoptions(PG_FUNCTION_ARGS);
 
 /*
  * prototypes for functions in nbtinsert.c
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
new file mode 100644
index 0000000000..82474f4707
--- /dev/null
+++ b/src/include/access/reloptions.h
@@ -0,0 +1,38 @@
+/*-------------------------------------------------------------------------
+ *
+ * reloptions.h
+ *	  Core support for relation options (pg_class.reloptions)
+ *
+ * Note: the functions dealing with text-array reloptions values declare
+ * them as Datum, not ArrayType *, to avoid needing to include array.h
+ * into a lot of low-level code.
+ *
+ *
+ * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * $PostgreSQL: pgsql/src/include/access/reloptions.h,v 1.1 2006/07/03 22:45:40 tgl Exp $
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef RELOPTIONS_H
+#define RELOPTIONS_H
+
+#include "nodes/pg_list.h"
+
+extern Datum transformRelOptions(Datum oldOptions, List *defList,
+								 bool ignoreOids, bool isReset);
+
+extern void parseRelOptions(Datum options, int numkeywords,
+							const char * const *keywords,
+							char **values, bool validate);
+
+extern bytea *default_reloptions(Datum reloptions, bool validate,
+								 int minFillfactor, int defaultFillfactor);
+
+extern bytea *heap_reloptions(char relkind, Datum reloptions, bool validate);
+
+extern bytea *index_reloptions(RegProcedure amoptions, Datum reloptions,
+							   bool validate);
+
+#endif   /* RELOPTIONS_H */