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.

15 files changed, 453 insertions, 271 deletions

diff --git a/src/backend/access/common/Makefile b/src/backend/access/common/Makefile
index 7cd4a4ab0e7..103bae2f5b5 100644
--- a/src/backend/access/common/Makefile
+++ b/src/backend/access/common/Makefile
@@ -4,7 +4,7 @@
 #    Makefile for access/common
 #
 # IDENTIFICATION
-#    $PostgreSQL: pgsql/src/backend/access/common/Makefile,v 1.21 2006/01/14 22:03:35 tgl Exp $
+#    $PostgreSQL: pgsql/src/backend/access/common/Makefile,v 1.22 2006/07/03 22:45:36 tgl Exp $
 #
 #-------------------------------------------------------------------------
 
@@ -12,7 +12,7 @@ subdir = src/backend/access/common
 top_builddir = ../../../..
 include $(top_builddir)/src/Makefile.global
 
-OBJS = heaptuple.o indextuple.o printtup.o scankey.o tupdesc.o  
+OBJS = heaptuple.o indextuple.o printtup.o reloptions.o scankey.o tupdesc.o  
 
 all: SUBSYS.o
 
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index edd90c8a56f..70e8fd948dd 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -16,7 +16,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/common/heaptuple.c,v 1.108 2006/07/02 02:23:18 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/common/heaptuple.c,v 1.109 2006/07/03 22:45:36 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1694,9 +1694,8 @@ minimal_tuple_from_heap_tuple(HeapTuple htup)
  * presumed to contain no null fields and no varlena fields.
  *
  * This routine is really only useful for certain system tables that are
- * known to be fixed-width and null-free.  It is used in some places for
- * pg_class, but that is a gross hack (it only works because relacl can
- * be omitted from the tuple entirely in those places).
+ * known to be fixed-width and null-free.  Currently it is only used for
+ * pg_attribute tuples.
  * ----------------
  */
 HeapTuple
@@ -1738,54 +1737,3 @@ heap_addheader(int natts,		/* max domain index */
 
 	return tuple;
 }
