On Thu, Jan 20, 2022 at 7:09 AM Shruthi Gowda <gowdas...@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