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