-
-/*
- * build_class_tuple
- *
- *	XXX Natts_pg_class_fixed is a hack - see pg_class.h
- */
-HeapTuple
-build_class_tuple(Form_pg_class pgclass, ArrayType *options)
-{
-	HeapTuple		tuple;
-	HeapTupleHeader	td;
-	Form_pg_class	data;	/* contents of tuple */
-	Size			len;
-	Size			size;
-	int				hoff;
-
-	/* size of pg_class tuple with options */
-	if (options)
-		size = offsetof(FormData_pg_class, reloptions) + VARATT_SIZE(options);
-	else
-		size = CLASS_TUPLE_SIZE;
-
-	/* header needs no null bitmap */
-	hoff = offsetof(HeapTupleHeaderData, t_bits);
-	hoff += sizeof(Oid);
-	hoff = MAXALIGN(hoff);
-	len = hoff + size;
-
-	tuple = (HeapTuple) palloc0(HEAPTUPLESIZE + len);
-	tuple->t_data = td = (HeapTupleHeader) ((char *) tuple + HEAPTUPLESIZE);
-
-	tuple->t_len = len;
-	ItemPointerSetInvalid(&(tuple->t_self));
-	tuple->t_tableOid = InvalidOid;
-
-	/* we don't bother to fill the Datum fields */
-
-	td->t_natts = Natts_pg_class_fixed;
-	td->t_hoff = hoff;
-	td->t_infomask = HEAP_HASOID;
-
-	data = (Form_pg_class) ((char *) td + hoff);
-	memcpy(data, pgclass, CLASS_TUPLE_SIZE);
-	if (options)
-	{
-		td->t_natts++;
-		memcpy(data->reloptions, options, VARATT_SIZE(options));
-	}
-
-	return tuple;
-}
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
new file mode 100644
index 00000000000..8506070f101
--- /dev/null
+++ b/src/backend/access/common/reloptions.c
@@ -0,0 +1,326 @@
+/*-------------------------------------------------------------------------
+ *
+ * reloptions.c
+ *	  Core support for relation options (pg_class.reloptions)
+ *
+ * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *	  $PostgreSQL: pgsql/src/backend/access/common/reloptions.c,v 1.1 2006/07/03 22:45:36 tgl Exp $
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "access/reloptions.h"
+#include "catalog/pg_type.h"
+#include "commands/defrem.h"
+#include "utils/array.h"
+#include "utils/builtins.h"
+#include "utils/rel.h"
+
+
+/*
+ * Transform a relation options list (list of DefElem) into the text array
+ * format that is kept in pg_class.reloptions.
+ *
+ * This is used for three cases: CREATE TABLE/INDEX, ALTER TABLE SET, and
+ * ALTER TABLE RESET.  In the ALTER cases, oldOptions is the existing
+ * reloptions value (possibly NULL), and we replace or remove entries
+ * as needed.
+ *
+ * If ignoreOids is true, then we should ignore any occurrence of "oids"
+ * in the list (it will be or has been handled by interpretOidsOption()).
+ *
+ * Note that this is not responsible for determining whether the options
+ * are valid.
+ *
+ * Both oldOptions and the result are text arrays (or NULL for "default"),
+ * but we declare them as Datums to avoid including array.h in reloptions.h.
+ */
+Datum
+transformRelOptions(Datum oldOptions, List *defList,
+					bool ignoreOids, bool isReset)
+{
+	Datum		result;
+	ArrayBuildState *astate;
+	ListCell   *cell;
+
+	/* no change if empty list */
+	if (defList == NIL)
+		return oldOptions;
+
+	/* We build new array using accumArrayResult */
+	astate = NULL;
+
+	/* Copy any oldOptions that aren't to be replaced */
+	if (oldOptions != (Datum) 0)
+	{
+		ArrayType *array = DatumGetArrayTypeP(oldOptions);
+		Datum	   *oldoptions;
+		int			noldoptions;
+		int			i;
+
+		Assert(ARR_ELEMTYPE(array) == TEXTOID);
+
+		deconstruct_array(array, TEXTOID, -1, false, 'i',
+						  &oldoptions, NULL, &noldoptions);
+
+		for (i = 0; i < noldoptions; i++)
+		{
+			text   *oldoption = DatumGetTextP(oldoptions[i]);
+			char   *text_str = (char *) VARATT_DATA(oldoption);
+			int		text_len = VARATT_SIZE(oldoption) - VARHDRSZ;
+
+			/* Search for a match in defList */
+			foreach(cell, defList)
+			{
+				DefElem	   *def = lfirst(cell);
+				int		kw_len = strlen(def->defname);
+
+				if (text_len > kw_len && text_str[kw_len] == '=' &&
+					pg_strncasecmp(text_str, def->defname, kw_len) == 0)
+					break;
+			}
+			if (!cell)
+			{
+				/* No match, so keep old option */
+				astate = accumArrayResult(astate, oldoptions[i],
+										  false, TEXTOID,
+										  CurrentMemoryContext);
+			}
+		}
+	}
+
+	/*
+	 * If CREATE/SET, add new options to array; if RESET, just check that
+	 * the user didn't say RESET (option=val).  (Must do this because the
+	 * grammar doesn't enforce it.)
+	 */
+	foreach(cell, defList)
+	{
+		DefElem	   *def = lfirst(cell);
+
+		if (isReset)
+		{
+			if (def->arg != NULL)
+				ereport(ERROR,
+						(errcode(ERRCODE_SYNTAX_ERROR),
+						 errmsg("RESET must not include values for parameters")));
+		}
+		else
+		{
+			text   *t;
+			const char *value;
+			Size	len;
+
+			if (ignoreOids && pg_strcasecmp(def->defname, "oids") == 0)
+				continue;
+
+			/*
+			 * Flatten the DefElem into a text string like "name=arg".
+			 * If we have just "name", assume "name=true" is meant.
+			 */
+			if (def->arg != NULL)
+				value = defGetString(def);
+			else
+				value = "true";
+			len = VARHDRSZ + strlen(def->defname) + 1 + strlen(value);
+			/* +1 leaves room for sprintf's trailing null */
+			t = (text *) palloc(len + 1);
+			VARATT_SIZEP(t) = len;
+			sprintf((char *) VARATT_DATA(t), "%s=%s", def->defname, value);
+
+			astate = accumArrayResult(astate, PointerGetDatum(t),
+									  false, TEXTOID,
+									  CurrentMemoryContext);
+		}
+	}
+
+	if (astate)
+		result = makeArrayResult(astate, CurrentMemoryContext);
+	else
+		result = (Datum) 0;
+
+	return result;
+}
+
+
+/*
+ * Interpret reloptions that are given in text-array format.
+ *
+ *	options: array of "keyword=value" strings, as built by transformRelOptions
+ *	numkeywords: number of legal keywords
+ *	keywords: the allowed keywords
+ *	values: output area
+ *	validate: if true, throw error for unrecognized keywords.
+ *
+ * The keywords and values arrays must both be of length numkeywords.
+ * The values entry corresponding to a keyword is set to a palloc'd string
+ * containing the corresponding value, or NULL if the keyword does not appear.
+ */
+void
+parseRelOptions(Datum options, int numkeywords, const char * const *keywords,
+				char **values, bool validate)
+{
+	ArrayType *array;
+	Datum	   *optiondatums;
+	int			noptions;
+	int			i;
+
+	/* Initialize to "all defaulted" */
+	MemSet(values, 0, numkeywords * sizeof(char *));
+
+	/* Done if no options */
+	if (options == (Datum) 0)
+		return;
+
+	array = DatumGetArrayTypeP(options);
+
+	Assert(ARR_ELEMTYPE(array) == TEXTOID);
+
+	deconstruct_array(array, TEXTOID, -1, false, 'i',
+					  &optiondatums, NULL, &noptions);
+
+	for (i = 0; i < noptions; i++)
+	{
+		text   *optiontext = DatumGetTextP(optiondatums[i]);
+		char   *text_str = (char *) VARATT_DATA(optiontext);
+		int		text_len = VARATT_SIZE(optiontext) - VARHDRSZ;
+		int		j;
+
+		/* Search for a match in keywords */
+		for (j = 0; j < numkeywords; j++)
+		{
+			int		kw_len = strlen(keywords[j]);
+
+			if (text_len > kw_len && text_str[kw_len] == '=' &&
+				pg_strncasecmp(text_str, keywords[j], kw_len) == 0)
+			{
+				char   *value;
+				int		value_len;
+
+				if (values[j] && validate)
+					ereport(ERROR,
+							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+							 errmsg("duplicate parameter \"%s\"",
+									keywords[j])));
+				value_len = text_len - kw_len - 1;
+				value = (char *) palloc(value_len + 1);
+				memcpy(value, text_str + kw_len + 1, value_len);
+				value[value_len] = '\0';
+				values[j] = value;
+				break;
+			}
+		}
+		if (j >= numkeywords && validate)
+		{
+			char *s;
+			char *p;
+
+			s = DatumGetCString(DirectFunctionCall1(textout, optiondatums[i]));
+			p = strchr(s, '=');
+			if (p)
+				*p = '\0';
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("unrecognized parameter \"%s\"", s)));
+		}
+	}
+}
+
+
+/*
+ * Parse reloptions for anything using StdRdOptions (ie, fillfactor only)
+ */
+bytea *
+default_reloptions(Datum reloptions, bool validate,
+				   int minFillfactor, int defaultFillfactor)
+{
+	static const char * const default_keywords[1] = { "fillfactor" };
+	char   *values[1];
+	int32	fillfactor;
+	StdRdOptions *result;
+
+	parseRelOptions(reloptions, 1, default_keywords, values, validate);
+
+	/*
+	 * If no options, we can just return NULL rather than doing anything.
+	 * (defaultFillfactor is thus not used, but we require callers to pass
+	 * it anyway since we would need it if more options were added.)
+	 */
+	if (values[0] == NULL)
+		return NULL;
+
+	fillfactor = pg_atoi(values[0], sizeof(int32), 0);
+	if (fillfactor < minFillfactor || fillfactor > 100)
+	{
+		if (validate)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("fillfactor=%d is out of range (should be between %d and 100)",
+							fillfactor, minFillfactor)));
+		return NULL;
+	}
+
+	result = (StdRdOptions *) palloc(sizeof(StdRdOptions));
+	VARATT_SIZEP(result) = sizeof(StdRdOptions);
+
+	result->fillfactor = fillfactor;
+
+	return (bytea *) result;
+}
+
+
+/*
+ * Parse options for heaps (and perhaps someday toast tables).
+ */
+bytea *
+heap_reloptions(char relkind, Datum reloptions, bool validate)
+{
+	return default_reloptions(reloptions, validate,
+							  HEAP_MIN_FILLFACTOR,
+							  HEAP_DEFAULT_FILLFACTOR);
+}
+
+
+/*
+ * Parse options for indexes.
+ *
+ *	amoptions	Oid of option parser
+ *	reloptions	options as text[] datum
+ *	validate	error flag
+ */
+bytea *
+index_reloptions(RegProcedure amoptions, Datum reloptions, bool validate)
+{
+	FmgrInfo	flinfo;
+	FunctionCallInfoData fcinfo;
+	Datum		result;
+
+	Assert(RegProcedureIsValid(amoptions));
+
+	/* Assume function is strict */
+	if (reloptions == (Datum) 0)
+		return NULL;
+
+	/* Can't use OidFunctionCallN because we might get a NULL result */
+	fmgr_info(amoptions, &flinfo);
+
+	InitFunctionCallInfoData(fcinfo, &flinfo, 2, NULL, NULL);
+
+	fcinfo.arg[0] = reloptions;
+	fcinfo.arg[1] = BoolGetDatum(validate);
+	fcinfo.argnull[0] = false;
+	fcinfo.argnull[1] = false;
+
+	result = FunctionCallInvoke(&fcinfo);
+
+	if (fcinfo.isnull || DatumGetPointer(result) == NULL)
+		return NULL;
+
+	return DatumGetByteaP(result);
+}
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 17e041bc4a8..544956c84ef 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *          $PostgreSQL: pgsql/src/backend/access/gin/ginutil.c,v 1.2 2006/07/02 02:23:18 momjian Exp $
+ *          $PostgreSQL: pgsql/src/backend/access/gin/ginutil.c,v 1.3 2006/07/03 22:45:36 tgl Exp $
  *-------------------------------------------------------------------------
  */
 
