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 directory
> >
> > This 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
>From ecc7a9a4698eb138e63adcf23302c8e7d43ab96e Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
>From daafe36c54d14587df6bac41d48d92f0a6f8e1e7 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
