On Mon, Dec 6, 2021 at 10:14 AM Sadhuprasad Patro <b.sa...@gmail.com> wrote: > > On Tue, Oct 26, 2021 at 6:55 PM Shruthi Gowda <gowdas...@gmail.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
v6-0001-Preserve-relfilenode-and-tablespace-OID-in-pg_upg.patch
Description: Binary data
v6-0002-Preserve-database-OIDs-in-pg_upgrade.patch
Description: Binary data
