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, 29 insertions, 295 deletions

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 9e66ca1ab65..0d1adeac18c 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- *	$PostgreSQL: pgsql/src/backend/parser/analyze.c,v 1.337 2006/07/02 02:23:20 momjian Exp $
+ *	$PostgreSQL: pgsql/src/backend/parser/analyze.c,v 1.338 2006/07/03 22:45:39 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1882,7 +1882,6 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
 	if (stmt->intoColNames)
 		applyColumnNames(qry->targetList, stmt->intoColNames);
 
-	qry->intoHasOids = interpretOidsOption(stmt->intoOptions);
 	qry->intoOptions = copyObject(stmt->intoOptions);
 	qry->intoOnCommit = stmt->intoOnCommit;
 	qry->intoTableSpaceName = stmt->intoTableSpaceName;
@@ -2754,8 +2753,6 @@ transformExecuteStmt(ParseState *pstate, ExecuteStmt *stmt)
 
 	paramtypes = FetchPreparedStatementParams(stmt->name);
 
-	stmt->into_has_oids = interpretOidsOption(stmt->intoOptions);
-
 	if (stmt->params || paramtypes)
 	{
 		int			nparams = list_length(stmt->params);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index f09a7a6b2de..754777c57bc 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -11,7 +11,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/parser/gram.y,v 2.550 2006/07/02 02:23:21 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/parser/gram.y,v 2.551 2006/07/03 22:45:39 tgl Exp $
  *
  * HISTORY
  *	  AUTHOR			DATE			MAJOR EVENT
@@ -232,7 +232,7 @@ static void doNegateFloat(Value *v);
 				func_as createfunc_opt_list alterfunc_opt_list
 				aggr_args aggr_args_list old_aggr_definition old_aggr_list
 				oper_argtypes RuleActionList RuleActionMulti
-				opt_column_list columnList opt_name_list 
+				opt_column_list columnList opt_name_list
 				sort_clause opt_sort_clause sortby_list index_params
 				name_list from_clause from_list opt_array_bounds
 				qualified_name_list any_name any_name_list
@@ -1562,25 +1562,15 @@ alter_rel_cmd:
 			| SET definition
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
-					n->subtype = AT_SetOptions;
+					n->subtype = AT_SetRelOptions;
 					n->def = (Node *)$2;
 					$$ = (Node *)n;
 				}
+			/* ALTER [TABLE|INDEX] <name> RESET (...) */
 			| RESET definition
 				{
-					AlterTableCmd *n;
-					ListCell	  *cell;
-					
-					foreach(cell, $2)
-					{
-						if (((DefElem *) lfirst(cell))->arg != NULL)
-							ereport(ERROR,
-									(errcode(ERRCODE_SYNTAX_ERROR),
-									 errmsg("parameters for RESET should not take values")));
-					}
-
-					n = makeNode(AlterTableCmd);
-					n->subtype = AT_SetOptions;
+					AlterTableCmd *n = makeNode(AlterTableCmd);
+					n->subtype = AT_ResetRelOptions;
 					n->def = (Node *)$2;
 					$$ = (Node *)n;
 				}
@@ -1919,7 +1909,7 @@ ColConstraintElem:
 					n->indexspace = NULL;
 					$$ = (Node *)n;
 				}
-			| UNIQUE OptConsTableSpace
+			| UNIQUE opt_definition OptConsTableSpace
 				{
 					Constraint *n = makeNode(Constraint);
 					n->contype = CONSTR_UNIQUE;
@@ -1927,7 +1917,8 @@ ColConstraintElem:
 					n->raw_expr = NULL;
 					n->cooked_expr = NULL;
 					n->keys = NULL;
-					n->indexspace = $2;
+					n->options = $2;
+					n->indexspace = $3;
 					$$ = (Node *)n;
 				}
 			| PRIMARY KEY opt_definition OptConsTableSpace
@@ -2100,7 +2091,7 @@ ConstraintElem:
 					n->indexspace = NULL;
 					$$ = (Node *)n;
 				}
