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.

Author: tglsfdc

doc/src/sgml/catalogs.sgml

@@ -1,4 +1,4 @@
-<!-- $PostgreSQL: pgsql/doc/src/sgml/catalogs.sgml,v 2.124 2006/06/03 02:53:04 tgl Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/catalogs.sgml,v 2.125 2006/07/03 22:45:36 tgl Exp $ -->
 <!--
  Documentation of the system catalogs, directed toward PostgreSQL developers
  -->
@@ -499,6 +499,13 @@
       <entry>Function to estimate cost of an index scan</entry>
      </row>
 
+     <row>
+      <entry><structfield>amoptions</structfield></entry>
+      <entry><type>regproc</type></entry>
+      <entry><literal><link linkend="catalog-pg-proc"><structname>pg_proc</structname></link>.oid</literal></entry>
+      <entry>Function to parse and validate reloptions for an index</entry>
+     </row>
+
     </tbody>
    </tgroup>
   </table>
@@ -1643,6 +1650,15 @@
        for details
       </entry>
      </row>
+
+     <row>
+      <entry><structfield>reloptions</structfield></entry>
+      <entry><type>text[]</type></entry>
+      <entry></entry>
+      <entry>
+       Access-method-specific options, as <quote>keyword=value</> strings
+      </entry>
+     </row>
     </tbody>
    </tgroup>
   </table>

doc/src/sgml/indexam.sgml

@@ -1,4 +1,4 @@
-<!-- $PostgreSQL: pgsql/doc/src/sgml/indexam.sgml,v 2.14 2006/06/06 17:59:57 tgl Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/indexam.sgml,v 2.15 2006/07/03 22:45:36 tgl Exp $ -->
 
 <chapter id="indexam">
  <title>Index Access Method Interface Definition</title>
@@ -233,6 +233,47 @@ amvacuumcleanup (IndexVacuumInfo *info,
    be returned.
   </para>
 
+  <para>
+<programlisting>
+void
+amcostestimate (PlannerInfo *root,
+                IndexOptInfo *index,
+                List *indexQuals,
+                RelOptInfo *outer_rel,
+                Cost *indexStartupCost,
+                Cost *indexTotalCost,
+                Selectivity *indexSelectivity,
+                double *indexCorrelation);
+</programlisting>
+   Estimate the costs of an index scan.  This function is described fully
+   in <xref linkend="index-cost-estimation">, below.
+  </para>
+
+  <para>
+<programlisting>
+bytea *
+amoptions (ArrayType *reloptions,
+           bool validate);
+</programlisting>
+   Parse and validate the reloptions array for an index.  This is called only
+   when a non-null reloptions array exists for the index.
+   <parameter>reloptions</> is a <type>text</> array containing entries of the
+   form <replaceable>name</><literal>=</><replaceable>value</>.
+   The function should construct a <type>bytea</> value, which will be copied
+   into the <structfield>rd_options</> field of the index's relcache entry.
+   The data contents of the <type>bytea</> value are open for the access
+   method to define, but the standard access methods currently all use struct
+   <structname>StdRdOptions</>.
+   When <parameter>validate</> is true, the function should report a suitable
+   error message if any of the options are unrecognized or have invalid
+   values; when <parameter>validate</> is false, invalid entries should be
+   silently ignored.  (<parameter>validate</> is false when loading options
+   already stored in <structname>pg_catalog</>; an invalid entry could only
+   be found if the access method has changed its rules for options, and in
+   that case ignoring obsolete entries is appropriate.)
+   It is OK to return NULL if default behavior is wanted.
+  </para>
+
   <para>
    The purpose of an index, of course, is to support scans for tuples matching
    an indexable <literal>WHERE</> condition, often called a
@@ -339,28 +380,16 @@ amrestrpos (IndexScanDesc scan);
   </para>
 
   <para>
-<programlisting>
-void
-amcostestimate (PlannerInfo *root,
-                IndexOptInfo *index,
-                List *indexQuals,
-                RelOptInfo *outer_rel,
-                Cost *indexStartupCost,
-                Cost *indexTotalCost,
-                Selectivity *indexSelectivity,
-                double *indexCorrelation);
-</programlisting>
-   Estimate the costs of an index scan.  This function is described fully
-   in <xref linkend="index-cost-estimation">, below.
-  </para>
-
-  <para>
-   By convention, the <literal>pg_proc</literal> entry for any index
+   By convention, the <literal>pg_proc</literal> entry for an index
    access method function should show the correct number of arguments,
    but declare them all as type <type>internal</> (since most of the arguments
    have types that are not known to SQL, and we don't want users calling
    the functions directly anyway).  The return type is declared as
    <type>void</>, <type>internal</>, or <type>boolean</> as appropriate.
+   The only exception is <function>amoptions</>, which should be correctly
+   declared as taking <type>text[]</> and <type>bool</> and returning
+   <type>bytea</>.  This provision allows client code to execute
+   <function>amoptions</> to test validity of options settings.
   </para>
 
  </sect1>

src/backend/access/common/Makefile

@@ -4,15 +4,15 @@
 #    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 $
 #
 #-------------------------------------------------------------------------
 
 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
 

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;
-}