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

On Thu, Jan 20, 2022 at 7:09 AM Shruthi Gowda <gowdashru(at)gmail(dot)com> wrote:
> > Here's an updated version in which I've reverted the changes to gram.y
> > and tried to improve the comments and documentation. Could you have a
> > look at implementing (2) above?
>
> Attached is the patch that implements comment (2).

This probably needs minor rebasing on account of the fact that I just
pushed the patch to remove datlastsysoid. I intended to do that before
you posted a new version to save you the trouble, but I was too slow
(or you were too fast, however you want to look at it).

+ errmsg("Invalid value for option \"%s\"", defel->defname),

Per the "error message style" section of the documentation, primary
error messages neither begin with a capital letter nor end with a
period, while errdetail() messages are complete sentences and thus
both begin with a capital letter and end with a period. But what I
think you should really do here is get rid of the error detail and
convey all the information in a primary error message. e.g. "OID %u is
a system OID", or maybe better, "OIDs less than %u are reserved for
system objects".

+ errmsg("database oid %u is already used by database %s",
+ errmsg("data directory exists for database oid %u", dboid));

Usually we write "OID" rather than "oid" in error messages. I think
maybe it would be best to change the text slightly too. I suggest:

database OID %u is already in use by database \"%s\"
data directory already exists for database with OID %u

+ * it would fail. To avoid that, assign a fixed OID to template0 and
+ * postgres rather than letting the server choose one.

a fixed OID -> fixed OIDs
one -> them

Or maybe put this comment back the way I had it and just talk about
postgres, and then change the comment in make_postgres to say "Assign
a fixed OID to postgres, for the same reasons as template0."

+ /*
+ * Make sure that binary upgrade propagate the database OID to the new
+ * cluster
+ */

This comment doesn't really seem necessary. It's sort of self-explanatory.

+# Push the OID that is reserved for template0 database.
+my $Template0ObjectId =
+ Catalog::FindDefinedSymbol('access/transam.h', '..', 'Template0ObjectId');
+push @{$oids}, $Template0ObjectId;

Don't you need to do this for PostgresObjectId also?

--
Robert Haas
EDB: http://www.enterprisedb.com