-			| UNIQUE '(' columnList ')' OptConsTableSpace
+			| UNIQUE '(' columnList ')' opt_definition OptConsTableSpace
 				{
 					Constraint *n = makeNode(Constraint);
 					n->contype = CONSTR_UNIQUE;
@@ -2108,7 +2099,8 @@ ConstraintElem:
 					n->raw_expr = NULL;
 					n->cooked_expr = NULL;
 					n->keys = $3;
-					n->indexspace = $5;
+					n->options = $5;
+					n->indexspace = $6;
 					$$ = (Node *)n;
 				}
 			| PRIMARY KEY '(' columnList ')' opt_definition OptConsTableSpace
@@ -2214,13 +2206,12 @@ OptInherit: INHERITS '(' qualified_name_list ')'	{ $$ = $3; }
 			| /*EMPTY*/								{ $$ = NIL; }
 		;
 
+/* WITH (options) is preferred, WITH OIDS and WITHOUT OIDS are legacy forms */
 OptWith:
-			WITH OIDS						{ $$ = list_make1(defWithOids(true)); }
-			| WITHOUT OIDS					{ $$ = list_make1(defWithOids(false)); }
-			| WITH definition				{ $$ = $2; }
-			| WITH OIDS WITH definition		{ $$ = lappend($4, defWithOids(true)); }
-			| WITHOUT OIDS WITH definition	{ $$ = lappend($4, defWithOids(false)); }
-			| /*EMPTY*/						{ $$ = NIL; }
+			WITH definition				{ $$ = $2; }
+			| WITH OIDS					{ $$ = list_make1(defWithOids(true)); }
+			| WITHOUT OIDS				{ $$ = list_make1(defWithOids(false)); }
+			| /*EMPTY*/					{ $$ = NIL; }
 		;
 
 OnCommitOption:  ON COMMIT DROP				{ $$ = ONCOMMIT_DROP; }
@@ -2874,6 +2865,8 @@ def_elem:  ColLabel '=' def_arg
 
 /* Note: any simple identifier will be returned as a type name! */
 def_arg:	func_type						{ $$ = (Node *)$1; }
+			| func_name_keyword				{ $$ = (Node *)makeString(pstrdup($1)); }
+			| reserved_keyword				{ $$ = (Node *)makeString(pstrdup($1)); }
 			| qual_all_Op					{ $$ = (Node *)$1; }
 			| NumericOnly					{ $$ = (Node *)$1; }
 			| Sconst						{ $$ = (Node *)makeString($1); }
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index a4fe1999fe5..b83780408e5 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/parser/parse_clause.c,v 1.150 2006/07/02 02:23:21 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/parser/parse_clause.c,v 1.151 2006/07/03 22:45:39 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -34,7 +34,6 @@
 #include "rewrite/rewriteManip.h"
 #include "utils/builtins.h"
 #include "utils/guc.h"
-#include "utils/memutils.h"
 
 
 #define ORDER_CLAUSE 0
@@ -66,8 +65,6 @@ static Node *buildMergedJoinVar(ParseState *pstate, JoinType jointype,
 				   Var *l_colvar, Var *r_colvar);
 static TargetEntry *findTargetlistEntry(ParseState *pstate, Node *node,
 					List **tlist, int clause);
-static bool OptionMatches(text *t, const char* kw, char **str, Size *len);
-static Datum OptionToText(DefElem *def);
 
 
 /*
@@ -216,18 +213,18 @@ interpretInhOption(InhOption inhOpt)
 }
 
 /*
- * Given a List that indicates whether WITH / WITHOUT OIDS was
- * specified by the user, return true iff the specified table/result
- * set should be created with OIDs. This needs to be done after
- * parsing the query string because the return value can depend upon
- * the default_with_oids GUC var.
+ * Given a relation-options list (of DefElems), return true iff the specified
+ * table/result set should be created with OIDs. This needs to be done after
+ * parsing the query string because the return value can depend upon the
+ * default_with_oids GUC var.
  */
 bool
