Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)

On Tue, Oct 26, 2021 at 6:55 PM Shruthi Gowda <gowdashru@gmail.com> wrote:
>
>
> I have revised the patch w.r.t the way 'create_storage' is interpreted
> in heap_create() along with some minor changes to preserve the DBOID
> patch.
>

Hi Shruthi,

I am reviewing the attached patches and providing a few comments here
below for patch "v5-0002-Preserve-database-OIDs-in-pg_upgrade.patch"

1.
--- a/doc/src/sgml/ref/create_database.sgml
+++ b/doc/src/sgml/ref/create_database.sgml
@@ -31,7 +31,8 @@ CREATE DATABASE <replaceable
class="parameter">name</replaceable>
-           [ IS_TEMPLATE [=] <replaceable
class="parameter">istemplate</replaceable> ] ]
+           [ IS_TEMPLATE [=] <replaceable
class="parameter">istemplate</replaceable> ]
+           [ OID [=] <replaceable
class="parameter">db_oid</replaceable> ] ]

Replace "db_oid" with 'oid'. Below in the listitem, we have mentioned 'oid'.

2.
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
+ if ((dboid < FirstNormalObjectId) &&
+ (strcmp(dbname, "template0") != 0) &&
+ (!IsBinaryUpgrade))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("Invalid value for option \"%s\"", defel->defname),
+ errhint("The specified OID %u is less than the minimum OID for user
objects %u.",
+ dboid, FirstNormalObjectId));
+ }

Are we sure that 'IsBinaryUpgrade' will be set properly, before the
createdb function is called? Can we recheck once ?

3.
@@ -504,11 +525,15 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
  */
  pg_database_rel = table_open(DatabaseRelationId, RowExclusiveLock);

- do
+ /* Select an OID for the new database if is not explicitly configured. */
+ if (!OidIsValid(dboid))
  {
- dboid = GetNewOidWithIndex(pg_database_rel, DatabaseOidIndexId,
-    Anum_pg_database_oid);
- } while (check_db_file_conflict(dboid));

I think we need to do 'check_db_file_conflict' for the USER given OID
also.. right? It may already be present.

4.
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c

/*
+ * Create template0 database with oid Template0ObjectId i.e, 4
+ */
+

Better to mention here, why OID 4 is reserved for template0 database?.

5.
+ /*
+ * Create template0 database with oid Template0ObjectId i.e, 4
+ */
+ static const char *const template0_setup[] = {
+ "CREATE DATABASE template0 IS_TEMPLATE = true ALLOW_CONNECTIONS = false OID "
+ CppAsString2(Template0ObjectId) ";\n\n",

Can we write something like, 'OID = CppAsString2(Template0ObjectId)'?
mention "=".

6.
+
+ /*
+ * We use the OID of postgres to determine datlastsysoid
+ */
+ "UPDATE pg_database SET datlastsysoid = "
+ "    (SELECT oid FROM pg_database "
+ "    WHERE datname = 'postgres');\n\n",
+

Make the above comment a single line comment.

7.
There are some spelling mistakes in the comments as below, please
correct the same
+ /*
+ * Make sure that binary upgrade propogate the database OID to the
new                 =====> correct spelling
+ * cluster
+ */

+/* OID 4 is reserved for Templete0 database */
 ====> Correct spelling
+#define Template0ObjectId 4


I am reviewing another patch
"v5-0001-Preserve-relfilenode-and-tablespace-OID-in-pg_upg" as well
and will provide the comments soon if any...

Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com