@@ -16,7 +16,7 @@
 #include "access/genam.h"
 #include "access/gin.h"
 #include "access/heapam.h"
-#include "catalog/index.h"
+#include "access/reloptions.h"
 #include "miscadmin.h"
 #include "storage/freespace.h"
 
@@ -203,15 +203,23 @@ GinPageGetCopyPage( Page page ) {
 }
 
 Datum
-ginoption(PG_FUNCTION_ARGS)
+ginoptions(PG_FUNCTION_ARGS)
 {
-	ArrayType	   *options = (ArrayType *) PG_GETARG_POINTER(0);
-
-	if (options != NULL)
-		ereport(ERROR,
-			(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				errmsg("GIN does not support parameters at all")));
-
-	/* Do not use PG_RETURN_NULL. */
-	PG_RETURN_BYTEA_P(NULL);
+	Datum		reloptions = PG_GETARG_DATUM(0);
+	bool		validate = PG_GETARG_BOOL(1);
+	bytea	   *result;
+
+	/*
+	 * It's not clear that fillfactor is useful for GIN, but for the moment
+	 * we'll accept it anyway.  (It won't do anything...)
+	 */
+#define GIN_MIN_FILLFACTOR			50
+#define GIN_DEFAULT_FILLFACTOR		100
+
+	result = default_reloptions(reloptions, validate,
+								GIN_MIN_FILLFACTOR,
+								GIN_DEFAULT_FILLFACTOR);
+	if (result)
+		PG_RETURN_BYTEA_P(result);
+	PG_RETURN_NULL();
 }
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 4137ab4426f..bc915332b51 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/gist/gist.c,v 1.140 2006/07/02 02:23:18 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/gist/gist.c,v 1.141 2006/07/03 22:45:36 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -197,9 +197,13 @@ gistbuildCallback(Relation index,
 	 * which locks the relation for write.	This is the right thing to do if
 	 * you're inserting single tups, but not when you're initializing the
 	 * whole index at once.
+	 *
+	 * In this path we respect the fillfactor setting, whereas insertions
+	 * after initial build do not.
 	 */
-	gistdoinsert(index, itup, IndexGetPageFreeSpace(index),
-		&buildstate->giststate);
+	gistdoinsert(index, itup,
+				 RelationGetTargetPageFreeSpace(index, GIST_DEFAULT_FILLFACTOR),
+				 &buildstate->giststate);
 
 	buildstate->indtuples += 1;
 	MemoryContextSwitchTo(oldCtx);
@@ -283,7 +287,6 @@ gistplacetopage(GISTInsertState *state, GISTSTATE *giststate)
 	bool		is_splitted = false;
 	bool		is_leaf = (GistPageIsLeaf(state->stack->page)) ? true : false;
 
-
 	/*
 	 * if (!is_leaf) remove old key:
 	 * This node's key has been modified, either because a child split
@@ -294,14 +297,13 @@ gistplacetopage(GISTInsertState *state, GISTSTATE *giststate)
 	 * setting up a one-element todelete array; in the split case, it's
 	 * handled implicitly because the tuple vector passed to gistSplit
 	 * won't include this tuple.
-	 */
-
-
-	/*
+	 *
 	 * XXX: If we want to change fillfactors between node and leaf,
 	 * fillfactor = (is_leaf ? state->leaf_fillfactor : state->node_fillfactor)
 	 */
-	if (gistnospace(state->stack->page, state->itup, state->ituplen, (is_leaf) ? InvalidOffsetNumber : state->stack->childoffnum, state->freespace))
+	if (gistnospace(state->stack->page, state->itup, state->ituplen,
+					is_leaf ? InvalidOffsetNumber : state->stack->childoffnum,
+					state->freespace))
 	{
 		/* no space for insertion */
 		IndexTuple *itvec;
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index ae1fbc73201..33cdae37c3e 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *			$PostgreSQL: pgsql/src/backend/access/gist/gistutil.c,v 1.17 2006/07/02 02:23:18 momjian Exp $
+ *			$PostgreSQL: pgsql/src/backend/access/gist/gistutil.c,v 1.18 2006/07/03 22:45:36 tgl Exp $
  *-------------------------------------------------------------------------
  */
 #include "postgres.h"
@@ -17,7 +17,7 @@
 #include "access/gist_private.h"
 #include "access/gistscan.h"
 #include "access/heapam.h"
-#include "catalog/index.h"
+#include "access/reloptions.h"
 #include "miscadmin.h"
 #include "storage/freespace.h"
 
@@ -637,14 +637,16 @@ gistNewBuffer(Relation r)
 }
 
 Datum
-gistoption(PG_FUNCTION_ARGS)
+gistoptions(PG_FUNCTION_ARGS)
 {
-#define GIST_DEFAULT_FILLFACTOR		90
-#define GIST_MIN_FILLFACTOR			50
-
-	ArrayType	   *options = (ArrayType *) PG_GETARG_POINTER(0);
-
-	/* Use index common routine. */
-	PG_RETURN_BYTEA_P(genam_option(options,
-		GIST_MIN_FILLFACTOR, GIST_DEFAULT_FILLFACTOR));
+	Datum		reloptions = PG_GETARG_DATUM(0);
+	bool		validate = PG_GETARG_BOOL(1);
+	bytea	   *result;
+
+	result = default_reloptions(reloptions, validate,
+								GIST_MIN_FILLFACTOR,
+								GIST_DEFAULT_FILLFACTOR);
+	if (result)
+		PG_RETURN_BYTEA_P(result);
+	PG_RETURN_NULL();
 }
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index bed0b5dbd72..f5a1fcfd814 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/hash/hashpage.c,v 1.58 2006/07/02 02:23:18 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/hash/hashpage.c,v 1.59 2006/07/03 22:45:36 tgl Exp $
  *
  * NOTES
  *	  Postgres hash pages look like ordinary relation pages.  The opaque
@@ -30,7 +30,6 @@
 
 #include "access/genam.h"
 #include "access/hash.h"
-#include "catalog/index.h"
 #include "miscadmin.h"
 #include "storage/lmgr.h"
 #include "utils/lsyscache.h"
@@ -223,16 +222,17 @@ _hash_metapinit(Relation rel)
 			 RelationGetRelationName(rel));
 
 	/*
-	 * Determine the target fill factor (tuples per bucket) for this index.
-	 * The idea is to make the fill factor correspond to pages about 3/4ths
-	 * full.  We can compute it exactly if the index datatype is fixed-width,
-	 * but for var-width there's some guessing involved.
+	 * Determine the target fill factor (in tuples per bucket) for this index.
+	 * The idea is to make the fill factor correspond to pages about as full
+	 * as the user-settable fillfactor parameter says.  We can compute it
+	 * exactly if the index datatype is fixed-width, but for var-width there's
+	 * some guessing involved.
 	 */
 	data_width = get_typavgwidth(RelationGetDescr(rel)->attrs[0]->atttypid,
 								 RelationGetDescr(rel)->attrs[0]->atttypmod);
 	item_width = MAXALIGN(sizeof(IndexTupleData)) + MAXALIGN(data_width) +
 		sizeof(ItemIdData);		/* include the line pointer */