-interpretOidsOption(List *options)
+interpretOidsOption(List *defList)
 {
 	ListCell   *cell;
 
-	foreach(cell, options)
+	/* Scan list to see if OIDS was included */
+	foreach(cell, defList)
 	{
 		DefElem    *def = (DefElem *) lfirst(cell);
 
@@ -235,264 +232,11 @@ interpretOidsOption(List *options)
 			return defGetBoolean(def);
 	}
 
-	/* oids option is not specified. */
+	/* OIDS option was not specified, so use default. */
 	return default_with_oids;
 }
 
 /*
- * Test if t is start with 'kw='.
- */
-static bool
-OptionMatches(text *t, const char* kw, char **str, Size *len)
-{
-	char   *text_str = (char *) VARATT_DATA(t);
-	int		text_len = VARATT_SIZE(t) - VARHDRSZ;
-	Size	kwlen = strlen(kw);
-
-	if (text_len > kwlen && text_str[kwlen] == '=' && 
-        pg_strncasecmp(text_str, kw, kwlen) == 0)
-	{
-		*str = text_str + kwlen + 1;
-		*len = text_len - kwlen - 1;
-		return true;
-	}
-
-	return false;
-}
-
-/*
- * Flatten a DefElem to a text like as 'defname=arg'.
- */
-static Datum
-OptionToText(DefElem *def)
-{
-	text   *t;
-	char   *value = defGetString(def);
-	Size	len = VARHDRSZ + strlen(def->defname) + 1 + strlen(value);
-
-	t = palloc(len + 1);
-	VARATT_SIZEP(t) = len;
-	sprintf((char *) VARATT_DATA(t), "%s=%s", def->defname, value);
-
-	return PointerGetDatum(t);
-}
-
-/*
- * Merge option array and option list.
- *
- *	array	Existing option, or NULL if new option.
- *	list	List of DefElems to be added to array.
- */
-ArrayType *
-OptionBuild(ArrayType *array, List *list)
-{
-	ListCell		   *cell;
-	bool			   *used;
-	int					index;
-	int					o;
-	ArrayType		   *result;
-	ArrayBuildState	   *astate;
-	MemoryContext		myContext;
-	MemoryContext		oldContext;
-
-	if (list_length(list) == 0)
-	{
-		/* no additinal elements, so just clone. */
-		if (array == NULL)
-			return NULL;
-		result = palloc(VARATT_SIZE(array));
-		memcpy(result, array, VARATT_SIZE(array));
-		return result;
-	}
-
-	/* Make a temporary context to hold all the junk */
-	myContext = AllocSetContextCreate(CurrentMemoryContext,
-									  "OptionBuild",
-									  ALLOCSET_DEFAULT_MINSIZE,
-									  ALLOCSET_DEFAULT_INITSIZE,
-									  ALLOCSET_DEFAULT_MAXSIZE);
-	oldContext = MemoryContextSwitchTo(myContext);
-
-	astate = NULL;
-	used = (bool *) palloc0(sizeof(bool) * list_length(list));
-
-	if (array)
-	{
-		Assert(ARR_ELEMTYPE(array) == TEXTOID);
-		Assert(ARR_NDIM(array) == 1);
-		Assert(ARR_LBOUND(array)[0] == 1);
-
-		for (o = 1; o <= ARR_DIMS(array)[0]; o++)
-		{
-			bool		isnull;
-			Datum		datum;
-
-			datum = array_ref(array, 1, &o,
-						-1 /* varlenarray */ ,
-						-1 /* TEXT's typlen */ ,
-						false /* TEXT's typbyval */ ,
-						'i' /* TEXT's typalign */ ,
-						&isnull);
-			if (isnull)
-				continue;
-
-			index = 0;
-			foreach(cell, list)
-			{
-				DefElem	   *def = lfirst(cell);
-
-				/*
-				 * We ignore 'oids' item because it is stored
-				 * in pg_class.relhasoids.
-				 */
-				if (!used[index] &&
-					pg_strcasecmp(def->defname, "oids") != 0)
-				{
-					char   *value_str;
-					Size	value_len;
-					if (OptionMatches(DatumGetTextP(datum),
-						def->defname, &value_str, &value_len))
-					{
-						used[index] = true;
-						if (def->arg)
-						{
-							/* Replace an existing option. */
-							datum = OptionToText(def);
-							goto next;	/* skip remain items in list */
-						}
-						else
-						{
-							/* Remove the option from array. */
-							goto skip;
-						}
-					}
-				}
-
-				index++;
-			}
-
-			/*
-			 * The datum is an existing parameter and is not modified.
-			 * Fall down.
-			 */
-
-next:
-			astate = accumArrayResult(astate, datum, false, TEXTOID, myContext);
-skip:
-			;
-		}
-	}
-
-	/*
-	 * add options not in array
-	 */
-	index = 0;
-	foreach(cell, list)
-	{
-		DefElem	   *def = lfirst(cell);
-
-		if (!used[index] && def->arg &&
-			pg_strcasecmp(def->defname, "oids") != 0)
-		{
-			astate = accumArrayResult(astate, OptionToText(def),
-				false, TEXTOID, myContext);
-		}
-
-		index++;
-	}
-
-	if (astate)
-		result = DatumGetArrayTypeP(makeArrayResult(astate, oldContext));
-	else
-		result = NULL;
-
-	MemoryContextSwitchTo(oldContext);
-	MemoryContextDelete(myContext);
-	return result;
-}
-
-/*
- * Support routine to parse options.
- *
- *	options	List of DefElems
- *	num		length of kwds
- *	kwds	supported keywords
- *	strict	Throw error if unsupported option is found.
- *
- * FIXME: memory is leaked in kwds[].arg.
- */
-void
-OptionParse(ArrayType *options, Size num, DefElem kwds[], bool strict)
-{
-	Size	k;
-	int		o;
-
-	for (k = 0; k < num; k++)
-	{
-		Assert(kwds[k].defname);
-		kwds[k].arg = NULL;
-	}
-
-	if (options == NULL)
-		return;	/* use default for all */
-
-	Assert(ARR_ELEMTYPE(options) == TEXTOID);
-	Assert(ARR_NDIM(options) == 1);
-	Assert(ARR_LBOUND(options)[0] == 1);
-
-	for (o = 1; o <= ARR_DIMS(options)[0]; o++)
-	{
-		bool		isnull;
-		Datum		datum;
-
-		datum = array_ref(options, 1, &o,
-					  -1 /* varlenarray */ ,
-					  -1 /* TEXT's typlen */ ,
-					  false /* TEXT's typbyval */ ,
-					  'i' /* TEXT's typalign */ ,
-					  &isnull);
-		if (isnull)
-			continue;
-
-		for (k = 0; k < num; k++)
-		{
-			char	   *value_str;
-			Size		value_len;
-
-			if (OptionMatches(DatumGetTextP(datum),
-				kwds[k].defname, &value_str, &value_len))
-			{
-				char   *value;
-
-				if (kwds[k].arg != NULL)
-					ereport(ERROR,
-							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-							 errmsg("duplicated parameter %s",
-							 kwds[k].defname)));
-
-				/* copy value as Value node */
-				value = (char *) palloc(value_len + 1);
-				strncpy(value, value_str, value_len);
-				value[value_len] = '\0';
-				kwds[k].arg = (Node *) makeString(value);
-				goto next;	/* skip remain keywords */
-			}
-		}
-
-		/* parameter is not in kwds */
-		if (strict)
-		{
-			char *c = DatumGetCString(DirectFunctionCall1(textout, datum));
-
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("unsupported parameter %s", c)));
-		}
-next:;
-	}
-}
-
-/*
  * Extract all not-in-common columns from column lists of a source table
  */
 static void