pg_upgrade allows itself to be run twice
Now that I've gotten your attention..
I expect pg_upgrade to fail if I run it twice in a row.
It would be reasonable if it were to fail during the "--check" phase,
maybe by failing like this:
| New cluster database "..." is not empty: found relation "..."
But that fails to happen if the cluster has neither tables nor matviews, in
which case, it passes the check phase and then fails like this:
Performing Upgrade
------------------
Analyzing all rows in the new cluster ok
Freezing all rows in the new cluster ok
Deleting files from new pg_xact ok
Copying old pg_clog to new server ok
Setting oldest XID for new cluster ok
Setting next transaction ID and epoch for new cluster ok
Deleting files from new pg_multixact/offsets ok
Copying old pg_multixact/offsets to new server ok
Deleting files from new pg_multixact/members ok
Copying old pg_multixact/members to new server ok
Setting next multixact ID and offset for new cluster ok
Resetting WAL archives ok
Setting frozenxid and minmxid counters in new cluster connection to server on socket "/home/pryzbyj/src/postgres/.s.PGSQL.50432" failed: FATAL: could not open relation with OID 2610
Failure, exiting
I'll concede that a cluster which has no tables sounds a lot like a toy, but I
find it disturbing that nothing prevents running the process twice, up to the
point that it's evidently corrupted the catalog.
While looking at this, I noticed that starting postgres --single immediately
after initdb allows creating objects with OIDs below FirstNormalObjectId
(thereby defeating pg_upgrade's check). I'm not familiar with the behavioral
differences of single user mode, and couldn't find anything in the
documentation.
On Sat, Jun 25, 2022 at 11:04:37AM -0500, Justin Pryzby wrote:
I expect pg_upgrade to fail if I run it twice in a row.
Yep.
It would be reasonable if it were to fail during the "--check" phase,
maybe by failing like this:
| New cluster database "..." is not empty: found relation "..."
So, we get a complaint that the new cluster is not empty after one
pg_upgrade run with a new command of pg_upgrade, with or without
--check. This happens in check_new_cluster(), where we'd fatal if a
relation uses a namespace different than pg_catalog.
But that fails to happen if the cluster has neither tables nor matviews, in
which case, it passes the check phase and then fails like this:
Indeed, as of get_rel_infos().
I'll concede that a cluster which has no tables sounds a lot like a toy, but I
find it disturbing that nothing prevents running the process twice, up to the
point that it's evidently corrupted the catalog.
I have heard of cases where instances were only used with a set of
foreign tables, for example. Not sure that this is spread enough to
worry about, but this would fail as much as your case.
While looking at this, I noticed that starting postgres --single immediately
after initdb allows creating objects with OIDs below FirstNormalObjectId
(thereby defeating pg_upgrade's check). I'm not familiar with the behavioral
differences of single user mode, and couldn't find anything in the
documentation.
This one comes from NextOID in the control data file after a fresh
initdb, and GetNewObjectId() would enforce that in a postmaster
environment to be FirstNormalObjectId when assigning the first user
OID. Would you imply an extra step at the end of initdb to update the
control data file of the new cluster to reflect FirstNormalObjectId?
--
Michael
On Wed, Jun 29, 2022 at 01:17:33PM +0900, Michael Paquier wrote:
On Sat, Jun 25, 2022 at 11:04:37AM -0500, Justin Pryzby wrote:
I'll concede that a cluster which has no tables sounds a lot like a toy, but I
find it disturbing that nothing prevents running the process twice, up to the
point that it's evidently corrupted the catalog.I have heard of cases where instances were only used with a set of
foreign tables, for example. Not sure that this is spread enough to
worry about, but this would fail as much as your case.
foreign tables have OIDs too.
ts=# SELECT * FROM pg_class WHERE relkind ='f';
oid | 1554544611
While looking at this, I noticed that starting postgres --single immediately
after initdb allows creating objects with OIDs below FirstNormalObjectId
(thereby defeating pg_upgrade's check). I'm not familiar with the behavioral
differences of single user mode, and couldn't find anything in the
documentation.This one comes from NextOID in the control data file after a fresh
initdb, and GetNewObjectId() would enforce that in a postmaster
environment to be FirstNormalObjectId when assigning the first user
OID. Would you imply an extra step at the end of initdb to update the
control data file of the new cluster to reflect FirstNormalObjectId?
I added a call to reset xlog, similar to what's in pg_upgrade.
Unfortunately, I don't see an easy way to silence it.
--
Justin
Attachments:
0001-wip-initdb-should-advance-the-OID-counter-to-FirstNo.patchtext/x-diff; charset=us-asciiDownload
From bb8133a01b61e2b3b1ee123bafc4a59b9f4a0c70 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 6 Jul 2022 08:55:11 -0500
Subject: [PATCH] wip: initdb should advance the OID counter to FirstNormal
Otherwise, a cluster started in single-user mode immediately after initdb can
create objects in the range of system OIDs.
---
src/backend/access/transam/varsup.c | 5 ++---
src/bin/initdb/initdb.c | 9 +++++++++
src/bin/pg_upgrade/controldata.c | 17 +----------------
src/bin/pg_upgrade/pg_upgrade.c | 11 -----------
src/bin/pg_upgrade/pg_upgrade.h | 1 -
5 files changed, 12 insertions(+), 31 deletions(-)
diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c
index 849a7ce9d6d..d93974fcaa5 100644
--- a/src/backend/access/transam/varsup.c
+++ b/src/backend/access/transam/varsup.c
@@ -543,8 +543,7 @@ GetNewObjectId(void)
*
* During initdb, we start the OID generator at FirstGenbkiObjectId, so we
* only wrap if before that point when in bootstrap or standalone mode.
- * The first time through this routine after normal postmaster start, the
- * counter will be forced up to FirstNormalObjectId. This mechanism
+ * This mechanism
* leaves the OIDs between FirstGenbkiObjectId and FirstNormalObjectId
* available for automatic assignment during initdb, while ensuring they
* will never conflict with user-assigned OIDs.
@@ -553,7 +552,7 @@ GetNewObjectId(void)
{
if (IsPostmasterEnvironment)
{
- /* wraparound, or first post-initdb assignment, in normal mode */
+ /* wraparound */
ShmemVariableCache->nextOid = FirstNormalObjectId;
ShmemVariableCache->oidCount = 0;
}
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 89b888eaa5a..b3f248b649a 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 */
@@ -2788,6 +2789,14 @@ initialize_data_directory(void)
PG_CMD_CLOSE;
+ /* Set FirstNormal OID */
+ snprintf(cmd, sizeof(cmd),
+ "\"%s/pg_resetwal\" -o %u %s",
+ bin_path, FirstNormalObjectId, pg_data);
+
+ PG_CMD_OPEN;
+ PG_CMD_CLOSE;
+
check_ok();
}
diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
index 41b8f69b8cb..26b569f7102 100644
--- a/src/bin/pg_upgrade/controldata.c
+++ b/src/bin/pg_upgrade/controldata.c
@@ -41,7 +41,6 @@ get_control_data(ClusterInfo *cluster, bool live_check)
bool got_log_id = false;
bool got_log_seg = false;
bool got_xid = false;
- bool got_oid = false;
bool got_multi = false;
bool got_oldestmulti = false;
bool got_oldestxid = false;
@@ -291,17 +290,6 @@ get_control_data(ClusterInfo *cluster, bool live_check)
cluster->controldata.chkpnt_nxtxid = str2uint(p);
got_xid = true;
}
- else if ((p = strstr(bufin, "Latest checkpoint's NextOID:")) != NULL)
- {
- p = strchr(p, ':');
-
- if (p == NULL || strlen(p) <= 1)
- pg_fatal("%d: controldata retrieval problem\n", __LINE__);
-
- p++; /* remove ':' char */
- cluster->controldata.chkpnt_nxtoid = str2uint(p);
- got_oid = true;
- }
else if ((p = strstr(bufin, "Latest checkpoint's NextMultiXactId:")) != NULL)
{
p = strchr(p, ':');
@@ -555,7 +543,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
}
/* verify that we got all the mandatory pg_control data */
- if (!got_xid || !got_oid ||
+ if (!got_xid ||
!got_multi || !got_oldestxid ||
(!got_oldestmulti &&
cluster->controldata.cat_ver >= MULTIXACT_FORMATCHANGE_CAT_VER) ||
@@ -577,9 +565,6 @@ get_control_data(ClusterInfo *cluster, bool live_check)
if (!got_xid)
pg_log(PG_REPORT, " checkpoint next XID\n");
- if (!got_oid)
- pg_log(PG_REPORT, " latest checkpoint next OID\n");
-
if (!got_multi)
pg_log(PG_REPORT, " latest checkpoint next MultiXactId\n");
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 265d8294906..785fe8dd18d 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -168,17 +168,6 @@ main(int argc, char **argv)
transfer_all_new_tablespaces(&old_cluster.dbarr, &new_cluster.dbarr,
old_cluster.pgdata, new_cluster.pgdata);
- /*
- * Assuming OIDs are only used in system tables, there is no need to
- * restore the OID counter because we have not transferred any OIDs from
- * the old system, but we do it anyway just in case. We do it late here
- * because there is no need to have the schema load use new oids.
- */
- prep_status("Setting next OID for new cluster");
- exec_prog(UTILITY_LOG_FILE, NULL, true, true,
- "\"%s/pg_resetwal\" -o %u \"%s\"",
- new_cluster.bindir, old_cluster.controldata.chkpnt_nxtoid,
- new_cluster.pgdata);
check_ok();
if (user_opts.do_sync)
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 50dfe9e81c1..7062da0ceb1 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -200,7 +200,6 @@ typedef struct
char nextxlogfile[25];
uint32 chkpnt_nxtxid;
uint32 chkpnt_nxtepoch;
- uint32 chkpnt_nxtoid;
uint32 chkpnt_nxtmulti;
uint32 chkpnt_nxtmxoff;
uint32 chkpnt_oldstMulti;
--
2.17.1
rebased and updated
Robert thought that it might be reasonable for someone to initdb, and
then connect and make some modifications, and then pg_upgrade.
/messages/by-id/CA+TgmoYwaXh_wRRa2CqL4XpM4r6YEbq1+ec=+8b7Dr7-T_tT+Q@mail.gmail.com
But the DBs are dropped by pg_upgrade, so that seems to serve no
purpose, except for shared relations (and global objects?). In the case
of shared relations, it seems unsafe (even though my test didn't cause
an immediate explosion).
So rather than continuing to allow arbitrary changes between initdb and
pg_upgrade, I propose to prohibit all changes. I'd consider allowing an
"inclusive" list of allowable changes, if someone were to propose such a
thing - but since DBs are dropped, I'm not sure what it could include.
Attachments:
v2-0001-wip-initdb-should-advance-the-OID-counter-to-Firs.patchtext/x-diff; charset=us-asciiDownload
From 7958ec4b13e9dc581f69df65933789274de499b4 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 6 Jul 2022 08:55:11 -0500
Subject: [PATCH v2] wip: initdb should advance the OID counter to FirstNormal
Otherwise, a cluster started in single-user mode immediately after
initdb can create objects in the range of system OIDs, and pg_upgrade
might be able to be run twice (if the cluster has no relations/objects).
---
src/backend/access/transam/varsup.c | 5 ++---
src/bin/initdb/initdb.c | 9 +++++++++
src/bin/pg_upgrade/check.c | 5 +++++
src/bin/pg_upgrade/pg_upgrade.c | 13 -------------
4 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c
index 849a7ce9d6d..d93974fcaa5 100644
--- a/src/backend/access/transam/varsup.c
+++ b/src/backend/access/transam/varsup.c
@@ -543,8 +543,7 @@ GetNewObjectId(void)
*
* During initdb, we start the OID generator at FirstGenbkiObjectId, so we
* only wrap if before that point when in bootstrap or standalone mode.
- * The first time through this routine after normal postmaster start, the
- * counter will be forced up to FirstNormalObjectId. This mechanism
+ * This mechanism
* leaves the OIDs between FirstGenbkiObjectId and FirstNormalObjectId
* available for automatic assignment during initdb, while ensuring they
* will never conflict with user-assigned OIDs.
@@ -553,7 +552,7 @@ GetNewObjectId(void)
{
if (IsPostmasterEnvironment)
{
- /* wraparound, or first post-initdb assignment, in normal mode */
+ /* wraparound */
ShmemVariableCache->nextOid = FirstNormalObjectId;
ShmemVariableCache->oidCount = 0;
}
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index e00837ecacf..1aad2b335b8 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -61,6 +61,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 */
@@ -2735,6 +2736,14 @@ initialize_data_directory(void)
PG_CMD_CLOSE;
+ /* Set FirstNormal OID */
+ snprintf(cmd, sizeof(cmd),
+ "\"%s/pg_resetwal\" -o %u %s",
+ bin_path, FirstNormalObjectId, pg_data);
+
+ PG_CMD_OPEN;
+ PG_CMD_CLOSE;
+
check_ok();
}
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index f4969bcdad7..f7ff9b5f65b 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -9,6 +9,7 @@
#include "postgres_fe.h"
+#include "access/transam.h"
#include "catalog/pg_authid_d.h"
#include "catalog/pg_collation.h"
#include "fe_utils/string_utils.h"
@@ -434,6 +435,10 @@ check_new_cluster_is_empty(void)
{
int dbnum;
+ if (new_cluster.controldata.chkpnt_nxtoid != FirstNormalObjectId)
+ pg_fatal("New cluster is not pristine: OIDs have been assigned since initdb (%u != %u)\n",
+ new_cluster.controldata.chkpnt_nxtoid, FirstNormalObjectId);
+
for (dbnum = 0; dbnum < new_cluster.dbarr.ndbs; dbnum++)
{
int relnum;
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 115faa222e3..ea7d93d9cbe 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -172,19 +172,6 @@ main(int argc, char **argv)
transfer_all_new_tablespaces(&old_cluster.dbarr, &new_cluster.dbarr,
old_cluster.pgdata, new_cluster.pgdata);
- /*
- * Assuming OIDs are only used in system tables, there is no need to
- * restore the OID counter because we have not transferred any OIDs from
- * the old system, but we do it anyway just in case. We do it late here
- * because there is no need to have the schema load use new oids.
- */
- prep_status("Setting next OID for new cluster");
- exec_prog(UTILITY_LOG_FILE, NULL, true, true,
- "\"%s/pg_resetwal\" -o %u \"%s\"",
- new_cluster.bindir, old_cluster.controldata.chkpnt_nxtoid,
- new_cluster.pgdata);
- check_ok();
-
if (user_opts.do_sync)
{
prep_status("Sync data directory to disk");
--
2.17.1
On 07.07.22 08:22, Justin Pryzby wrote:
This one comes from NextOID in the control data file after a fresh
initdb, and GetNewObjectId() would enforce that in a postmaster
environment to be FirstNormalObjectId when assigning the first user
OID. Would you imply an extra step at the end of initdb to update the
control data file of the new cluster to reflect FirstNormalObjectId?I added a call to reset xlog, similar to what's in pg_upgrade.
Unfortunately, I don't see an easy way to silence it.
I think it would be better to update the control file directly instead
of going through pg_resetwal. (See
src/include/common/controldata_utils.h for the required functions.)
However, I don't know whether we need to add special provisions that
guard against people using postgres --single in complicated ways. Many
consider the single-user mode deprecated outside of initdb use.
On Tue, Nov 01, 2022 at 01:54:35PM +0100, Peter Eisentraut wrote:
On 07.07.22 08:22, Justin Pryzby wrote:
This one comes from NextOID in the control data file after a fresh
initdb, and GetNewObjectId() would enforce that in a postmaster
environment to be FirstNormalObjectId when assigning the first user
OID. Would you imply an extra step at the end of initdb to update the
control data file of the new cluster to reflect FirstNormalObjectId?I added a call to reset xlog, similar to what's in pg_upgrade.
Unfortunately, I don't see an easy way to silence it.I think it would be better to update the control file directly instead of
going through pg_resetwal. (See src/include/common/controldata_utils.h for
the required functions.)However, I don't know whether we need to add special provisions that guard
against people using postgres --single in complicated ways. Many consider
the single-user mode deprecated outside of initdb use.
Thanks for looking.
One other thing I noticed (by accident!) is that pg_upgrade doesn't
prevent itself from trying to upgrade a cluster on top of itself:
| $ /usr/pgsql-15/bin/initdb -D pg15.dat -N
| $ /usr/pgsql-15/bin/pg_upgrade -D pg15.dat -d pg15.dat -b /usr/pgsql-15/bin
| Performing Upgrade
| ------------------
| Analyzing all rows in the new cluster ok
| Freezing all rows in the new cluster ok
| Deleting files from new pg_xact ok
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| Copying old pg_xact to new server
| *failure*
|
| Consult the last few lines of "pg15.dat/pg_upgrade_output.d/20221101T055916.486/log/pg_upgrade_utility.log" for
| command: cp -Rf "pg15.dat/pg_xact" "pg15.dat/pg_xact" >> "pg15.dat/pg_upgrade_output.d/20221101T055916.486/log/pg_upgrade_utility.log" 2>&1
| cp: cannot stat 'pg15.dat/pg_xact': No such file or directory
This may be of little concern since it's upgrading a version to itself, which
only applies to developers.
--
Justin
On 01.11.22 14:07, Justin Pryzby wrote:
On Tue, Nov 01, 2022 at 01:54:35PM +0100, Peter Eisentraut wrote:
On 07.07.22 08:22, Justin Pryzby wrote:
This one comes from NextOID in the control data file after a fresh
initdb, and GetNewObjectId() would enforce that in a postmaster
environment to be FirstNormalObjectId when assigning the first user
OID. Would you imply an extra step at the end of initdb to update the
control data file of the new cluster to reflect FirstNormalObjectId?I added a call to reset xlog, similar to what's in pg_upgrade.
Unfortunately, I don't see an easy way to silence it.I think it would be better to update the control file directly instead of
going through pg_resetwal. (See src/include/common/controldata_utils.h for
the required functions.)However, I don't know whether we need to add special provisions that guard
against people using postgres --single in complicated ways. Many consider
the single-user mode deprecated outside of initdb use.Thanks for looking.
I think the above is a "returned with feedback" at this point.
One other thing I noticed (by accident!) is that pg_upgrade doesn't
prevent itself from trying to upgrade a cluster on top of itself:| $ /usr/pgsql-15/bin/initdb -D pg15.dat -N
| $ /usr/pgsql-15/bin/pg_upgrade -D pg15.dat -d pg15.dat -b /usr/pgsql-15/bin
| Performing Upgrade
| ------------------
| Analyzing all rows in the new cluster ok
| Freezing all rows in the new cluster ok
| Deleting files from new pg_xact ok
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| Copying old pg_xact to new server
| *failure*
|
| Consult the last few lines of "pg15.dat/pg_upgrade_output.d/20221101T055916.486/log/pg_upgrade_utility.log" for| command: cp -Rf "pg15.dat/pg_xact" "pg15.dat/pg_xact" >> "pg15.dat/pg_upgrade_output.d/20221101T055916.486/log/pg_upgrade_utility.log" 2>&1
| cp: cannot stat 'pg15.dat/pg_xact': No such file or directoryThis may be of little concern since it's upgrading a version to itself, which
only applies to developers.
I think this would be worth addressing nonetheless, for robustness. For
comparison, "cp" and "mv" will error if you give source and destination
that are the same file.
On Thu, Dec 01, 2022 at 10:30:16AM +0100, Peter Eisentraut wrote:
On 01.11.22 14:07, Justin Pryzby wrote:
On Tue, Nov 01, 2022 at 01:54:35PM +0100, Peter Eisentraut wrote:
On 07.07.22 08:22, Justin Pryzby wrote:
This one comes from NextOID in the control data file after a fresh
initdb, and GetNewObjectId() would enforce that in a postmaster
environment to be FirstNormalObjectId when assigning the first user
OID. Would you imply an extra step at the end of initdb to update the
control data file of the new cluster to reflect FirstNormalObjectId?I added a call to reset xlog, similar to what's in pg_upgrade.
Unfortunately, I don't see an easy way to silence it.I think it would be better to update the control file directly instead of
going through pg_resetwal. (See src/include/common/controldata_utils.h for
the required functions.)However, I don't know whether we need to add special provisions that guard
against people using postgres --single in complicated ways. Many consider
the single-user mode deprecated outside of initdb use.Thanks for looking.
To be clear, I don't think it's worth doing anything special just to
avoid creating special OIDs if someone runs postgres --single
immediately after initdb.
However, setting FirstNormalOid in initdb itself (rather than in the
next invocation of postgres, if it isn't in single-user-mode) was the
mechanism by which to avoid the original problem that pg_upgrade can be
run twice, if there are no non-system relations. That still seems
desirable to fix somehow.
I think the above is a "returned with feedback" at this point.
One other thing I noticed (by accident!) is that pg_upgrade doesn't
prevent itself from trying to upgrade a cluster on top of itself:
| command: cp -Rf "pg15.dat/pg_xact" "pg15.dat/pg_xact" >> "pg15.dat/pg_upgrade_output.d/20221101T055916.486/log/pg_upgrade_utility.log" 2>&1
| cp: cannot stat 'pg15.dat/pg_xact': No such file or directoryThis may be of little concern since it's upgrading a version to itself, which
only applies to developers.I think this would be worth addressing nonetheless, for robustness. For
comparison, "cp" and "mv" will error if you give source and destination that
are the same file.
I addressed this in 002.
--
Justin
Attachments:
0001-wip-initdb-should-advance-the-OID-counter-to-FirstNo.patchtext/x-diff; charset=us-asciiDownload
From ecc7a9a4698eb138e63adcf23302c8e7d43ab96e Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 6 Jul 2022 08:55:11 -0500
Subject: [PATCH 1/2] wip: initdb should advance the OID counter to FirstNormal
Otherwise, a cluster started in single-user mode immediately after
initdb can create objects in the range of system OIDs, and pg_upgrade
might be able to be run twice (if the cluster has no relations/objects).
---
src/backend/access/transam/varsup.c | 5 ++---
src/bin/initdb/initdb.c | 10 ++++++++++
src/bin/pg_upgrade/check.c | 5 +++++
src/bin/pg_upgrade/pg_upgrade.c | 13 -------------
4 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c
index 849a7ce9d6d..d93974fcaa5 100644
--- a/src/backend/access/transam/varsup.c
+++ b/src/backend/access/transam/varsup.c
@@ -543,8 +543,7 @@ GetNewObjectId(void)
*
* During initdb, we start the OID generator at FirstGenbkiObjectId, so we
* only wrap if before that point when in bootstrap or standalone mode.
- * The first time through this routine after normal postmaster start, the
- * counter will be forced up to FirstNormalObjectId. This mechanism
+ * This mechanism
* leaves the OIDs between FirstGenbkiObjectId and FirstNormalObjectId
* available for automatic assignment during initdb, while ensuring they
* will never conflict with user-assigned OIDs.
@@ -553,7 +552,7 @@ GetNewObjectId(void)
{
if (IsPostmasterEnvironment)
{
- /* wraparound, or first post-initdb assignment, in normal mode */
+ /* wraparound */
ShmemVariableCache->nextOid = FirstNormalObjectId;
ShmemVariableCache->oidCount = 0;
}
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 7c391aaf0b1..1ac602da7f3 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -61,11 +61,13 @@
#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 */
#include "catalog/pg_collation_d.h"
#include "catalog/pg_database_d.h" /* pgrminclude ignore */
+#include "common/controldata_utils.h"
#include "common/file_perm.h"
#include "common/file_utils.h"
#include "common/logging.h"
@@ -2725,6 +2727,14 @@ initialize_data_directory(void)
PG_CMD_CLOSE;
+ /* Set FirstNormal OID */
+ {
+ bool crc_ok;
+ ControlFileData *cfd = get_controlfile(pg_data, &crc_ok);
+ cfd->checkPointCopy.nextOid = FirstNormalObjectId;
+ update_controlfile(pg_data, cfd, false);
+ }
+
check_ok();
}
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index d4a3691438e..a6c8a79a133 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -9,6 +9,7 @@
#include "postgres_fe.h"
+#include "access/transam.h"
#include "catalog/pg_authid_d.h"
#include "catalog/pg_collation.h"
#include "fe_utils/string_utils.h"
@@ -442,6 +443,10 @@ check_new_cluster_is_empty(void)
{
int dbnum;
+ if (new_cluster.controldata.chkpnt_nxtoid != FirstNormalObjectId)
+ pg_fatal("New cluster is not pristine: OIDs have been assigned since initdb (%u != %u)\n",
+ new_cluster.controldata.chkpnt_nxtoid, FirstNormalObjectId);
+
for (dbnum = 0; dbnum < new_cluster.dbarr.ndbs; dbnum++)
{
int relnum;
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 115faa222e3..ea7d93d9cbe 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -172,19 +172,6 @@ main(int argc, char **argv)
transfer_all_new_tablespaces(&old_cluster.dbarr, &new_cluster.dbarr,
old_cluster.pgdata, new_cluster.pgdata);
- /*
- * Assuming OIDs are only used in system tables, there is no need to
- * restore the OID counter because we have not transferred any OIDs from
- * the old system, but we do it anyway just in case. We do it late here
- * because there is no need to have the schema load use new oids.
- */
- prep_status("Setting next OID for new cluster");
- exec_prog(UTILITY_LOG_FILE, NULL, true, true,
- "\"%s/pg_resetwal\" -o %u \"%s\"",
- new_cluster.bindir, old_cluster.controldata.chkpnt_nxtoid,
- new_cluster.pgdata);
- check_ok();
-
if (user_opts.do_sync)
{
prep_status("Sync data directory to disk");
--
2.25.1
0002-pg_upgrade-prohibit-attempts-to-upgrade-a-cluster-on.patchtext/x-diff; charset=us-asciiDownload
From daafe36c54d14587df6bac41d48d92f0a6f8e1e7 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Tue, 1 Nov 2022 18:32:51 -0500
Subject: [PATCH 2/2] pg_upgrade: prohibit attempts to upgrade a cluster on top
of itself
./tmp_install/usr/local/pgsql/bin/initdb -N -D pg16.dat
./tmp_install/usr/local/pgsql/bin/pg_upgrade -D pg16.dat -d `pwd`/pg16.dat -b ./tmp_install/usr/local/pgsql/bin
---
src/bin/pg_upgrade/option.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 2939f584b4c..598d9c5fc10 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -233,6 +233,10 @@ parseCommandLine(int argc, char *argv[])
check_required_directory(&user_opts.socketdir, "PGSOCKETDIR", true,
"-s", _("sockets will be created"), false);
+ if (strcmp(make_absolute_path(old_cluster.pgdata),
+ make_absolute_path(new_cluster.pgdata)) == 0)
+ pg_fatal("cannot upgrade a cluster on top of itself");
+
#ifdef WIN32
/*
--
2.25.1
On Fri, Dec 16, 2022 at 07:38:09AM -0600, Justin Pryzby wrote:
However, setting FirstNormalOid in initdb itself (rather than in the
next invocation of postgres, if it isn't in single-user-mode) was the
mechanism by which to avoid the original problem that pg_upgrade can be
run twice, if there are no non-system relations. That still seems
desirable to fix somehow.
+ if (new_cluster.controldata.chkpnt_nxtoid != FirstNormalObjectId)
+ pg_fatal("New cluster is not pristine: OIDs have been assigned since initdb (%u != %u)\n",
+ new_cluster.controldata.chkpnt_nxtoid, FirstNormalObjectId);On wraparound FirstNormalObjectId would be the first value we use for
nextOid. Okay, that's very unlikely going to happen, still, strictly
speaking, that could be incorrect.
I think this would be worth addressing nonetheless, for robustness. For
comparison, "cp" and "mv" will error if you give source and destination that
are the same file.I addressed this in 002.
+ if (strcmp(make_absolute_path(old_cluster.pgdata),
+ make_absolute_path(new_cluster.pgdata)) == 0)
+ pg_fatal("cannot upgrade a cluster on top of itself");Shouldn't this be done after adjust_data_dir(), which is the point
where we'll know the actual data folders we are working on by querying
postgres -C data_directory?
--
Michael