pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedpg_upgrade: fail early if a tablespace dir already exists for new cluster version
13+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2020-10-16 9dcffe69a9 16be4e435b b05672c725 6e34cc8ab5 0ab7ca98a1 536de14e2b 79fe23465d pg_upgrade: remove C99 compiler req. from commit 3c0471b5fd
2020-10-15 85fedf39f0 d9ae2297bf 39c23c199d eb421c83b0 a106236d82 59cfff65b1 3c0471b5fd pg_upgrade: generate check error for left-over new tablespace
* pg_upgrade: fail early if a tablespace dir already exists for new cluster version
@ 2020-09-25 00:55 Justin Pryzby <pryzby@telsasoft.com>
2020-09-25 20:47 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 18:53 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
0 siblings, 2 replies; 13+ messages in thread
From: Justin Pryzby @ 2020-09-25 00:55 UTC (permalink / raw)
To: pgsql-hackers
This should be caught during --check, or earlier in the upgrade, rather than
only after dumping the schema.
Typically, the tablespace dir would be left behind by a previous failed upgrade
attempt, causing a cascade of failured upgrades. I guess I may not be the only
one with a 3 year old wrapper which has a hack to check for this.
|rm -fr pgsql12.dat pgsql13.dat
|/usr/pgsql-12/bin/initdb -D pgsql12.dat --no-sync
|/usr/pgsql-13/bin/initdb -D pgsql13.dat --no-sync
|/usr/pgsql-12/bin/postgres -D pgsql12.dat -c port=5678 -k /tmp
|mkdir tblspc tblspc/PG_13_202007203
|psql -h /tmp -p 5678 postgres -c "CREATE TABLESPACE one LOCATION '/home/pryzbyj/tblspc'"
|/usr/pgsql-13/bin/pg_upgrade -D pgsql13.dat -d pgsql12.dat -b /usr/pgsql-12/bin
|pg_upgrade_utility.log:
|CREATE TABLESPACE "one" OWNER "pryzbyj" LOCATION '/home/pryzbyj/tblspc';
|psql:pg_upgrade_dump_globals.sql:27: ERROR: directory "/home/pryzbyj/tblspc/PG_13_202007201" already in use as a tablespace
Attachments:
v1-0001-pg_upgrade-check-to-avoid-tablespace-failure-mode.patch (text/x-diff, 2.4K) download
From 3df104610d73833da60bffcc54756a6ecb2f390f Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 24 Sep 2020 19:49:40 -0500
Subject: [PATCH v1] pg_upgrade --check to avoid tablespace failure mode
---
src/bin/pg_upgrade/check.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 2f7aa632c5..b07e63dab2 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -13,6 +13,7 @@
#include "fe_utils/string_utils.h"
#include "mb/pg_wchar.h"
#include "pg_upgrade.h"
+#include "common/relpath.h"
static void check_new_cluster_is_empty(void);
static void check_databases_are_compatible(void);
@@ -27,6 +28,7 @@ static void check_for_tables_with_oids(ClusterInfo *cluster);
static void check_for_reg_data_type_usage(ClusterInfo *cluster);
static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
static void check_for_pg_role_prefix(ClusterInfo *cluster);
+static void check_for_existing_tablespace_dirs(ClusterInfo *new_cluster);
static char *get_canonical_locale_name(int category, const char *locale);
@@ -187,6 +189,8 @@ check_new_cluster(void)
check_is_install_user(&new_cluster);
check_for_prepared_transactions(&new_cluster);
+
+ check_for_existing_tablespace_dirs(&new_cluster);
}
@@ -541,6 +545,34 @@ create_script_for_cluster_analyze(char **analyze_script_file_name)
check_ok();
}
+/*
+ * Check that tablespace dirs for new cluster do not already exist.
+ * If they did, they'd cause an error while restoring global objects.
+ * This allows failing earlier rather than only after dumping schema.
+ */
+static void
+check_for_existing_tablespace_dirs(ClusterInfo *new_cluster)
+{
+ char old_tablespace_dir[MAXPGPATH];
+
+ prep_status("Checking for pre-existing tablespace directories for new cluster version");
+
+// if (GET_MAJOR_VERSION(old_cluster.major_version) <= 804)
+ for (int tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
+ {
+ struct stat statbuf;
+ snprintf(old_tablespace_dir, MAXPGPATH, "%s%c%s",
+ os_info.old_tablespaces[tblnum],
+ PATH_SEPARATOR, TABLESPACE_VERSION_DIRECTORY);
+
+ canonicalize_path(old_tablespace_dir);
+
+ // XXX: lstat ?
+ if (stat(old_tablespace_dir, &statbuf) == 0)
+ pg_fatal("tablespace directory already exists for new cluster version: \"%s\"\n",
+ old_tablespace_dir);
+ }
+}
/*
* create_script_for_old_cluster_deletion()
--
2.17.0
* Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
@ 2020-09-25 20:47 ` Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 0 replies; 13+ messages in thread
From: Justin Pryzby @ 2020-09-25 20:47 UTC (permalink / raw)
To: pgsql-hackers
Added at
https://commitfest.postgresql.org/30/2739/
Attachments:
v2-0001-pg_upgrade-check-to-avoid-tablespace-failure-mode.patch (text/x-diff, 2.5K) download
From 831246aa6d6837b2b0da7c96ad2f44ffd6cd3951 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 24 Sep 2020 19:49:40 -0500
Subject: [PATCH v2] pg_upgrade --check to avoid tablespace failure mode
---
src/bin/pg_upgrade/check.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 2f7aa632c5..154e3d249e 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -13,6 +13,7 @@
#include "fe_utils/string_utils.h"
#include "mb/pg_wchar.h"
#include "pg_upgrade.h"
+#include "common/relpath.h"
static void check_new_cluster_is_empty(void);
static void check_databases_are_compatible(void);
@@ -27,6 +28,7 @@ static void check_for_tables_with_oids(ClusterInfo *cluster);
static void check_for_reg_data_type_usage(ClusterInfo *cluster);
static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
static void check_for_pg_role_prefix(ClusterInfo *cluster);
+static void check_for_existing_tablespace_dirs(ClusterInfo *new_cluster);
static char *get_canonical_locale_name(int category, const char *locale);
@@ -187,6 +189,8 @@ check_new_cluster(void)
check_is_install_user(&new_cluster);
check_for_prepared_transactions(&new_cluster);
+
+ check_for_existing_tablespace_dirs(&new_cluster);
}
@@ -542,6 +546,39 @@ create_script_for_cluster_analyze(char **analyze_script_file_name)
}
+/*
+ * Check that tablespace dirs do not already exist for new cluster version.
+ * If they did, it'd cause an error while restoring global objects.
+ * This allows failing earlier rather than only after dumping schema.
+ *
+ * Note, v8.4 has no tablespace_suffix, which is fine so long as the version we
+ * being upgraded *to* has a suffix.
+ */
+static void
+check_for_existing_tablespace_dirs(ClusterInfo *new_cluster)
+{
+ char old_tablespace_dir[MAXPGPATH];
+
+ prep_status("Checking for pre-existing tablespace directories");
+
+ for (int tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
+ {
+ struct stat statbuf;
+ snprintf(old_tablespace_dir, MAXPGPATH, "%s%s",
+ os_info.old_tablespaces[tblnum],
+ new_cluster->tablespace_suffix);
+
+ canonicalize_path(old_tablespace_dir);
+
+ // XXX: lstat ?
+ if (stat(old_tablespace_dir, &statbuf) == 0 || errno != ENOENT)
+ pg_fatal("tablespace directory already exists for new cluster version: \"%s\"\n",
+ old_tablespace_dir);
+ }
+
+ check_ok();
+}
+
/*
* create_script_for_old_cluster_deletion()
*
--
2.17.0
* Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
@ 2020-10-09 18:53 ` Bruce Momjian <bruce@momjian.us>
2020-10-09 19:23 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 1 reply; 13+ messages in thread
From: Bruce Momjian @ 2020-10-09 18:53 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers
On Thu, Sep 24, 2020 at 07:55:31PM -0500, Justin Pryzby wrote:
> This should be caught during --check, or earlier in the upgrade, rather than
> only after dumping the schema.
>
> Typically, the tablespace dir would be left behind by a previous failed upgrade
> attempt, causing a cascade of failured upgrades. I guess I may not be the only
> one with a 3 year old wrapper which has a hack to check for this.
This is an interesting failure case I never considered --- running
pg_upgrade, having it fail, deleting and recreating the _new_ cluster
directory, but not removing the new cluster's tablepace directories. I
was able to recreate the failure, and verify that your patch properly
throws an error during 'check' for this case.
Modified patch attached. I plan to apply this to all supported Postgres
versions.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Attachments:
tablespace.diff (text/x-diff, 2.0K) download
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
new file mode 100644
index 168058a..0c845b7
*** a/src/bin/pg_upgrade/check.c
--- b/src/bin/pg_upgrade/check.c
*************** static void check_for_tables_with_oids(C
*** 27,32 ****
--- 27,33 ----
static void check_for_reg_data_type_usage(ClusterInfo *cluster);
static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
static void check_for_pg_role_prefix(ClusterInfo *cluster);
+ static void check_for_new_tablespace_dir(ClusterInfo *new_cluster);
static char *get_canonical_locale_name(int category, const char *locale);
*************** check_new_cluster(void)
*** 187,192 ****
--- 188,195 ----
check_is_install_user(&new_cluster);
check_for_prepared_transactions(&new_cluster);
+
+ check_for_new_tablespace_dir(&new_cluster);
}
*************** create_script_for_cluster_analyze(char *
*** 528,533 ****
--- 531,568 ----
/*
+ * Check that tablespace directories do not already exist for new cluster.
+ * If they do, it would cause an error while restoring global objects.
+ * This allows the failure to be detected at check time, rather than
+ * during schema restore.
+ *
+ * Note, v8.4 has no tablespace_suffix, which is fine as long as the new
+ * cluster version has a suffix.
+ */
+ static void
+ check_for_new_tablespace_dir(ClusterInfo *new_cluster)
+ {
+ char new_tablespace_dir[MAXPGPATH];
+
+ prep_status("Checking for new cluster tablespace directories");
+
+ for (int tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
+ {
+ struct stat statbuf;
+
+ snprintf(new_tablespace_dir, MAXPGPATH, "%s%s",
+ os_info.old_tablespaces[tblnum],
+ new_cluster->tablespace_suffix);
+
+ if (stat(new_tablespace_dir, &statbuf) == 0 || errno != ENOENT)
+ pg_fatal("new cluster tablespace directory already exists: \"%s\"\n",
+ new_tablespace_dir);
+ }
+
+ check_ok();
+ }
+
+ /*
* create_script_for_old_cluster_deletion()
*
* This is particularly useful for tablespace deletion.
* Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 18:53 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
@ 2020-10-09 19:23 ` Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 23:42 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 13+ messages in thread
From: Justin Pryzby @ 2020-10-09 19:23 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: pgsql-hackers
On Fri, Oct 09, 2020 at 02:53:25PM -0400, Bruce Momjian wrote:
> On Thu, Sep 24, 2020 at 07:55:31PM -0500, Justin Pryzby wrote:
> > This should be caught during --check, or earlier in the upgrade, rather than
> > only after dumping the schema.
> >
> > Typically, the tablespace dir would be left behind by a previous failed upgrade
> > attempt, causing a cascade of failured upgrades. I guess I may not be the only
> > one with a 3 year old wrapper which has a hack to check for this.
>
> This is an interesting failure case I never considered --- running
> pg_upgrade, having it fail, deleting and recreating the _new_ cluster
> directory, but not removing the new cluster's tablepace directories. I
> was able to recreate the failure, and verify that your patch properly
> throws an error during 'check' for this case.
>
> Modified patch attached. I plan to apply this to all supported Postgres
> versions.
Thanks for looking. It hits me a bunch of times every year.
> + * Check that tablespace directories do not already exist for new cluster.
> + * If they do, it would cause an error while restoring global objects.
> + * This allows the failure to be detected at check time, rather than
> + * during schema restore.
> + *
> + * Note, v8.4 has no tablespace_suffix, which is fine as long as the new
> + * cluster version has a suffix.
In my local branch, I had revised this comment to say:
+ * Note, v8.4 has no tablespace_suffix, which is fine so long as the version we
+ * being upgraded *to* has a suffix, since it's not allowed to pg_upgrade from
+ * a version to the same version if tablespaces are in use.
Cheers,
--
Justin
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 18:53 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-09 19:23 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
@ 2020-10-09 23:42 ` Bruce Momjian <bruce@momjian.us>
2020-10-12 17:33 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-15 23:35 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
0 siblings, 2 replies; 13+ messages in thread
From: Bruce Momjian @ 2020-10-09 23:42 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers
On Fri, Oct 9, 2020 at 02:23:10PM -0500, Justin Pryzby wrote:
> In my local branch, I had revised this comment to say:
>
> + * Note, v8.4 has no tablespace_suffix, which is fine so long as the version we
> + * being upgraded *to* has a suffix, since it's not allowed to pg_upgrade from
> + * a version to the same version if tablespaces are in use.
OK, updated patch attached. Also, from your original patch, I didn't
need to call canonicalize_path() since we are not comparing paths, and I
didn't need to include common/relpath.h. I also renamed a variable.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Attachments:
tablespace.diff (text/x-diff, 2.2K) download
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 168058a873..df4274d89a 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -27,6 +27,7 @@ static void check_for_tables_with_oids(ClusterInfo *cluster);
static void check_for_reg_data_type_usage(ClusterInfo *cluster);
static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
static void check_for_pg_role_prefix(ClusterInfo *cluster);
+static void check_for_new_tablespace_dir(ClusterInfo *new_cluster);
static char *get_canonical_locale_name(int category, const char *locale);
@@ -187,6 +188,8 @@ check_new_cluster(void)
check_is_install_user(&new_cluster);
check_for_prepared_transactions(&new_cluster);
+
+ check_for_new_tablespace_dir(&new_cluster);
}
@@ -527,6 +530,43 @@ create_script_for_cluster_analyze(char **analyze_script_file_name)
}
+/*
+ * A previous run of pg_upgrade might have failed and the new cluster
+ * directory recreated, but they might have forgotten to remove
+ * the new cluser's tablespace directories. Therefore, check that
+ * new cluster tablespace directories do not already exist. If
+ * they do, it would cause an error while restoring global objects.
+ * This allows the failure to be detected at check time, rather than
+ * during schema restore.
+ *
+ * Note, v8.4 has no tablespace_suffix, which is fine so long as the
+ * version we being upgraded *to* has a suffix, since it's not allowed
+ * to pg_upgrade from a version to the same version if tablespaces are
+ * in use.
+ */
+static void
+check_for_new_tablespace_dir(ClusterInfo *new_cluster)
+{
+ char new_tablespace_dir[MAXPGPATH];
+
+ prep_status("Checking for new cluster tablespace directories");
+
+ for (int tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
+ {
+ struct stat statbuf;
+
+ snprintf(new_tablespace_dir, MAXPGPATH, "%s%s",
+ os_info.old_tablespaces[tblnum],
+ new_cluster->tablespace_suffix);
+
+ if (stat(new_tablespace_dir, &statbuf) == 0 || errno != ENOENT)
+ pg_fatal("new cluster tablespace directory already exists: \"%s\"\n",
+ new_tablespace_dir);
+ }
+
+ check_ok();
+}
+
/*
* create_script_for_old_cluster_deletion()
*
* Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 18:53 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-09 19:23 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 23:42 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
@ 2020-10-12 17:33 ` Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 0 replies; 13+ messages in thread
From: Justin Pryzby @ 2020-10-12 17:33 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: pgsql-hackers
On Fri, Oct 09, 2020 at 07:42:51PM -0400, Bruce Momjian wrote:
> On Fri, Oct 9, 2020 at 02:23:10PM -0500, Justin Pryzby wrote:
> > In my local branch, I had revised this comment to say:
> >
> > + * Note, v8.4 has no tablespace_suffix, which is fine so long as the version we
> > + * being upgraded *to* has a suffix, since it's not allowed to pg_upgrade from
> > + * a version to the same version if tablespaces are in use.
>
> OK, updated patch attached. Also, from your original patch, I didn't
> need to call canonicalize_path() since we are not comparing paths, and I
> didn't need to include common/relpath.h. I also renamed a variable.
Since I just hit it again, I'll take the opportunity to give an example of how
this can happen.
Here, I've pg_upgraded from a pg12 to pg13, but the pg12 cluster has
postgis-3.0, and pg13 has postgis-3.1. Maybe that's not guaranteed/intended to
work, but that's what's been working well so far this year, except that there's
two gis functions which no longer exist. So we fail while "Restoring database
schemas in the new cluster", leaving behind tablespace dirs, which then cause
future pg_upgrades to fail.
pg_restore: from TOC entry 13543; 1255 17106 FUNCTION pgis_geometry_union_transfn("internal", "public"."geometry") postgres
pg_restore: error: could not execute query: ERROR: could not find function "pgis_geometry_union_transfn" in file "/usr/pgsql-13/lib/postgis-3.so"
Command was: CREATE FUNCTION "public"."pgis_geometry_union_transfn"("internal", "public"."geometry") RETURNS "internal"
LANGUAGE "c" PARALLEL SAFE
AS '$libdir/postgis-3', 'pgis_geometry_union_transfn';
I imagine in previous years I hit that some other way, like maybe I installed
postgres RC1 on a customer server, imported their schema (and maybe data),
which then caused upgrade failure 1-2 months later when trying to upgrade their
production instance.
--
Justin
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 18:53 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-09 19:23 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 23:42 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
@ 2020-10-15 23:35 ` Bruce Momjian <bruce@momjian.us>
2020-10-16 00:19 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Andres Freund <andres@anarazel.de>
2020-10-20 23:06 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 2 replies; 13+ messages in thread
From: Bruce Momjian @ 2020-10-15 23:35 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers@postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
On Fri, Oct 9, 2020 at 07:42:51PM -0400, Bruce Momjian wrote:
> On Fri, Oct 9, 2020 at 02:23:10PM -0500, Justin Pryzby wrote:
> > In my local branch, I had revised this comment to say:
> >
> > + * Note, v8.4 has no tablespace_suffix, which is fine so long as the version we
> > + * being upgraded *to* has a suffix, since it's not allowed to pg_upgrade from
> > + * a version to the same version if tablespaces are in use.
>
> OK, updated patch attached. Also, from your original patch, I didn't
> need to call canonicalize_path() since we are not comparing paths, and I
> didn't need to include common/relpath.h. I also renamed a variable.
Patch applied to all supported versions. Thanks for the report, and the
patch. While I listed you as reporter, I forgot to list you as the
patch author --- Tom, can you remember that for the minor release notes
please? Thanks
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 18:53 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-09 19:23 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 23:42 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-15 23:35 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
@ 2020-10-16 00:19 ` Andres Freund <andres@anarazel.de>
2020-10-16 00:37 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
1 sibling, 1 reply; 13+ messages in thread
From: Andres Freund @ 2020-10-16 00:19 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
Hi,
On 2020-10-15 19:35:30 -0400, Bruce Momjian wrote:
> On Fri, Oct 9, 2020 at 07:42:51PM -0400, Bruce Momjian wrote:
> > On Fri, Oct 9, 2020 at 02:23:10PM -0500, Justin Pryzby wrote:
> > > In my local branch, I had revised this comment to say:
> > >
> > > + * Note, v8.4 has no tablespace_suffix, which is fine so long as the version we
> > > + * being upgraded *to* has a suffix, since it's not allowed to pg_upgrade from
> > > + * a version to the same version if tablespaces are in use.
> >
> > OK, updated patch attached. Also, from your original patch, I didn't
> > need to call canonicalize_path() since we are not comparing paths, and I
> > didn't need to include common/relpath.h. I also renamed a variable.
>
> Patch applied to all supported versions. Thanks for the report, and the
> patch. While I listed you as reporter, I forgot to list you as the
> patch author --- Tom, can you remember that for the minor release notes
> please? Thanks
This fails on some animals <= 11, because we didn't yet rely on C99.
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2020-10-15%2023%3A52%3A04
check.c:503:2: error: \342\200\230for\342\200\231 loop initial declarations are only allowed in C99 mode
for (int tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 18:53 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-09 19:23 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 23:42 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-15 23:35 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-16 00:19 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Andres Freund <andres@anarazel.de>
@ 2020-10-16 00:37 ` Bruce Momjian <bruce@momjian.us>
0 siblings, 0 replies; 13+ messages in thread
From: Bruce Momjian @ 2020-10-16 00:37 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
On Thu, Oct 15, 2020 at 05:19:59PM -0700, Andres Freund wrote:
> Hi,
>
> On 2020-10-15 19:35:30 -0400, Bruce Momjian wrote:
> > On Fri, Oct 9, 2020 at 07:42:51PM -0400, Bruce Momjian wrote:
> > > On Fri, Oct 9, 2020 at 02:23:10PM -0500, Justin Pryzby wrote:
> > > > In my local branch, I had revised this comment to say:
> > > >
> > > > + * Note, v8.4 has no tablespace_suffix, which is fine so long as the version we
> > > > + * being upgraded *to* has a suffix, since it's not allowed to pg_upgrade from
> > > > + * a version to the same version if tablespaces are in use.
> > >
> > > OK, updated patch attached. Also, from your original patch, I didn't
> > > need to call canonicalize_path() since we are not comparing paths, and I
> > > didn't need to include common/relpath.h. I also renamed a variable.
> >
> > Patch applied to all supported versions. Thanks for the report, and the
> > patch. While I listed you as reporter, I forgot to list you as the
> > patch author --- Tom, can you remember that for the minor release notes
> > please? Thanks
>
> This fails on some animals <= 11, because we didn't yet rely on C99.
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2020-10-15%2023%3A52%3A04
> check.c:503:2: error: \342\200\230for\342\200\231 loop initial declarations are only allowed in C99 mode
> for (int tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
Thanks, fixed.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 18:53 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-09 19:23 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 23:42 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-15 23:35 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
@ 2020-10-20 23:06 ` Justin Pryzby <pryzby@telsasoft.com>
2020-10-21 01:17 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 13+ messages in thread
From: Justin Pryzby @ 2020-10-20 23:06 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: pgsql-hackers
On Thu, Oct 15, 2020 at 07:35:30PM -0400, Bruce Momjian wrote:
> On Fri, Oct 9, 2020 at 07:42:51PM -0400, Bruce Momjian wrote:
> > On Fri, Oct 9, 2020 at 02:23:10PM -0500, Justin Pryzby wrote:
> > > In my local branch, I had revised this comment to say:
> > >
> > > + * Note, v8.4 has no tablespace_suffix, which is fine so long as the version we
> > > + * being upgraded *to* has a suffix, since it's not allowed to pg_upgrade from
> > > + * a version to the same version if tablespaces are in use.
> >
> > OK, updated patch attached. Also, from your original patch, I didn't
> > need to call canonicalize_path() since we are not comparing paths, and I
> > didn't need to include common/relpath.h. I also renamed a variable.
>
> Patch applied to all supported versions. Thanks for the report, and the
I wonder if pg_upgrade should try to rmdir() the tablespace dirs before
restoring global objects, allowing it to succeed, rather than just "failing
early". That can avoid the need to re-create the new cluster, which I imagine
in some scenarios might be bad enough to require aborting the upgrade.
I propose only for master branch.
This doesn't avoid the need to re-create the new cluster if anything has been
restored into it (eg. if pg_tablespace is populated), it just cleans out any
empty dirs left behind by a previous pg_upgrade which failed after "restoring
global objects" but before copying/linking data into that tablespace.
|rm -fr pgsql12.dat pgsql14.dat tblspc;
|/usr/lib/postgresql/12/bin/initdb -D pgsql12.dat
|./tmp_install/usr/local/pgsql/bin/initdb -D pgsql14.dat
|mkdir tblspc tblspc/PG_14_202010201
|echo "CREATE TABLESPACE one LOCATION '`pwd`/tblspc'" |/usr/lib/postgresql/12/bin/postgres --single postgres -D pgsql12.dat -p 5678 -k /tmp
|./tmp_install/usr/local/pgsql/bin/pg_upgrade -D pgsql14.dat/ -d pgsql12.dat -b /usr/lib/postgresql/12/bin
--
Justin
Attachments:
v3-0001-Avoid-failure-if-tablespace-dirs-exist-and-empty.patch (text/x-diff, 2.9K) download
From 2eb5d39bc38474f3fe956d0f3344c47257133afd Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Tue, 20 Oct 2020 17:30:54 -0500
Subject: [PATCH v3] Avoid failure if tablespace dirs exist and empty..
See also: 3c0471b5fdd847febb30ec805a3e44f3eac5b66f
---
src/bin/pg_upgrade/check.c | 23 ++++++++++++++++-------
src/bin/pg_upgrade/pg_upgrade.c | 13 +++++++++++++
2 files changed, 29 insertions(+), 7 deletions(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 05e6bf7f2c..57f9fa9ad6 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -531,9 +531,9 @@ create_script_for_cluster_analyze(char **analyze_script_file_name)
/*
- * A previous run of pg_upgrade might have failed and the new cluster
- * directory recreated, but they might have forgotten to remove
- * the new cluster's tablespace directories. Therefore, check that
+ * A previous run of pg_upgrade might have failed.
+ * The admin might ahve recreated the new cluster directory, but forgotten to
+ * remove the new cluster's tablespace directories. Therefore, check that
* new cluster tablespace directories do not already exist. If
* they do, it would cause an error while restoring global objects.
* This allows the failure to be detected at check time, rather than
@@ -554,15 +554,24 @@ check_for_new_tablespace_dir(ClusterInfo *new_cluster)
for (tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
{
- struct stat statbuf;
-
snprintf(new_tablespace_dir, MAXPGPATH, "%s%s",
os_info.old_tablespaces[tblnum],
new_cluster->tablespace_suffix);
- if (stat(new_tablespace_dir, &statbuf) == 0 || errno != ENOENT)
- pg_fatal("new cluster tablespace directory already exists: \"%s\"\n",
+ /*
+ * New tablepace dirs are expected to not exist; if they
+ * do exist but are empty, we avoid failing here, and remove them
+ * before creating tablespaces while "restoring global objects".
+ */
+ switch (pg_check_dir(new_tablespace_dir))
+ {
+ case 0: /* Nonextant */
+ case 1: /* Empty */
+ break;
+ default:
+ pg_fatal("new cluster tablespace directory already exists and nonempty\"%s\"\n",
new_tablespace_dir);
+ }
}
check_ok();
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 1bc86e4205..6b7f8a690a 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -300,6 +300,19 @@ prepare_new_globals(void)
*/
set_frozenxids(false);
+ /*
+ * Remove any *empty* tablespace dirs left behind by a previous, failed
+ * upgrade.
+ */
+ for (int tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
+ {
+ char new_tablespace_dir[MAXPGPATH];
+ snprintf(new_tablespace_dir, MAXPGPATH, "%s%s",
+ os_info.old_tablespaces[tblnum],
+ new_cluster.tablespace_suffix);
+ (void) rmdir(new_tablespace_dir);
+ }
+
/*
* Now restore global objects (roles and tablespaces).
*/
--
2.17.0
* Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 18:53 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-09 19:23 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 23:42 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-15 23:35 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-20 23:06 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
@ 2020-10-21 01:17 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-10-21 11:54 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2020-10-21 01:17 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Bruce Momjian <bruce@momjian.us>; pgsql-hackers
Justin Pryzby <pryzby@telsasoft.com> writes:
> I wonder if pg_upgrade should try to rmdir() the tablespace dirs before
> restoring global objects, allowing it to succeed, rather than just "failing
> early".
I'm a little confused about that. If the directories aren't empty,
that will fail, but if they are, shouldn't the upgrade just work?
initdb is not normally unhappy about the target directory existing
if it's empty.
The reason why rmdir-and-recreate is not a great substitute for
"it just works" is that (a) you may lose the intended ownership or
permissions of those dirs, (b) you may lack write permission on their
parent dirs. So cases where the DBA has pre-created the directories
are important to support.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 18:53 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-09 19:23 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 23:42 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-15 23:35 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-20 23:06 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-21 01:17 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-10-21 11:54 ` Justin Pryzby <pryzby@telsasoft.com>
2020-10-27 02:17 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 13+ messages in thread
From: Justin Pryzby @ 2020-10-21 11:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Bruce Momjian <bruce@momjian.us>; pgsql-hackers
On Tue, Oct 20, 2020 at 09:17:22PM -0400, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > I wonder if pg_upgrade should try to rmdir() the tablespace dirs before
> > restoring global objects, allowing it to succeed, rather than just "failing
> > early".
>
> I'm a little confused about that. If the directories aren't empty,
> that will fail,
You mean rmdir() will fail, returning -1, which my patch will ignore, and the
pg_upgrade will fail, same as it would have before. The goal of the patch is
to improve the "empty" case, only.
> but if they are, shouldn't the upgrade just work?
It fails in "Restoring global objects", which runs "CREATE TABLESPACE".
| errmsg("directory \"%s\" already in use as a tablespace",
I considered the possibility of changing that, but it seems like this is
specific to pg_upgrade. I wouldn't want to change the core server just for
that, and it has a good reason for failing in that case:
| * The creation of the version directory prevents more than one tablespace
| * in a single location.
--
Justin
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 18:53 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-09 19:23 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 23:42 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-15 23:35 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Bruce Momjian <bruce@momjian.us>
2020-10-20 23:06 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-10-21 01:17 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Tom Lane <tgl@sss.pgh.pa.us>
2020-10-21 11:54 ` Re: pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
@ 2020-10-27 02:17 ` Bruce Momjian <bruce@momjian.us>
0 siblings, 0 replies; 13+ messages in thread
From: Bruce Momjian @ 2020-10-27 02:17 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Wed, Oct 21, 2020 at 06:54:17AM -0500, Justin Pryzby wrote:
> On Tue, Oct 20, 2020 at 09:17:22PM -0400, Tom Lane wrote:
> > Justin Pryzby <pryzby@telsasoft.com> writes:
> > > I wonder if pg_upgrade should try to rmdir() the tablespace dirs before
> > > restoring global objects, allowing it to succeed, rather than just "failing
> > > early".
> >
> > I'm a little confused about that. If the directories aren't empty,
> > that will fail,
>
> You mean rmdir() will fail, returning -1, which my patch will ignore, and the
> pg_upgrade will fail, same as it would have before. The goal of the patch is
> to improve the "empty" case, only.
>
> > but if they are, shouldn't the upgrade just work?
>
> It fails in "Restoring global objects", which runs "CREATE TABLESPACE".
> | errmsg("directory \"%s\" already in use as a tablespace",
>
> I considered the possibility of changing that, but it seems like this is
> specific to pg_upgrade. I wouldn't want to change the core server just for
> that, and it has a good reason for failing in that case:
> | * The creation of the version directory prevents more than one tablespace
> | * in a single location.
I think it is best to report an error --- pg_upgrade rarely tries to fix
things because that adds complexity, and perhaps bugs.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2020-10-27 02:17 UTC | newest]
Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-25 00:55 pg_upgrade: fail early if a tablespace dir already exists for new cluster version Justin Pryzby <pryzby@telsasoft.com>
2020-09-25 20:47 ` Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 18:53 ` Bruce Momjian <bruce@momjian.us>
2020-10-09 19:23 ` Justin Pryzby <pryzby@telsasoft.com>
2020-10-09 23:42 ` Bruce Momjian <bruce@momjian.us>
2020-10-12 17:33 ` Justin Pryzby <pryzby@telsasoft.com>
2020-10-15 23:35 ` Bruce Momjian <bruce@momjian.us>
2020-10-16 00:19 ` Andres Freund <andres@anarazel.de>
2020-10-16 00:37 ` Bruce Momjian <bruce@momjian.us>
2020-10-20 23:06 ` Justin Pryzby <pryzby@telsasoft.com>
2020-10-21 01:17 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-10-21 11:54 ` Justin Pryzby <pryzby@telsasoft.com>
2020-10-27 02:17 ` Bruce Momjian <bruce@momjian.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox