Re: WIP: a way forward on bootstrap data

Attached is a round of minor review fixes.  Much of this is cleaning
up existing mistakes or out-of-date comments, rather than anything
introduced by this patchset, but I noticed it while going through the
patch.

The additional EXPOSE_TO_CLIENT_CODE bits actually are necessary,
in some cases, as I had compile failures without them after changing
client include lines.  (I'll post that separately.)

I added a couple more BKI_DEFAULT markers here, but omitted the ensuing
.dat file updates, because they're just mechanical.

I don't think there's any great need to incorporate this into your patch
set.  As far as I'm concerned, v14 is ready as-is, and I'll just apply
this over the top of it.  (Note that I'll probably smash the whole thing
to one commit when the time comes.)

I have some other work pending on the documentation aspect, but that's
not quite ready for public consumption yet.

            regards, tom lane

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 8729ccd..1626999 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -3566,7 +3566,7 @@ Oid PQftype(const PGresult *res,
        You can query the system table <literal>pg_type</literal> to
        obtain the names and properties of the various data types. The
        <acronym>OID</acronym>s of the built-in data types are defined
-       in the file <filename>src/include/catalog/pg_type.h</filename>
+       in the file <filename>src/include/catalog/pg_type_d.h</filename>
        in the source tree.
       </para>
      </listitem>
diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index 17213d4..d25d98a 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -25,8 +25,10 @@ BKIFILES = postgres.bki postgres.description postgres.shdescription

 include $(top_srcdir)/src/backend/common.mk

-# Note: there are some undocumented dependencies on the ordering in which
-# the catalog header files are assembled into postgres.bki.
+# Note: the order of this list determines the order in which the catalog
+# header files are assembled into postgres.bki.  BKI_BOOTSTRAP catalogs
+# must appear first, and there are reputedly other, undocumented ordering
+# dependencies.
 CATALOG_HEADERS := \
     pg_proc.h pg_type.h pg_attribute.h pg_class.h \
     pg_attrdef.h pg_constraint.h pg_inherits.h pg_index.h pg_operator.h \
@@ -49,7 +51,8 @@ CATALOG_HEADERS := \
 GENERATED_HEADERS := $(CATALOG_HEADERS:%.h=%_d.h) schemapg.h

 # In the list of headers used to assemble postgres.bki, indexing.h needs
-# be last, and toasting.h just before it.
+# be last, and toasting.h just before it.  This ensures we don't try to
+# create indexes or toast tables before their catalogs exist.
 POSTGRES_BKI_SRCS := $(addprefix $(top_srcdir)/src/include/catalog/,\
     $(CATALOG_HEADERS) toasting.h indexing.h \
     )
diff --git a/src/include/catalog/Makefile b/src/include/catalog/Makefile
index d84a572..1da3ea7 100644
--- a/src/include/catalog/Makefile
+++ b/src/include/catalog/Makefile
@@ -2,9 +2,6 @@
 #
 # Makefile for src/include/catalog
 #
-# 'make reformat-dat-files' is a convenience target for rewriting the
-# catalog data files in a standard format.
-#
 # Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
 # Portions Copyright (c) 1994, Regents of the University of California
 #
@@ -19,10 +16,16 @@ include $(top_builddir)/src/Makefile.global
 # location of Catalog.pm
 catalogdir = $(top_srcdir)/src/backend/catalog

+# 'make reformat-dat-files' is a convenience target for rewriting the
+# catalog data files in our standard format.  This includes collapsing
+# out any entries that are redundant with a BKI_DEFAULT annotation.
 reformat-dat-files:
     $(PERL) -I $(catalogdir) reformat_dat_file.pl pg_*.dat

+# 'make expand-dat-files' is a convenience target for expanding out all
+# default values in the catalog data files.  This should be run before
+# altering or removing any BKI_DEFAULT annotation.
 expand-dat-files:
     $(PERL) -I $(catalogdir) reformat_dat_file.pl pg_*.dat --full-tuples

-.PHONY: reformat-dat-files
+.PHONY: reformat-dat-files expand-dat-files
diff --git a/src/include/catalog/duplicate_oids b/src/include/catalog/duplicate_oids
index 0e6285f..8c143cf 100755
--- a/src/include/catalog/duplicate_oids
+++ b/src/include/catalog/duplicate_oids
@@ -30,7 +30,7 @@ foreach my $oid (sort { $a <=> $b } keys %oidcounts)
 {
     next unless $oidcounts{$oid} > 1;
     $found = 1;
-    print "***Duplicate OID: $oid\n";
+    print "$oid\n";
 }

 exit $found;
diff --git a/src/include/catalog/genbki.h b/src/include/catalog/genbki.h
index 02c38c4..b1e2cbd 100644
--- a/src/include/catalog/genbki.h
+++ b/src/include/catalog/genbki.h
@@ -34,7 +34,7 @@
 #define BKI_FORCE_NOT_NULL
 /* Specifies a default value for a catalog field */
 #define BKI_DEFAULT(value)
-/* Indicates how to perform name lookups for OID fields */
+/* Indicates how to perform name lookups for an OID or OID-array field */
 #define BKI_LOOKUP(catalog)

 /* The following are never defined; they are here only for documentation. */
@@ -48,11 +48,12 @@
 #undef CATALOG_VARLEN

 /*
- * There is code in the catalog headers that needs to be visible to clients,
- * but we don't want them to include the full header because of safety issues
- * with other code in the header. This symbol instructs genbki.pl to copy this
- * section when generating the corresponding definition header, where it can
- * be included by both client and backend code.
+ * There is code in some catalog headers that needs to be visible to clients,
+ * but we don't want clients to include the full header because of safety
+ * issues with other code in the header.  To handle that, surround code that
+ * should be visible to clients with "#ifdef EXPOSE_TO_CLIENT_CODE".  That
+ * instructs genbki.pl to copy the section when generating the corresponding
+ * "_d" header, which can be included by both client and backend code.
  */
 #undef EXPOSE_TO_CLIENT_CODE

diff --git a/src/include/catalog/pg_am.h b/src/include/catalog/pg_am.h
index 5aa2bac..9620bcd 100644
--- a/src/include/catalog/pg_am.h
+++ b/src/include/catalog/pg_am.h
@@ -47,9 +47,8 @@ typedef FormData_pg_am *Form_pg_am;

 #ifdef EXPOSE_TO_CLIENT_CODE

-/* ----------------
- *        compiler constant for amtype
- * ----------------
+/*
+ * Allowed values for amtype
  */
 #define AMTYPE_INDEX                    'i' /* index access method */

diff --git a/src/include/catalog/pg_authid.h b/src/include/catalog/pg_authid.h
index c410b99..863ef65 100644
--- a/src/include/catalog/pg_authid.h
+++ b/src/include/catalog/pg_authid.h
@@ -23,17 +23,6 @@
 #include "catalog/genbki.h"
 #include "catalog/pg_authid_d.h"

-/*
- * The CATALOG definition has to refer to the type of rolvaliduntil as
- * "timestamptz" (lower case) so that bootstrap mode recognizes it.  But
- * the C header files define this type as TimestampTz.  Since the field is
- * potentially-null and therefore can't be accessed directly from C code,
- * there is no particular need for the C struct definition to show the
- * field type as TimestampTz --- instead we just make it int.
- */
-#define timestamptz int
-
-
 /* ----------------
  *        pg_authid definition.  cpp turns this into
  *        typedef struct FormData_pg_authid
@@ -58,8 +47,6 @@ CATALOG(pg_authid,1260,AuthIdRelationId) BKI_SHARED_RELATION BKI_ROWTYPE_OID(284
 #endif
 } FormData_pg_authid;

-#undef timestamptz
-
 /* ----------------
  *        Form_pg_authid corresponds to a pointer to a tuple with
  *        the format of pg_authid relation.
diff --git a/src/include/catalog/pg_cast.h b/src/include/catalog/pg_cast.h
index e58eb8d..a9e7e2b 100644
--- a/src/include/catalog/pg_cast.h
+++ b/src/include/catalog/pg_cast.h
@@ -53,6 +53,8 @@ CATALOG(pg_cast,2605,CastRelationId)
  */
 typedef FormData_pg_cast *Form_pg_cast;

+#ifdef EXPOSE_TO_CLIENT_CODE
+
 /*
  * The allowable values for pg_cast.castcontext are specified by this enum.
  * Since castcontext is stored as a "char", we use ASCII codes for human
@@ -81,4 +83,6 @@ typedef enum CoercionMethod
     COERCION_METHOD_INOUT = 'i' /* use input/output functions */
 } CoercionMethod;

+#endif /* EXPOSE_TO_CLIENT_CODE */
+
 #endif                            /* PG_CAST_H */
diff --git a/src/include/catalog/pg_class.dat b/src/include/catalog/pg_class.dat
index ff972c0..e1450e3 100644
--- a/src/include/catalog/pg_class.dat
+++ b/src/include/catalog/pg_class.dat
@@ -12,9 +12,10 @@

 [

-# Note: only "bootstrapped" relations need to be declared here.  Be sure that
-# the OIDs listed here match those given in their CATALOG macros, and that
-# the relnatts values are correct.
+# Note: only "bootstrapped" relations, ie those marked BKI_BOOTSTRAP, need to
+# have entries here.  Be sure that the OIDs listed here match those given in
+# their CATALOG and BKI_ROWTYPE_OID macros, and that the relnatts values are
+# correct.

 # Note: "3" in the relfrozenxid column stands for FirstNormalTransactionId;
 # similarly, "1" in relminmxid stands for FirstMultiXactId
diff --git a/src/include/catalog/pg_db_role_setting.h b/src/include/catalog/pg_db_role_setting.h
index 0faa81f..896c410 100644
--- a/src/include/catalog/pg_db_role_setting.h
+++ b/src/include/catalog/pg_db_role_setting.h
@@ -1,7 +1,7 @@
 /*-------------------------------------------------------------------------
  *
  * pg_db_role_setting.h
- *    definition of configuration settings
+ *      definition of per-database/per-user configuration settings relation
  *
  *
  * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
@@ -18,6 +18,7 @@
 #ifndef PG_DB_ROLE_SETTING_H
 #define PG_DB_ROLE_SETTING_H

+#include "catalog/genbki.h"
 #include "catalog/pg_db_role_setting_d.h"
 #include "utils/guc.h"
 #include "utils/relcache.h"
diff --git a/src/include/catalog/pg_index.h b/src/include/catalog/pg_index.h
index 65c1110..b70ad73 100644
--- a/src/include/catalog/pg_index.h
+++ b/src/include/catalog/pg_index.h
@@ -64,6 +64,8 @@ CATALOG(pg_index,2610,IndexRelationId) BKI_WITHOUT_OIDS BKI_SCHEMA_MACRO
  */
 typedef FormData_pg_index *Form_pg_index;

+#ifdef EXPOSE_TO_CLIENT_CODE
+
 /*
  * Index AMs that support ordered scans must support these two indoption
  * bits.  Otherwise, the content of the per-column indoption fields is
@@ -72,6 +74,8 @@ typedef FormData_pg_index *Form_pg_index;
 #define INDOPTION_DESC            0x0001    /* values are in reverse order */
 #define INDOPTION_NULLS_FIRST    0x0002    /* NULLs are first instead of last */

+#endif /* EXPOSE_TO_CLIENT_CODE */
+
 /*
  * Use of these macros is recommended over direct examination of the state
  * flag columns where possible; this allows source code compatibility with
diff --git a/src/include/catalog/pg_largeobject.h b/src/include/catalog/pg_largeobject.h
index 07adca0..481d2ff 100644
--- a/src/include/catalog/pg_largeobject.h
+++ b/src/include/catalog/pg_largeobject.h
@@ -10,6 +10,8 @@
  * src/include/catalog/pg_largeobject.h
  *
  * NOTES
+ *      The Catalog.pm module reads this file and derives schema
+ *      information.
  *
  *-------------------------------------------------------------------------
  */
diff --git a/src/include/catalog/pg_operator.dat b/src/include/catalog/pg_operator.dat
index f8118ff..4382738 100644
--- a/src/include/catalog/pg_operator.dat
+++ b/src/include/catalog/pg_operator.dat
@@ -3054,8 +3054,6 @@
 { oid => '3681', descr => 'OR-concatenate',
   oprname => '||', oprleft => 'tsquery', oprright => 'tsquery',
   oprresult => 'tsquery', oprcode => 'tsquery_or' },
-
-# <-> operation calls tsquery_phrase, but function is polymorphic. So, point to OID of the tsquery_phrase
 { oid => '5005', descr => 'phrase-concatenate',
   oprname => '<->', oprleft => 'tsquery', oprright => 'tsquery',
   oprresult => 'tsquery', oprcode => 'tsquery_phrase(tsquery,tsquery)' },
diff --git a/src/include/catalog/pg_policy.h b/src/include/catalog/pg_policy.h
index 7ad0cde..45fdc28 100644
--- a/src/include/catalog/pg_policy.h
+++ b/src/include/catalog/pg_policy.h
@@ -7,6 +7,12 @@
  * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
+ * src/include/catalog/pg_policy.h
+ *
+ * NOTES
+ *      The Catalog.pm module reads this file and derives schema
+ *      information.
+ *
  *-------------------------------------------------------------------------
  */
 #ifndef PG_POLICY_H
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 43e24d2..5b5a1c0 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -30,7 +30,9 @@
 # "convert srctypename to desttypename" for cast functions
 # "less-equal-greater" for B-tree comparison functions

-# Keep the following ordered by OID so that later changes can be made easier.
+# Once upon a time these entries were ordered by OID.  Lately it's often
+# been the custom to insert new entries adjacent to related older entries.
+# Try to do one or the other though, don't just insert entries at random.

 # OIDS 1 - 99

diff --git a/src/include/catalog/pg_range.h b/src/include/catalog/pg_range.h
index 3762b3e..d8e16cc 100644
--- a/src/include/catalog/pg_range.h
+++ b/src/include/catalog/pg_range.h
@@ -35,7 +35,7 @@ CATALOG(pg_range,3541,RangeRelationId) BKI_WITHOUT_OIDS
     Oid            rngsubtype BKI_LOOKUP(pg_type);

     /* collation for this range type, or 0 */
-    Oid            rngcollation;
+    Oid            rngcollation BKI_DEFAULT(0);

     /* subtype's btree opclass */
     Oid            rngsubopc BKI_LOOKUP(pg_opclass);
diff --git a/src/include/catalog/pg_shdepend.h b/src/include/catalog/pg_shdepend.h
index 9f4dcb9..0f8508c 100644
--- a/src/include/catalog/pg_shdepend.h
+++ b/src/include/catalog/pg_shdepend.h
@@ -11,7 +11,6 @@
  * for example, there's not much value in creating an explicit dependency
  * from a relation to its database.  Currently, only dependencies on roles
  * are explicitly stored in pg_shdepend.
-
  *
  * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
diff --git a/src/include/catalog/pg_statistic.h b/src/include/catalog/pg_statistic.h
index 30d6868..c0ab74b 100644
--- a/src/include/catalog/pg_statistic.h
+++ b/src/include/catalog/pg_statistic.h
@@ -126,6 +126,8 @@ CATALOG(pg_statistic,2619,StatisticRelationId) BKI_WITHOUT_OIDS
  */
 typedef FormData_pg_statistic *Form_pg_statistic;

+#ifdef EXPOSE_TO_CLIENT_CODE
+
 /*
  * Several statistical slot "kinds" are defined by core PostgreSQL, as
  * documented below.  Also, custom data types can define their own "kind"
@@ -255,4 +257,6 @@ typedef FormData_pg_statistic *Form_pg_statistic;
  */
 #define STATISTIC_KIND_BOUNDS_HISTOGRAM  7

+#endif /* EXPOSE_TO_CLIENT_CODE */
+
 #endif                            /* PG_STATISTIC_H */
diff --git a/src/include/catalog/pg_statistic_ext.h b/src/include/catalog/pg_statistic_ext.h
index 24e4bd8..5d57b81 100644
--- a/src/include/catalog/pg_statistic_ext.h
+++ b/src/include/catalog/pg_statistic_ext.h
@@ -58,7 +58,11 @@ CATALOG(pg_statistic_ext,3381,StatisticExtRelationId)
  */
 typedef FormData_pg_statistic_ext *Form_pg_statistic_ext;

+#ifdef EXPOSE_TO_CLIENT_CODE
+
 #define STATS_EXT_NDISTINCT            'd'
 #define STATS_EXT_DEPENDENCIES        'f'

+#endif /* EXPOSE_TO_CLIENT_CODE */
+
 #endif                            /* PG_STATISTIC_EXT_H */
diff --git a/src/include/catalog/pg_subscription_rel.h b/src/include/catalog/pg_subscription_rel.h
index d82b262..033f5a1 100644
--- a/src/include/catalog/pg_subscription_rel.h
+++ b/src/include/catalog/pg_subscription_rel.h
@@ -12,9 +12,9 @@
 #ifndef PG_SUBSCRIPTION_REL_H
 #define PG_SUBSCRIPTION_REL_H

-#include "access/xlogdefs.h"
 #include "catalog/genbki.h"
 #include "catalog/pg_subscription_rel_d.h"
+#include "access/xlogdefs.h"
 #include "nodes/pg_list.h"

 /* ----------------
diff --git a/src/include/catalog/pg_trigger.h b/src/include/catalog/pg_trigger.h
index ee64bb4..7d60861 100644
--- a/src/include/catalog/pg_trigger.h
+++ b/src/include/catalog/pg_trigger.h
@@ -69,6 +69,8 @@ CATALOG(pg_trigger,2620,TriggerRelationId)
  */
 typedef FormData_pg_trigger *Form_pg_trigger;

+#ifdef EXPOSE_TO_CLIENT_CODE
+
 /* Bits within tgtype */
 #define TRIGGER_TYPE_ROW                (1 << 0)
 #define TRIGGER_TYPE_BEFORE                (1 << 1)
@@ -128,4 +130,6 @@ typedef FormData_pg_trigger *Form_pg_trigger;
 #define TRIGGER_USES_TRANSITION_TABLE(namepointer) \
     ((namepointer) != (char *) NULL)

+#endif /* EXPOSE_TO_CLIENT_CODE */
+
 #endif                            /* PG_TRIGGER_H */
diff --git a/src/include/catalog/pg_ts_parser.h b/src/include/catalog/pg_ts_parser.h
index 20b3a4b..ccaf40b 100644
--- a/src/include/catalog/pg_ts_parser.h
+++ b/src/include/catalog/pg_ts_parser.h
@@ -32,7 +32,7 @@ CATALOG(pg_ts_parser,3601,TSParserRelationId)
     NameData    prsname;

     /* name space */
-    Oid            prsnamespace;
+    Oid            prsnamespace BKI_DEFAULT(PGNSP);

     /* init parsing session */
     regproc        prsstart BKI_LOOKUP(pg_proc);
diff --git a/src/include/catalog/pg_ts_template.h b/src/include/catalog/pg_ts_template.h
index 5f5b580..5e66e02 100644
--- a/src/include/catalog/pg_ts_template.h
+++ b/src/include/catalog/pg_ts_template.h
@@ -32,7 +32,7 @@ CATALOG(pg_ts_template,3764,TSTemplateRelationId)
     NameData    tmplname;

     /* name space */
-    Oid            tmplnamespace;
+    Oid            tmplnamespace BKI_DEFAULT(PGNSP);

     /* initialization method of dict (may be 0) */
     regproc        tmplinit BKI_LOOKUP(pg_proc);
diff --git a/src/include/catalog/pg_type.dat b/src/include/catalog/pg_type.dat
index a430c79..3c2c813 100644
--- a/src/include/catalog/pg_type.dat
+++ b/src/include/catalog/pg_type.dat
@@ -12,20 +12,21 @@

 [

-# Keep the following ordered by OID so that later changes can be made more
-# easily.
-
 # For types used in the system catalogs, make sure the values here match
 # TypInfo[] in bootstrap.c.

-# The defined symbols for pg_type OIDs are generated by genbki.pl
+# OID symbol macro names for pg_type OIDs are generated by genbki.pl
 # according to the following rule, so you don't need to specify them
 # here:
 #  foo_bar  ->  FOO_BAROID
 # _foo_bar  ->  FOO_BARARRAYOID
 #
-# The only symbols in this file are ones that don't match this rule, and
-# are grandfathered in.
+# The only oid_symbol entries in this file are for names that don't match
+# this rule, and are grandfathered in.
+
+# Once upon a time these entries were ordered by OID.  Lately it's often
+# been the custom to insert new entries adjacent to related older entries.
+# Try to do one or the other though, don't just insert entries at random.

 # OIDS 1 - 99

diff --git a/src/include/catalog/pg_type.h b/src/include/catalog/pg_type.h
index 695ed4e..4bcfac9 100644
--- a/src/include/catalog/pg_type.h
+++ b/src/include/catalog/pg_type.h
@@ -92,7 +92,7 @@ CATALOG(pg_type,1247,TypeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(71,TypeRelati
     /* delimiter for arrays of this type */
     char        typdelim BKI_DEFAULT("\054");

-    /* 0 if not a composite type */
+    /* associated pg_class OID if a composite type, else 0 */
     Oid            typrelid BKI_DEFAULT(0);

     /*
@@ -246,7 +246,7 @@ typedef FormData_pg_type *Form_pg_type;
 #ifdef EXPOSE_TO_CLIENT_CODE

 /*
- * macros
+ * macros for values of poor-mans-enumerated-type columns
  */
 #define  TYPTYPE_BASE        'b' /* base type (ordinary scalar type) */
 #define  TYPTYPE_COMPOSITE    'c' /* composite (e.g., table's rowtype) */
diff --git a/src/include/catalog/reformat_dat_file.pl b/src/include/catalog/reformat_dat_file.pl
index f748a45..cb7a020 100644
--- a/src/include/catalog/reformat_dat_file.pl
+++ b/src/include/catalog/reformat_dat_file.pl
@@ -13,7 +13,7 @@
 # Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
 # Portions Copyright (c) 1994, Regents of the University of California
 #
-# /src/include/catalog/reformat_dat_file.pl
+# src/include/catalog/reformat_dat_file.pl
 #
 #----------------------------------------------------------------------