Am Dienstag, den 25.02.2020, 11:33 +0900 schrieb Michael Paquier:
> I really think that
> we should avoid duplicating the same logic around, and that we should
> remain consistent with non-directory entries in those paths,
> complaining with a proper failure if extra, unwanted files are
> present.
Okay, please find an updated patch attached.
My feeling is that in the case we cannot successfully resolve a
tablespace location from pg_tblspc, we should error out, but i could
imagine that people would like to have just a warning instead.
I've updated the TAP test for pg_checksums by adding a dummy
subdirectory into the tablespace directory already created for the
corrupted relfilenode test, containing a file to process in case an
unpatched pg_checksums is run. With the patch attached, these
directories simply won't be considered to check.
Thanks,
Bernd
From 15bd3a6457f59f3ba4bf72c645499132e90127cd Mon Sep 17 00:00:00 2001
From: Bernd Helmle <maili...@oopsware.de>
Date: Wed, 26 Feb 2020 16:49:03 +0100
Subject: [PATCH 1/2] Formerly pg_checksums recursively dived into pg_tblspc,
scanning any tablespace location found there without taking into account,
that tablespace locations might be used by other PostgreSQL instances at the
same time. This could likely happen for example by using pg_upgrade to
upgrade clusters with tablespaces.
Fix this by resolving tablespace locations explicitly by looking
up their TABLESPACE_VERSION_DIRECTORY subdirectory if called on
the pg_tblspc directory, and, if found, recursively diving into
them directly. If we can't resolve the tablespace location link, we
throw an error to indicate a probably orphaned or missing tablespace
location.
---
src/bin/pg_checksums/pg_checksums.c | 43 ++++++++++++++++++++++++++++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 9bd0bf947f..1367f53bbf 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -385,7 +385,48 @@ scan_directory(const char *basedir, const char *subdir, bool sizeonly)
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- dirsize += scan_directory(path, de->d_name, sizeonly);
+ {
+ /*
+ * If we are called with subdir pg_tblspc, we assume to
+ * operate on tablespace locations. We're just interested
+ * in TABLESPACE_VERSION_DIRECTORY only, so we're resolving
+ * the linked locations here explicitely and dive into them
+ * directly.
+ */
+ if (strncmp("pg_tblspc", subdir, strlen("pg_tblspc")) == 0)
+ {
+ char tblspc_path[MAXPGPATH];
+ struct stat tblspc_st;
+
+ /*
+ * Resolve tablespace location path and check
+ * whether a TABLESPACE_VERSION_DIRECTORY exists. Not finding
+ * a valid location is an unexpected condition, since there
+ * shouldn't be orphaned links. Tell the user something
+ * is wrong.
+ */
+ snprintf(tblspc_path, sizeof(tblspc_path), "%s/%s/%s",
+ path, de->d_name, TABLESPACE_VERSION_DIRECTORY);
+
+ if (lstat(tblspc_path, &tblspc_st) < 0)
+ {
+ pg_log_error("invalid tablespace location \"%s/%s\"",
+ subdir, de->d_name);
+ exit(1);
+ }
+
+ snprintf(tblspc_path, sizeof(tblspc_path), "%s/%s",
+ path, de->d_name);
+
+ /* Looks like a valid tablespace location */
+ dirsize += scan_directory(tblspc_path, TABLESPACE_VERSION_DIRECTORY,
+ sizeonly);
+ }
+ else
+ {
+ dirsize += scan_directory(path, de->d_name, sizeonly);
+ }
+ }
}
closedir(dir);
return dirsize;
--
2.24.1
From 77393902b2b9f92c51ad525eacde5ab87e55faac Mon Sep 17 00:00:00 2001
From: Bernd Helmle <maili...@oopsware.de>
Date: Wed, 26 Feb 2020 17:48:11 +0100
Subject: [PATCH 2/2] Update TAP tests for pg_checksums.
Add an additional test with a dummy foreign tablespace
location to check the new behavior of pg_checksums to properly
ignore any other versioned tablespace subdirectory from e.g.
pg_upgrade'd clusters.
---
src/bin/pg_checksums/t/002_actions.pl | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_checksums/t/002_actions.pl b/src/bin/pg_checksums/t/002_actions.pl
index 83a730ea94..88e186d873 100644
--- a/src/bin/pg_checksums/t/002_actions.pl
+++ b/src/bin/pg_checksums/t/002_actions.pl
@@ -5,7 +5,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 62;
+use Test::More tests => 63;
# Utility routine to create and check a table with corrupted checksums
@@ -217,6 +217,14 @@ sub fail_corrupt
# Stop instance for the follow-up checks.
$node->stop;
+# Create fake directories in the tablespace location, claiming
+# to be foreign objects we shouldn't touch.
+mkdir "$tablespace_dir/PG_99_999999991/";
+append_to_file "$tablespace_dir/PG_99_999999991/foo", "123";
+# Should not fail
+command_ok([ 'pg_checksums', '--check', '-D', $pgdata ],
+ "succeeds with foreign tablespace");
+
# Authorized relation files filled with corrupted data cause the
# checksum checks to fail. Make sure to use file names different
# than the previous ones.
--
2.24.1