-	ffactor = BLCKSZ * IndexGetFillFactor(rel) / 100 / item_width;
+	ffactor = RelationGetTargetPageUsage(rel, HASH_DEFAULT_FILLFACTOR) / item_width;
 	/* keep to a sane range */
 	if (ffactor < 10)
 		ffactor = 10;
diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
index fbee1fdc2aa..a1a03ddcf36 100644
--- a/src/backend/access/hash/hashutil.c
+++ b/src/backend/access/hash/hashutil.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/hash/hashutil.c,v 1.48 2006/07/02 02:23:18 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/hash/hashutil.c,v 1.49 2006/07/03 22:45:36 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -16,6 +16,7 @@
 
 #include "access/genam.h"
 #include "access/hash.h"
+#include "access/reloptions.h"
 #include "executor/execdebug.h"
 
 
@@ -175,14 +176,16 @@ _hash_checkpage(Relation rel, Buffer buf, int flags)
 }
 
 Datum
-hashoption(PG_FUNCTION_ARGS)
+hashoptions(PG_FUNCTION_ARGS)
 {
-#define HASH_MIN_FILLFACTOR			50
-#define HASH_DEFAULT_FILLFACTOR		75
-
-	ArrayType	   *options = (ArrayType *) PG_GETARG_POINTER(0);
-
-	/* Use index common routine. */
-	PG_RETURN_BYTEA_P(genam_option(options,
-		HASH_MIN_FILLFACTOR, HASH_DEFAULT_FILLFACTOR));
+	Datum		reloptions = PG_GETARG_DATUM(0);
+	bool		validate = PG_GETARG_BOOL(1);
+	bytea	   *result;
+
+	result = default_reloptions(reloptions, validate,
+								HASH_MIN_FILLFACTOR,
+								HASH_DEFAULT_FILLFACTOR);
+	if (result)
+		PG_RETURN_BYTEA_P(result);
+	PG_RETURN_NULL();
 }
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index cf3344c2009..cf4b920f7da 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.214 2006/07/02 02:23:18 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.215 2006/07/03 22:45:37 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -46,13 +46,9 @@
 #include "access/xlogutils.h"
 #include "catalog/catalog.h"
 #include "catalog/namespace.h"
