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.

3 files changed, 331 insertions, 57 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);
+}