public inbox for [email protected]
help / color / mirror / Atom feedFrom: Shruthi Gowda <[email protected]>
To: Robert Haas <[email protected]>
Cc: Julien Rouhaud <[email protected]>
Cc: Sadhuprasad Patro <[email protected]>
Cc: Stephen Frost <[email protected]>
Cc: Bruce Momjian <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Tom Kincaid <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: PostgreSQL Development <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Tom Lane <[email protected]>
Subject: Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
Date: Thu, 20 Jan 2022 21:33:23 +0530
Message-ID: <CAASxf_Oj8cSJonwsWMtGyUTkK-EhmygW0HGTSXb1jCq_beAwkQ@mail.gmail.com> (raw)
In-Reply-To: <CA+Tgmobqtiq13Td2ZaV1tMk2U8mi4=tAA2PAhpigP1NHDqzj9Q@mail.gmail.com>
References: <CA+TgmoZtbAnybMwni-u-9LArqYhsvpeF48itsJP=otrvE_ZzSQ@mail.gmail.com>
<CAASxf_Mj3SSXFmtd9O6sf-+y4PJz+oZ57RAi0bRKKLAOBDcorA@mail.gmail.com>
<CA+TgmoZhgeXfrT5zOUkVisPQ-cuYRg2M9MBG7WrE0sgcUKQVWg@mail.gmail.com>
<CAASxf_P+EFr++8Vmfjb88HMEcCRS6NbXXQhFath0oXLnoHwJSA@mail.gmail.com>
<CA+TgmoZ9ee+RHdFXvJ9obHfwJvpxRMN=dxrJNWoktc+QBj=XVw@mail.gmail.com>
<CAASxf_Nzsprv-JxO=8QDzK226iYMu0fspgRD4RoYQ=ddcO0Xqw@mail.gmail.com>
<CAFF0-CEPkpnc+SbPxqm0Q6A6cKjmNN+PndYf21+T+gQtiM0BGg@mail.gmail.com>
<CA+TgmoYbCSQzwBsn-33Pk=Y34uovR_bt9dVrZVMX0jHgZMWG1A@mail.gmail.com>
<CAASxf_Pqp44ERghe3JkiDoxNhf7DCDg-=emq1ZV3TVKTKajLmA@mail.gmail.com>
<CAASxf_Nmjh-Do92L6RXjjYGMnUZKx7RK1D266xRWhKjPA_7Egw@mail.gmail.com>
<20220115054719.yeqoy2mdllbc7wq2@jrouhaud>
<CAASxf_OL+GtnnfHJ6-KtgHFFskv_btd5_wn_B39HvOyQTMXNBw@mail.gmail.com>
<CA+TgmoaOFR=3eWScWrDDw0m9u-a+CaFXrZNwgnJfo7g9_mP_nA@mail.gmail.com>
<CAASxf_O4+cZeGb=W98=WEp0R=d+=Um7PM=8h7Ec-qZ=tJR1EEQ@mail.gmail.com>
<CA+Tgmobqtiq13Td2ZaV1tMk2U8mi4=tAA2PAhpigP1NHDqzj9Q@mail.gmail.com>
On Thu, Jan 20, 2022 at 7:57 PM Robert Haas <[email protected]> wrote:
>
> On Thu, Jan 20, 2022 at 7:09 AM Shruthi Gowda <[email protected]> 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
Attachments:
[application/octet-stream] v11-0001-pg_upgrade-perserve-database-OID-patch.patch (12.8K, ../CAASxf_Oj8cSJonwsWMtGyUTkK-EhmygW0HGTSXb1jCq_beAwkQ@mail.gmail.com/2-v11-0001-pg_upgrade-perserve-database-OID-patch.patch)
download | inline diff:
From 808e4d43d7e282fa98eba7d7084a0927b5cafc93 Mon Sep 17 00:00:00 2001
From: shruthikc-gowda <[email protected]>
Date: Thu, 20 Jan 2022 21:15:46 +0530
Subject: [PATCH v11] pg_upgrade perserve database OID patch
---
doc/src/sgml/ref/create_database.sgml | 18 ++++++++++-
src/backend/commands/dbcommands.c | 57 ++++++++++++++++++++++++++++++++---
src/bin/initdb/initdb.c | 22 ++++++++++++--
src/bin/pg_dump/pg_dump.c | 12 ++++++--
src/bin/pg_upgrade/IMPLEMENTATION | 2 +-
src/bin/pg_upgrade/info.c | 9 ++----
src/bin/pg_upgrade/pg_upgrade.h | 3 +-
src/bin/pg_upgrade/relfilenode.c | 4 +--
src/bin/psql/tab-complete.c | 2 +-
src/include/access/transam.h | 4 +++
src/include/catalog/unused_oids | 5 +++
11 files changed, 116 insertions(+), 22 deletions(-)
diff --git a/doc/src/sgml/ref/create_database.sgml b/doc/src/sgml/ref/create_database.sgml
index 41cb406..f22e28d 100644
--- 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>
[ TABLESPACE [=] <replaceable class="parameter">tablespace_name</replaceable> ]
[ ALLOW_CONNECTIONS [=] <replaceable class="parameter">allowconn</replaceable> ]
[ CONNECTION LIMIT [=] <replaceable class="parameter">connlimit</replaceable> ]
- [ IS_TEMPLATE [=] <replaceable class="parameter">istemplate</replaceable> ] ]
+ [ IS_TEMPLATE [=] <replaceable class="parameter">istemplate</replaceable> ]
+ [ OID [=] <replaceable class="parameter">oid</replaceable> ] ]
</synopsis>
</refsynopsisdiv>
@@ -203,6 +204,21 @@ CREATE DATABASE <replaceable class="parameter">name</replaceable>
</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term><replaceable class="parameter">oid</replaceable></term>
+ <listitem>
+ <para>
+ The object identifier to be used for the new database. If this
+ parameter is not specified, the database will choose a suitable
+ OID automatically. This parameter is primarily intended for internal
+ use by <application>pg_upgrade</application>, and only
+ <application>pg_upgrade</application> can specify a value less
+ than 16384.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
<para>
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index da83455..02529e5 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -115,7 +115,7 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
HeapTuple tuple;
Datum new_record[Natts_pg_database];
bool new_record_nulls[Natts_pg_database];
- Oid dboid;
+ Oid dboid = InvalidOid;
Oid datdba;
ListCell *option;
DefElem *dtablespacename = NULL;
@@ -215,6 +215,30 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
errhint("Consider using tablespaces instead."),
parser_errposition(pstate, defel->location)));
}
+ else if (strcmp(defel->defname, "oid") == 0)
+ {
+ dboid = defGetInt32(defel);
+
+ /*
+ * We don't normally permit new databases to be created with
+ * system-assigned OIDs. pg_upgrade tries to preserve database
+ * OIDs, and we don't want any OIDs that are used in this database
+ * cluster to be OIDs that might be used for some other cluster in
+ * a future version of PostgreSQL. We assume all such OIDs will be
+ * from the system-managed OID range.
+ *
+ * As an exception, however, we permit any OID to be assigned when
+ * allow_system_table_mods=on (so that initdb can assign system
+ * OIDs to template0 and postgres) or when performing a binary
+ * upgrade (so that pg_upgrade can preserve whatever OIDs it finds
+ * in the source cluster).
+ */
+ if (dboid < FirstNormalObjectId &&
+ !allowSystemTableMods && !IsBinaryUpgrade)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("OIDs less than %u are reserved for system objects", FirstNormalObjectId));
+ }
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -502,11 +526,34 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
*/
pg_database_rel = table_open(DatabaseRelationId, RowExclusiveLock);
- do
+ /*
+ * If database OID is configured, check if the OID is already in use or
+ * data directory already exists.
+ */
+ if (OidIsValid(dboid))
{
- dboid = GetNewOidWithIndex(pg_database_rel, DatabaseOidIndexId,
- Anum_pg_database_oid);
- } while (check_db_file_conflict(dboid));
+ char *existing_dbname = get_database_name(dboid);
+
+ if (existing_dbname != NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("database OID %u is already in use by database \"%s\"",
+ dboid, existing_dbname));
+
+ if (check_db_file_conflict(dboid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("data directory with the specified OID %u already exists", dboid));
+ }
+ else
+ {
+ /* Select an OID for the new database if is not explicitly configured. */
+ do
+ {
+ dboid = GetNewOidWithIndex(pg_database_rel, DatabaseOidIndexId,
+ Anum_pg_database_oid);
+ } while (check_db_file_conflict(dboid));
+ }
/*
* Insert a new tuple into pg_database. This establishes our ownership of
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 0a2dba7..7e03b51 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -59,6 +59,7 @@
#include "sys/mman.h"
#endif
+#include "access/transam.h"
#include "access/xlog_internal.h"
#include "catalog/pg_authid_d.h"
#include "catalog/pg_class_d.h" /* pgrminclude ignore */
@@ -1838,8 +1839,23 @@ static void
make_template0(FILE *cmdfd)
{
const char *const *line;
+
+ /*
+ * pg_upgrade tries to preserve database OIDs across upgrades. It's smart
+ * enough to drop and recreate a conflicting database with the same name,
+ * but if the same OID were used for one system-created database in the
+ * old cluster and a different system-created database in the new cluster,
+ * it would fail. To avoid that, assign a fixed OID to template0 rather
+ * than letting the server choose one.
+ *
+ * (Note that, while the user could have dropped and recreated these
+ * objects in the old cluster, the problem scenario only exists if the OID
+ * that is in use in the old cluster is also used in th new cluster - and
+ * the new cluster should be the result of a fresh initdb.)
+ */
static const char *const template0_setup[] = {
- "CREATE DATABASE template0 IS_TEMPLATE = true ALLOW_CONNECTIONS = false;\n\n",
+ "CREATE DATABASE template0 IS_TEMPLATE = true ALLOW_CONNECTIONS = false OID = "
+ CppAsString2(Template0ObjectId) ";\n\n",
/*
* Explicitly revoke public create-schema and create-temp-table
@@ -1869,8 +1885,10 @@ static void
make_postgres(FILE *cmdfd)
{
const char *const *line;
+
+ /* Assign a fixed OID to postgres, for the same reasons as template0 */
static const char *const postgres_setup[] = {
- "CREATE DATABASE postgres;\n\n",
+ "CREATE DATABASE postgres OID = " CppAsString2(PostgresObjectId) ";\n\n",
"COMMENT ON DATABASE postgres IS 'default administrative connection database';\n\n",
NULL
};
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7c2f1d3..d2847c7 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -2838,8 +2838,16 @@ dumpDatabase(Archive *fout)
* are left to the DATABASE PROPERTIES entry, so that they can be applied
* after reconnecting to the target DB.
*/
- appendPQExpBuffer(creaQry, "CREATE DATABASE %s WITH TEMPLATE = template0",
- qdatname);
+ if (dopt->binary_upgrade)
+ {
+ appendPQExpBuffer(creaQry, "CREATE DATABASE %s WITH TEMPLATE = template0 OID = %u",
+ qdatname, dbCatId.oid);
+ }
+ else
+ {
+ appendPQExpBuffer(creaQry, "CREATE DATABASE %s WITH TEMPLATE = template0",
+ qdatname);
+ }
if (strlen(encoding) > 0)
{
appendPQExpBufferStr(creaQry, " ENCODING = ");
diff --git a/src/bin/pg_upgrade/IMPLEMENTATION b/src/bin/pg_upgrade/IMPLEMENTATION
index 69fcd70..384834a 100644
--- a/src/bin/pg_upgrade/IMPLEMENTATION
+++ b/src/bin/pg_upgrade/IMPLEMENTATION
@@ -84,7 +84,7 @@ cluster using the first part of the pg_dumpall output.
Next, pg_upgrade executes the remainder of the script produced earlier
by pg_dumpall --- this script effectively creates the complete
user-defined metadata from the old cluster to the new cluster. It
-preserves the relfilenode numbers so TOAST and other references
+preserves the DB, tablespace, relfilenode OIDs so TOAST and other references
to relfilenodes in user data is preserved. (See binary-upgrade usage
in pg_dump).
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index f7fa082..69ef231 100644
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -190,10 +190,8 @@ create_rel_filename_map(const char *old_data, const char *new_data,
map->new_tablespace_suffix = new_cluster.tablespace_suffix;
}
- map->old_db_oid = old_db->db_oid;
- map->new_db_oid = new_db->db_oid;
-
- /* relfilenode is preserved across old and new cluster */
+ /* DB oid and relfilenodes are preserved between old and new cluster */
+ map->db_oid = old_db->db_oid;
map->relfilenode = old_rel->relfilenode;
/* used only for logging and error reporting, old/new are identical */
@@ -324,8 +322,7 @@ get_db_infos(ClusterInfo *cluster)
" LEFT OUTER JOIN pg_catalog.pg_tablespace t "
" ON d.dattablespace = t.oid "
"WHERE d.datallowconn = true "
- /* we don't preserve pg_database.oid so we sort by name */
- "ORDER BY 2");
+ "ORDER BY 1");
res = executeQueryOrDie(conn, "%s", query);
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index da6770d..1db8e3f 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -145,8 +145,7 @@ typedef struct
const char *new_tablespace;
const char *old_tablespace_suffix;
const char *new_tablespace_suffix;
- Oid old_db_oid;
- Oid new_db_oid;
+ Oid db_oid;
Oid relfilenode;
/* the rest are used only for logging and error reporting */
char *nspname; /* namespaces */
diff --git a/src/bin/pg_upgrade/relfilenode.c b/src/bin/pg_upgrade/relfilenode.c
index 6e0253c..2f4deb3 100644
--- a/src/bin/pg_upgrade/relfilenode.c
+++ b/src/bin/pg_upgrade/relfilenode.c
@@ -193,14 +193,14 @@ transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_fro
snprintf(old_file, sizeof(old_file), "%s%s/%u/%u%s%s",
map->old_tablespace,
map->old_tablespace_suffix,
- map->old_db_oid,
+ map->db_oid,
map->relfilenode,
type_suffix,
extent_suffix);
snprintf(new_file, sizeof(new_file), "%s%s/%u/%u%s%s",
map->new_tablespace,
map->new_tablespace_suffix,
- map->new_db_oid,
+ map->db_oid,
map->relfilenode,
type_suffix,
extent_suffix);
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 6bd33a0..502b5c5 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2633,7 +2633,7 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH("OWNER", "TEMPLATE", "ENCODING", "TABLESPACE",
"IS_TEMPLATE",
"ALLOW_CONNECTIONS", "CONNECTION LIMIT",
- "LC_COLLATE", "LC_CTYPE", "LOCALE");
+ "LC_COLLATE", "LC_CTYPE", "LOCALE", "OID");
else if (Matches("CREATE", "DATABASE", MatchAny, "TEMPLATE"))
COMPLETE_WITH_QUERY(Query_for_list_of_template_databases);
diff --git a/src/include/access/transam.h b/src/include/access/transam.h
index 338dfca..a9c25fc 100644
--- a/src/include/access/transam.h
+++ b/src/include/access/transam.h
@@ -196,6 +196,10 @@ FullTransactionIdAdvance(FullTransactionId *dest)
#define FirstUnpinnedObjectId 12000
#define FirstNormalObjectId 16384
+/* OIDs of Template0 and Postgres database are fixed */
+#define Template0ObjectId 4
+#define PostgresObjectId 16000
+
/*
* VariableCache is a data structure in shared memory that is used to track
* OID and XID assignment state. For largely historical reasons, there is
diff --git a/src/include/catalog/unused_oids b/src/include/catalog/unused_oids
index e55bc6f..314208b 100755
--- a/src/include/catalog/unused_oids
+++ b/src/include/catalog/unused_oids
@@ -32,6 +32,11 @@ my @input_files = glob("pg_*.h");
my $oids = Catalog::FindAllOidsFromHeaders(@input_files);
+# Push the OID that is reserved for template0 database.
+my $Template0ObjectId =
+ Catalog::FindDefinedSymbol('access/transam.h', '..', 'Template0ObjectId');
+push @{$oids}, $Template0ObjectId;
+
# Also push FirstGenbkiObjectId to serve as a terminator for the last gap.
my $FirstGenbkiObjectId =
Catalog::FindDefinedSymbol('access/transam.h', '..', 'FirstGenbkiObjectId');
--
1.8.3.1
view thread (3+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
In-Reply-To: <CAASxf_Oj8cSJonwsWMtGyUTkK-EhmygW0HGTSXb1jCq_beAwkQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox