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

On Thu, Jan 20, 2022 at 7:57 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Jan 20, 2022 at 7:09 AM Shruthi Gowda <gowdashru@gmail.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).

I have rebased the patch. Kindly have a 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".

Fixed

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

The second error message will be reported when the data directory with
the given OID exists in the data path but the corresponding DB does
not exist. This could happen only if the user creates directories in
the data folder which is indeed not an invalid usage. I have updated
the error message to "data directory with the specified OID %u already
exists" because the error message you recommended gives a slightly
different meaning.

> + * 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."

Fixed

> + /*
> + * 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.

Removed

> +# 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?

It is not required for PostgresObjectId. The unused_oids script
provides a list of unused oids in the manually-assignable OIDs range
(1-9999).

Shruthi KC
EnterpriseDB: http://www.enterprisedb.com