Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce) - Mailing list pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Jan 22, 2022 at 2:20 AM Shruthi Gowda <gowdashru@gmail.com> wrote:
>> Agree. In the latest patch, the template0 and postgres OIDs are fixed
>> to unused manually assigned OIDs 4 and 5 respectively. These OIDs are
>> no more listed as unused OIDs.

> Thanks. Committed with a few more cosmetic changes.

I happened to take a closer look at this patch today, and I'm pretty
unhappy with the way that the assignment of those OIDs was managed.
There are two big problems:

1. IsPinnedObject() will now report that template0 and postgres are
pinned.  This seems not to prevent one from dropping them (I guess
dropdb() doesn't consult IsPinnedObject), but it would probably bollix
any pg_shdepend management that should happen for them.

2. The Catalog.pm infrastructure knows nothing about these OIDs.
While the unused_oids script was hack-n-slashed to claim that the
OIDs are used, other scripts won't know about them; for example
duplicate_oids won't report conflicts if someone tries to reuse
those OIDs.

The attached draft patch attempts to improve this situation.
It reserves these OIDs, and creates the associated macros, through
the normal BKI infrastructure by adding entries in pg_database.dat.
We have to delete those rows again during initdb, which is slightly
ugly but surely no more so than initdb's other direct manipulations
of pg_database.

There are a few loose ends:

* I'm a bit inclined to simplify IsPinnedObject by just teaching
it that *no* entries of pg_database are pinned, which would correspond
to the evident lack of enforcement in dropdb().  Can anyone see a
reason why we might pin some database in future?

* I had to set up the additional pg_database entries with nonzero
datfrozenxid to avoid an assertion failure during initdb's first VACUUM.
(That VACUUM will overwrite template1's datfrozenxid before computing
the global minimum frozen XID, but not these others; and it doesn't like
finding that the minimum is zero.)  This feels klugy.  An alternative is
to delete the extra pg_database rows before that VACUUM, which would
mean taking those deletes out of make_template0 and make_postgres and
putting them somewhere seemingly unrelated, so that's a bit crufty too.
Anybody have a preference?

* The new macro names seem ill-chosen.  Template0ObjectId is spelled
randomly differently from the longstanding TemplateDbOid, and surely
PostgresObjectId is about as vague a name as could possibly have
been thought of (please name an object that it couldn't apply to).
I'm a little inclined to rename TemplateDbOid to Template1DbOid and
use Template0DbOid and PostgresDbOid for the others, but I didn't pull
that trigger here.

Comments?

            regards, tom lane

diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 520f77971b..d7e5c02f95 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -339,9 +339,11 @@ IsPinnedObject(Oid classId, Oid objectId)
      * robustness.
      */

-    /* template1 is not pinned */
+    /* template1, template0, postgres are not pinned */
     if (classId == DatabaseRelationId &&
-        objectId == TemplateDbOid)
+        (objectId == TemplateDbOid ||
+         objectId == Template0ObjectId ||
+         objectId == PostgresObjectId))
         return false;

     /* the public namespace is not pinned */
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 1cb4a5b0d2..04454b3d7c 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -59,11 +59,11 @@
 #include "sys/mman.h"
 #endif

-#include "access/transam.h"
 #include "access/xlog_internal.h"
 #include "catalog/pg_authid_d.h"
 #include "catalog/pg_class_d.h" /* pgrminclude ignore */
 #include "catalog/pg_collation_d.h"
+#include "catalog/pg_database_d.h"    /* pgrminclude ignore */
 #include "common/file_perm.h"
 #include "common/file_utils.h"
 #include "common/logging.h"
@@ -1806,14 +1806,24 @@ make_template0(FILE *cmdfd)
      * objects in the old cluster, the problem scenario only exists if the OID
      * that is in use in the old cluster is also used in the new cluster - and
      * the new cluster should be the result of a fresh initdb.)
-     *
-     * We use "STRATEGY = file_copy" here because checkpoints during initdb
-     * are cheap. "STRATEGY = wal_log" would generate more WAL, which would
-     * be a little bit slower and make the new cluster a little bit bigger.
      */
     static const char *const template0_setup[] = {
-        "CREATE DATABASE template0 IS_TEMPLATE = true ALLOW_CONNECTIONS = false OID = "
-        CppAsString2(Template0ObjectId)
+        /*
+         * Since pg_database.dat includes a dummy row for template0, we have
+         * to remove that before creating the database for real.
+         */
+        "DELETE FROM pg_database WHERE datname = 'template0';\n\n",
+
+        /*
+         * Clone template1 to make template0.
+         *
+         * We use "STRATEGY = file_copy" here because checkpoints during
+         * initdb are cheap.  "STRATEGY = wal_log" would generate more WAL,
+         * which would be a little bit slower and make the new cluster a
+         * little bit bigger.
+         */
+        "CREATE DATABASE template0 IS_TEMPLATE = true ALLOW_CONNECTIONS = false"
+        " OID = " CppAsString2(Template0ObjectId)
         " STRATEGY = file_copy;\n\n",

         /*
@@ -1836,12 +1846,11 @@ make_template0(FILE *cmdfd)
         "REVOKE CREATE,TEMPORARY ON DATABASE template1 FROM public;\n\n",
         "REVOKE CREATE,TEMPORARY ON DATABASE template0 FROM public;\n\n",

-        "COMMENT ON DATABASE template0 IS 'unmodifiable empty database';\n\n",
-
         /*
-         * Finally vacuum to clean up dead rows in pg_database
+         * Note: postgres.bki filled in a comment for template0, so we need
+         * not do that here.
          */
-        "VACUUM pg_database;\n\n",
+
         NULL
     };

