Re: Add primary keys to system catalogs - Mailing list pgsql-hackers

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2021-01-17 23:07, Tom Lane wrote:
>> I've reviewed this patch.  It looks pretty solid to me, with a couple
>> trivial nits as mentioned below, and one bigger thing that's perhaps
>> in the category of bikeshedding.  Namely, do we really want to prefer
>> using the OID indexes as the primary keys?

> I chose this because the notional foreign keys point to the OID.
> If you design some basic business database with customer IDs, product 
> IDs, etc., you'd also usually make the ID the primary key, even if you 
> have, say, a unique constraint on the product name.  But this is of 
> course a matter of taste to some degree.

Fair enough.  As I said upthread, I just wanted to be sure we'd considered
the alternative.  I'm content to use the OIDs as pkeys, although I think
that decision should be explicitly recorded somewhere (cf attachment).

>> The contents of system_constraints.sql seem pretty randomly ordered,
>> and I bet the order isn't stable across machines.

> They follow the order in which the catalogs are processed byt genbki.pl. 

Looking closer, I see the data structure is an array not a hash, so
I withdraw the concern about instability.

After reading the patch again, I have a couple more nits about comments,
which I'll just present as a proposed delta patch.  Otherwise it's good.
I'll mark it RFC.

            regards, tom lane

diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 6747996ce3..b68c1752c0 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -663,6 +663,8 @@ die
   "genbki OID counter reached $GenbkiNextOid, overrunning FirstBootstrapObjectId\n"
   if $GenbkiNextOid > $FirstBootstrapObjectId;

+# Now generate system_constraints.sql
+
 foreach my $c (@system_constraints)
 {
     print $constraints $c, "\n";
diff --git a/src/include/catalog/genbki.h b/src/include/catalog/genbki.h
index 296765d987..5d05fafb5d 100644
--- a/src/include/catalog/genbki.h
+++ b/src/include/catalog/genbki.h
@@ -55,12 +55,15 @@
 #define DECLARE_TOAST(name,toastoid,indexoid) extern int no_such_variable

 /*
- * These lines processed by genbki.pl to create the statements
+ * These lines are processed by genbki.pl to create the statements
  * the bootstrap parser will turn into DefineIndex calls.
  *
- * The keyword is DECLARE_INDEX or DECLARE_UNIQUE_INDEX or DECLARE_UNIQUE_INDEX_PKEY.  The first two
- * arguments are the index name and OID, the rest is much like a standard
- * 'create index' SQL command.
+ * The keyword is DECLARE_INDEX or DECLARE_UNIQUE_INDEX or
+ * DECLARE_UNIQUE_INDEX_PKEY.  ("PKEY" marks the index as being the catalog's
+ * primary key; currently this is only cosmetically different from a regular
+ * unique index.  By convention, we usually make a catalog's OID column its
+ * pkey, if it has one.)  The first two arguments are the index's name and
+ * OID, the rest is much like a standard 'create index' SQL command.
  *
  * For each index, we also provide a #define for its OID.  References to
  * the index in the C code should always use these #defines, not the actual
diff --git a/src/test/regress/expected/misc_sanity.out b/src/test/regress/expected/misc_sanity.out
index ae85817c61..9ebe28a78d 100644
--- a/src/test/regress/expected/misc_sanity.out
+++ b/src/test/regress/expected/misc_sanity.out
@@ -43,6 +43,9 @@ WHERE refclassid = 0 OR refobjid = 0 OR
 -- whatever OID the test is complaining about must have been added later
 -- in initdb, where it intentionally isn't pinned.  Legitimate exceptions
 -- to that rule are listed in the comments in setup_depend().
+-- Currently, pg_rewrite is also listed by this check, even though it is
+-- covered by setup_depend().  That happens because there are no rules in
+-- the pinned data, but initdb creates some intentionally-not-pinned views.
 do $$
 declare relnm text;
   reloid oid;
diff --git a/src/test/regress/sql/misc_sanity.sql b/src/test/regress/sql/misc_sanity.sql
index ea80cf1f10..9699f5cc3b 100644
--- a/src/test/regress/sql/misc_sanity.sql
+++ b/src/test/regress/sql/misc_sanity.sql
@@ -44,6 +44,9 @@ WHERE refclassid = 0 OR refobjid = 0 OR
 -- whatever OID the test is complaining about must have been added later
 -- in initdb, where it intentionally isn't pinned.  Legitimate exceptions
 -- to that rule are listed in the comments in setup_depend().
+-- Currently, pg_rewrite is also listed by this check, even though it is
+-- covered by setup_depend().  That happens because there are no rules in
+-- the pinned data, but initdb creates some intentionally-not-pinned views.

 do $$
 declare relnm text;