-#include "commands/defrem.h"
 #include "miscadmin.h"
-#include "nodes/parsenodes.h"
-#include "parser/parse_clause.h"
 #include "pgstat.h"
 #include "storage/procarray.h"
-#include "utils/catcache.h"
 #include "utils/inval.h"
 #include "utils/relcache.h"
 
@@ -3592,59 +3588,3 @@ heap_desc(StringInfo buf, uint8 xl_info, char *rec)
 	else
 		appendStringInfo(buf, "UNKNOWN");
 }
-
-/*
- * Parse options for heaps.
- *
- *	relkind		Kind of relation
- *	options		Options as text[]
- */
-bytea *
-heap_option(char relkind, ArrayType *options)
-{
-	/*
-	 * XXX: What fillfactor should be default?
-	 * overriding databases:
-	 *   - Oracle, DB2 = 90%
-	 *   - SQL Server = 100%
-	 * non-overriding database:
-	 *   - Firebird = 70%
-	 */
-#define HEAP_MIN_FILLFACTOR			50
-#define HEAP_DEFAULT_FILLFACTOR		100
-
-	int				fillfactor;
-	HeapOption	   *result;
-
-	DefElem	kwds[] =
-	{
-		{ T_DefElem, "fillfactor" },
-	};
-
-	/*
-	 * parse options
-	 */
-	OptionParse(options, lengthof(kwds), kwds, true);
-
-	/* 0: fillfactor */
-	if (kwds[0].arg)
-		fillfactor = (int) defGetInt64(&kwds[0]);
-	else
-		fillfactor = HEAP_DEFAULT_FILLFACTOR;
-	if (fillfactor < HEAP_MIN_FILLFACTOR || 100 < fillfactor)
-	{
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("fillfactor=%d should be between %d and 100",
-                 fillfactor, HEAP_MIN_FILLFACTOR)));
-	}
-
-	/*
-	 * build option
-	 */
-	result = (HeapOption *)
-		MemoryContextAlloc(CacheMemoryContext, sizeof(HeapOption));
-	VARATT_SIZEP(result) = sizeof(HeapOption);
-	result->fillfactor = fillfactor;
-	return (bytea *) result;
-}
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 82fb0a3268a..aaaec863ba1 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/heap/hio.c,v 1.62 2006/07/02 02:23:18 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/heap/hio.c,v 1.63 2006/07/03 22:45:37 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -93,6 +93,11 @@ RelationPutHeapTuple(Relation relation,
  *	any committed data of other transactions.  (See heap_insert's comments
  *	for additional constraints needed for safe usage of this behavior.)
  *
+ *	We always try to avoid filling existing pages further than the fillfactor.
+ *	This is OK since this routine is not consulted when updating a tuple and
+ *	keeping it on the same page, which is the scenario fillfactor is meant
+ *	to reserve space for.
+ *
  *	ereport(ERROR) is allowed here, so this routine *must* be called
  *	before any (unlogged) changes are made in buffer pool.
  */
@@ -103,17 +108,12 @@ RelationGetBufferForTuple(Relation relation, Size len,
 	Buffer		buffer = InvalidBuffer;
 	Page		pageHeader;
 	Size		pageFreeSpace,
-				freespace;
+				saveFreeSpace;
 	BlockNumber targetBlock,
 				otherBlock;
 	bool		needLock;
 
-	if (relation->rd_options == NULL)
-		elog(ERROR, "RelationGetBufferForTuple %s IS NULL", RelationGetRelationName(relation));
-	Assert(relation->rd_options != NULL);
-
 	len = MAXALIGN(len);		/* be conservative */
-	freespace = HeapGetPageFreeSpace(relation);
 
 	/*
 	 * If we're gonna fail for oversize tuple, do it right away
@@ -125,6 +125,10 @@ RelationGetBufferForTuple(Relation relation, Size len,
 						(unsigned long) len,
 						(unsigned long) MaxTupleSize)));
 
+	/* Compute desired extra freespace due to fillfactor option */
+	saveFreeSpace = RelationGetTargetPageFreeSpace(relation,
+												   HEAP_DEFAULT_FILLFACTOR);
+
 	if (otherBuffer != InvalidBuffer)
 		otherBlock = BufferGetBlockNumber(otherBuffer);
 	else
@@ -143,8 +147,14 @@ RelationGetBufferForTuple(Relation relation, Size len,
 	 * When use_fsm is false, we either put the tuple onto the existing target
 	 * page or extend the relation.
 	 */
-
-	targetBlock = relation->rd_targblock;
+	if (len + saveFreeSpace <= MaxTupleSize)
+		targetBlock = relation->rd_targblock;
+	else
+	{
+		/* can't fit, don't screw up FSM request tracking by trying */
+		targetBlock = InvalidBlockNumber;
+		use_fsm = false;
+	}
 
 	if (targetBlock == InvalidBlockNumber && use_fsm)
 	{
@@ -152,7 +162,8 @@ RelationGetBufferForTuple(Relation relation, Size len,
 		 * We have no cached target page, so ask the FSM for an initial
 		 * target.
 		 */
-		targetBlock = GetPageWithFreeSpace(&relation->rd_node, len + freespace);
+		targetBlock = GetPageWithFreeSpace(&relation->rd_node,
+										   len + saveFreeSpace);
 
 		/*
 		 * If the FSM knows nothing of the rel, try the last page before we
@@ -208,7 +219,7 @@ RelationGetBufferForTuple(Relation relation, Size len,
 		 */
 		pageHeader = (Page) BufferGetPage(buffer);
 		pageFreeSpace = PageGetFreeSpace(pageHeader);
-		if (len + freespace <= pageFreeSpace)
+		if (len + saveFreeSpace <= pageFreeSpace)
 		{
 			/* use this page as future insert target, too */
 			relation->rd_targblock = targetBlock;
@@ -241,7 +252,7 @@ RelationGetBufferForTuple(Relation relation, Size len,
 		targetBlock = RecordAndGetPageWithFreeSpace(&relation->rd_node,
 													targetBlock,
 													pageFreeSpace,
-													len + freespace);
+													len + saveFreeSpace);
 	}
 
 	/*
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index cfc2a833cd3..399386c8567 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/index/genam.c,v 1.56 2006/07/02 02:23:18 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/index/genam.c,v 1.57 2006/07/03 22:45:37 tgl Exp $
  *
  * NOTES
  *	  many of the old access method routines have been turned into
@@ -21,12 +21,8 @@
 
 #include "access/genam.h"
 #include "access/heapam.h"
-#include "commands/defrem.h"
 #include "miscadmin.h"
-#include "nodes/parsenodes.h"
-#include "parser/parse_clause.h"
 #include "pgstat.h"
-#include "utils/catcache.h"
 
 
 /* ----------------------------------------------------------------
@@ -264,44 +260,3 @@ systable_endscan(SysScanDesc sysscan)
 
 	pfree(sysscan);
 }
-
-/*
- * Parse options for generic indexes.
- */
-bytea *
-genam_option(ArrayType *options,
-             int minFillfactor, int defaultFillfactor)
-{
-	int				fillfactor;
-	IndexOption	   *result;
-
-	DefElem	kwds[] =
-	{
-		{ T_DefElem, "fillfactor" },
-	};
-
-	/*
-	 * parse options
-	 */
-	OptionParse(options, lengthof(kwds), kwds, true);
-
-	/* 0: fillfactor */
-	if (kwds[0].arg)
-		fillfactor = (int) defGetInt64(&kwds[0]);
-	else
-		fillfactor = defaultFillfactor;
-	if (fillfactor < minFillfactor || 100 < fillfactor)
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("fillfactor=%d should be between %d and 100",
-				 fillfactor, minFillfactor)));
-
-	/*
-	 * build options
-	 */
-	result = (IndexOption *)
-		MemoryContextAlloc(CacheMemoryContext, sizeof(IndexOption));
-	VARATT_SIZEP(result) = sizeof(IndexOption);
-	result->fillfactor = fillfactor;
-	return (bytea *) result;
-}
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 3b78843101e..a1768886915 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -8,14 +8,13 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtinsert.c,v 1.138 2006/07/02 02:23:18 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtinsert.c,v 1.139 2006/07/03 22:45:37 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
 
 #include "postgres.h"
 
-#include "access/genam.h"
 #include "access/heapam.h"
 #include "access/nbtree.h"
 #include "miscadmin.h"
@@ -26,7 +25,7 @@ typedef struct
 {
 	/* context data for _bt_checksplitloc */
 	Size		newitemsz;		/* size of new item to be inserted */
-	int			fillfactor;		/* used when insert at right most */
+	int			fillfactor;		/* needed when splitting rightmost page */
 	bool		is_leaf;		/* T if splitting a leaf page */
 	bool		is_rightmost;	/* T if splitting a rightmost page */
 
@@ -988,11 +987,11 @@ _bt_split(Relation rel, Buffer buf, OffsetNumber firstright,
  * it needs to go into!)
  *
  * If the page is the rightmost page on its level, we instead try to arrange
- * for reserving (100-fillfactor)% of free space on left page. In this way,
- * when we are inserting successively increasing keys (consider sequences,
- * timestamps, etc) we will end up with a tree whose pages are about fillfactor% full,
+ * to leave the left split page fillfactor% full.  In this way, when we are
+ * inserting successively increasing keys (consider sequences, timestamps,
+ * etc) we will end up with a tree whose pages are about fillfactor% full,
  * instead of the 50% full result that we'd get without this special case.
- * This is the same as initially-loaded tree.
+ * This is the same as nbtsort.c produces for a newly-created tree.
  *
  * We are passed the intended insert position of the new tuple, expressed as
  * the offsetnumber of the tuple it must go in front of.  (This could be
@@ -1026,7 +1025,7 @@ _bt_findsplitloc(Relation rel,
 	/* Passed-in newitemsz is MAXALIGNED but does not include line pointer */
 	newitemsz += sizeof(ItemIdData);
 	state.newitemsz = newitemsz;
-	state.fillfactor = IndexGetFillFactor(rel);
+	state.fillfactor = RelationGetFillFactor(rel, BTREE_DEFAULT_FILLFACTOR);
 	state.is_leaf = P_ISLEAF(opaque);
 	state.is_rightmost = P_RIGHTMOST(opaque);
 	state.have_split = false;
@@ -1157,7 +1156,7 @@ _bt_checksplitloc(FindSplitData *state, OffsetNumber firstright,
 		if (state->is_rightmost)
 		{
 			/*
-			 * On a rightmost page, try to reserve (100-fillfactor)% of
+			 * If splitting a rightmost page, try to put (100-fillfactor)% of
 			 * free space on left page. See comments for _bt_findsplitloc.
 			 */
 			delta = (state->fillfactor * leftfree)
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index 05785d98eb5..1dd3be0a63d 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -27,9 +27,10 @@
  * insertion would cause a split (and not only of the leaf page; the need
  * for a split would cascade right up the tree).  The steady-state load
  * factor for btrees is usually estimated at 70%.  We choose to pack leaf
- * pages to 90% and upper pages to 70%.  This gives us reasonable density
- * (there aren't many upper pages if the keys are reasonable-size) without
- * incurring a lot of cascading splits during early insertions.
+ * pages to the user-controllable fill factor while upper pages are always
+ * packed to 70%.  This gives us reasonable density (there aren't many upper
+ * pages if the keys are reasonable-size) without incurring a lot of cascading
+ * splits during early insertions.
  *
  * Formerly the index pages being built were kept in shared buffers, but
  * that is of no value (since other backends have no interest in them yet)
@@ -56,14 +57,13 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtsort.c,v 1.103 2006/07/02 02:23:19 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtsort.c,v 1.104 2006/07/03 22:45:37 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
 
 #include "postgres.h"
 
-#include "access/genam.h"
 #include "access/nbtree.h"
 #include "access/xlog.h"
 #include "miscadmin.h"
@@ -121,7 +121,6 @@ typedef struct BTWriteState
 
 
 static Page _bt_blnewpage(uint32 level);
-static Size	_bt_full_threshold(Relation index, Size pagesize, bool leaf);
 static BTPageState *_bt_pagestate(BTWriteState *wstate, uint32 level);
 static void _bt_slideleft(Page page);
 static void _bt_sortaddtup(Page page, Size itemsize,
@@ -330,22 +329,6 @@ _bt_blwritepage(BTWriteState *wstate, Page page, BlockNumber blkno)
 }
 
 /*
- * The steady-state load factor for btrees is usually estimated at 70%.
- * We choose to pack leaf pages to 90% and upper pages to 70% as defaults.
- */
-static Size
-_bt_full_threshold(Relation index, Size pagesize, bool leaf)
-{
-	int fillfactor = IndexGetFillFactor(index);
-	if (!leaf)
-	{
-		/* XXX: Is this reasonable? */
-		fillfactor = Max(70, 3 * fillfactor - 200);
-	}
-	return pagesize * (100 - fillfactor) / 100;
-}
-
-/*
  * allocate and initialize a new BTPageState.  the returned structure
  * is suitable for immediate use by _bt_buildadd.
  */
@@ -365,8 +348,11 @@ _bt_pagestate(BTWriteState *wstate, uint32 level)
 	state->btps_lastoff = P_HIKEY;
 	state->btps_level = level;
 	/* set "full" threshold based on level.  See notes at head of file. */
-	state->btps_full = _bt_full_threshold(wstate->index,
-		PageGetPageSize(state->btps_page), level == 0);
+	if (level > 0)
+		state->btps_full = (BLCKSZ * (100 - BTREE_MIN_FILLFACTOR) / 100);
+	else
+		state->btps_full = RelationGetTargetPageFreeSpace(wstate->index,
+												BTREE_DEFAULT_FILLFACTOR);
 	/* no parent level, yet */
 	state->btps_next = NULL;
 
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index 93a9b0df653..6750a982d7c 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtutils.c,v 1.75 2006/07/02 02:23:19 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/nbtree/nbtutils.c,v 1.76 2006/07/03 22:45:37 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -19,7 +19,7 @@
 
 #include "access/genam.h"
 #include "access/nbtree.h"
-#include "catalog/catalog.h"
+#include "access/reloptions.h"
 #include "executor/execdebug.h"
 #include "miscadmin.h"
 
@@ -1081,14 +1081,16 @@ BTreeShmemInit(void)
 }
 
 Datum
-btoption(PG_FUNCTION_ARGS)
+btoptions(PG_FUNCTION_ARGS)
 {
-#define BTREE_MIN_FILLFACTOR		50
-#define BTREE_DEFAULT_FILLFACTOR	90
-
-	ArrayType	   *options = (ArrayType *) PG_GETARG_POINTER(0);
-
-	/* Use index common routine. */
-	PG_RETURN_BYTEA_P(genam_option(options,
-		BTREE_MIN_FILLFACTOR, BTREE_DEFAULT_FILLFACTOR));
+	Datum		reloptions = PG_GETARG_DATUM(0);
+	bool		validate = PG_GETARG_BOOL(1);
+	bytea	   *result;
+
+	result = default_reloptions(reloptions, validate,
+								BTREE_MIN_FILLFACTOR,
+								BTREE_DEFAULT_FILLFACTOR);
+	if (result)
+		PG_RETURN_BYTEA_P(result);
+	PG_RETURN_NULL();
 }
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index da30e39b42d..b6fd45d6dfb 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -11,7 +11,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/backend/access/transam/xlogutils.c,v 1.45 2006/07/02 02:23:19 momjian Exp $
+ * $PostgreSQL: pgsql/src/backend/access/transam/xlogutils.c,v 1.46 2006/07/03 22:45:37 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -337,7 +337,7 @@ _xl_remove_hash_entry(XLogRelDesc *rdesc)
 	RelationCloseSmgr(&(rdesc->reldata));
 
 	memset(rdesc, 0, sizeof(XLogRelDesc));
-	memset(tpgc, 0, CLASS_TUPLE_SIZE);
+	memset(tpgc, 0, sizeof(FormData_pg_class));
 	rdesc->reldata.rd_rel = tpgc;
 }