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

On Mon, Dec 6, 2021 at 10:14 AM Sadhuprasad Patro <b(dot)sadhu(at)gmail(dot)com> wrote:
>
> On Tue, Oct 26, 2021 at 6:55 PM Shruthi Gowda <gowdashru(at)gmail(dot)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'.

Replaced "db_oid" with "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 ?

I believe 'IsBinaryUpgrade' will be set to true when pg_upgrade is invoked.
pg_ugrade internally does pg_dump and pg_restore for every database in
the cluster.

> 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.

If a datafile with user-specified OID exists, the create database
fails with the below error.
postgres=# create database d2 oid 16452;
ERROR: could not create directory "base/16452": File exists

> 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?.

The comment is updated to explain why template0 oid is fixed.

> 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 "=".

Fixed

> 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.

As Robert confirmed, this part of the code is moved from a different place.

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

Fixed.

> 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. I have rebased relfilenode oid preserve patch. You may use the
rebased patch for review.

Thanks & Regards
Shruthi K C
EnterpriseDB: http://www.enterprisedb.com