@@ -1858,12 +1867,19 @@ make_postgres(FILE *cmdfd)
     const char *const *line;

     /*
-     * Just as we did for template0, and for the same reasons, assign a fixed
-     * OID to postgres and select the file_copy strategy.
+     * Comments in make_template0() mostly apply here too.
      */
     static const char *const postgres_setup[] = {
-        "CREATE DATABASE postgres OID = " CppAsString2(PostgresObjectId) " STRATEGY = file_copy;\n\n",
-        "COMMENT ON DATABASE postgres IS 'default administrative connection database';\n\n",
+        "DELETE FROM pg_database WHERE datname = 'postgres';\n\n",
+
+        "CREATE DATABASE postgres OID = " CppAsString2(PostgresObjectId)
+        " STRATEGY = file_copy;\n\n",
+
+        /*
+         * Finally vacuum to clean up dead rows in pg_database
+         */
+        "VACUUM pg_database;\n\n",
+
         NULL
     };

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 969e2a7a46..bcb81e02c4 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -2844,10 +2844,11 @@ dumpDatabase(Archive *fout)
     qdatname = pg_strdup(fmtId(datname));

     /*
-     * Prepare the CREATE DATABASE command.  We must specify encoding, locale,
-     * and tablespace since those can't be altered later.  Other DB properties
-     * are left to the DATABASE PROPERTIES entry, so that they can be applied
-     * after reconnecting to the target DB.
+     * Prepare the CREATE DATABASE command.  We must specify OID (if we want
+     * to preserve that), as well as the encoding, locale, and tablespace
+     * since those can't be altered later.  Other DB properties are left to
+     * the DATABASE PROPERTIES entry, so that they can be applied after
+     * reconnecting to the target DB.
      */
     if (dopt->binary_upgrade)
     {
diff --git a/src/include/access/transam.h b/src/include/access/transam.h
index 9a2816de51..338dfca5a0 100644
--- a/src/include/access/transam.h
+++ b/src/include/access/transam.h
@@ -196,10 +196,6 @@ FullTransactionIdAdvance(FullTransactionId *dest)
 #define FirstUnpinnedObjectId    12000
 #define FirstNormalObjectId        16384

-/* OIDs of Template0 and Postgres database are fixed */
-#define Template0ObjectId        4
-#define PostgresObjectId        5
-
 /*
  * VariableCache is a data structure in shared memory that is used to track
  * OID and XID assignment state.  For largely historical reasons, there is
diff --git a/src/include/catalog/pg_database.dat b/src/include/catalog/pg_database.dat
index 5feedff7bf..6c2221a4e9 100644
--- a/src/include/catalog/pg_database.dat
+++ b/src/include/catalog/pg_database.dat
@@ -19,4 +19,24 @@
   datminmxid => '1', dattablespace => 'pg_default', datcollate => 'LC_COLLATE',
   datctype => 'LC_CTYPE', daticulocale => 'ICU_LOCALE', datacl => '_null_' },

+# The template0 and postgres entries are included here to reserve their
+# associated OIDs.  We show their correct properties for reference, but
+# note that these pg_database rows are removed and replaced by initdb.
+# Unlike the row for template1, their datfrozenxids will not be overwritten
+# during initdb's first VACUUM, so we have to provide normal-looking XIDs.
+
+{ oid => '4', oid_symbol => 'Template0ObjectId',
+  descr => 'unmodifiable empty database',
+  datname => 'template0', encoding => 'ENCODING', datlocprovider => 'LOCALE_PROVIDER', datistemplate => 't',
+  datallowconn => 'f', datconnlimit => '-1', datfrozenxid => '4',
+  datminmxid => '1', dattablespace => 'pg_default', datcollate => 'LC_COLLATE',
+  datctype => 'LC_CTYPE', daticulocale => 'ICU_LOCALE', datacl => '_null_' },
+
+{ oid => '5', oid_symbol => 'PostgresObjectId',
+  descr => 'default administrative connection database',
+  datname => 'postgres', encoding => 'ENCODING', datlocprovider => 'LOCALE_PROVIDER', datistemplate => 'f',
+  datallowconn => 't', datconnlimit => '-1', datfrozenxid => '4',
+  datminmxid => '1', dattablespace => 'pg_default', datcollate => 'LC_COLLATE',
+  datctype => 'LC_CTYPE', daticulocale => 'ICU_LOCALE', datacl => '_null_' },
+
 ]
diff --git a/src/include/catalog/unused_oids b/src/include/catalog/unused_oids
index 61d41e7561..e55bc6fa3c 100755
--- a/src/include/catalog/unused_oids
+++ b/src/include/catalog/unused_oids
@@ -32,15 +32,6 @@ my @input_files = glob("pg_*.h");

 my $oids = Catalog::FindAllOidsFromHeaders(@input_files);

-# Push the template0 and postgres database OIDs.
-my $Template0ObjectId =
-  Catalog::FindDefinedSymbol('access/transam.h', '..', 'Template0ObjectId');
-push @{$oids}, $Template0ObjectId;
-
-my $PostgresObjectId =
-  Catalog::FindDefinedSymbol('access/transam.h', '..', 'PostgresObjectId');
-push @{$oids}, $PostgresObjectId;
-
 # Also push FirstGenbkiObjectId to serve as a terminator for the last gap.
 my $FirstGenbkiObjectId =
   Catalog::FindDefinedSymbol('access/transam.h', '..', 'FirstGenbkiObjectId');