Am Dienstag, den 18.02.2020, 15:15 +0900 schrieb Michael Paquier:
> Fair point. Now, while the proposed patch is right to use
> TABLESPACE_VERSION_DIRECTORY, shouldn't we use strncmp based on the
> length of TABLESPACE_VERSION_DIRECTORY instead of de->d_name? It
> seems also to me that the code as proposed is rather fragile, and
> that
> we had better be sure that the check only happens when we are
> scanning
> entries within pg_tblspc.
>
Yes, after thinking and playing around with it a little i share your
position. You can still easily cause pg_checksums to error out by just
having arbitrary files around in the reference tablespace locations.
Though i don't think this is something of a big issue, it looks strange
and misleading if pg_checksums just complains about files not belonging
to the scanned PostgreSQL data directory (even we explicitly note in
the docs that even tablespace locations are somehow taboo for DBAs to
put other files and/or directories in there).
So i propose a different approach like the attached patch tries to
implement: instead of just blindly iterating over directory contents
and filter them out, reference the tablespace location and
TABLESPACE_VERSION_DIRECTORY directly. This is done by a new function
scan_tablespaces() which is specialized in just follow the
symlinks/junctions in pg_tblspc and call scan_directory() with just
what it has found there. It will also honour directories, just in case
an experienced DBA has copied over the tablespace into pg_tblspc
directly.
> The issue with pg_internal.init.XX is quite different, so I think
> that
> it would be better to commit that separately first.
Agreed.
Thanks,
Bernd
From 4b6d369f731f111216f9ed6613ad1b41872fb0ea Mon Sep 17 00:00:00 2001
From: Bernd Helmle <maili...@oopsware.de>
Date: Thu, 20 Feb 2020 17:18:10 +0100
Subject: [PATCH] Make scanning pg_tblspc more robust.
Currently we dive blindly into the subdirectories referenced by
the symlinks (or under Windows via file junctions) located in pg_tblspc,
which has various problems. We need to filter out any other possible tablespace
location found in the referenced subdirectory, which could likely happen
if you have e.g. an upgraded instance via pg_upgrade or other installations using
the same tablespace location.
Instead of filtering out any of these possible foreign directories, try to
resolve the locations in pg_tblspc directly. This is done with the new
scan_tablespaces() helper function, which indirectly calls scan_directory()
after resolving the links in pg_tblspc. It explicitely follows directories found
there too, since it might happen that users copy over tablespace locations directly
into pg_tblspc.
---
src/bin/pg_checksums/pg_checksums.c | 99 ++++++++++++++++++++++++++++-
1 file changed, 97 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 46ee1f1dc3..4e2e9cb3e2 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -108,6 +108,15 @@ static const char *const skip[] = {
NULL,
};
+/*
+ * Forwarded declarations.
+ */
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly);
+
+static int64
+scan_tablespaces(const char *basedir, bool sizeonly);
+
/*
* Report current progress status. Parts borrowed from
* src/bin/pg_basebackup/pg_basebackup.c.
@@ -267,6 +276,91 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
+/*
+ * Scans pg_tblspc of the given base directory for either links
+ * or directories and examines wether an existing tablespace version
+ * directory exists there. If true, scan_directory() will do the checksum
+ * legwork. sizeonly specifies wether we want to compute the directory size
+ * only, required for progress reporting.
+ */
+static int64
+scan_tablespaces(const char *basedir, bool sizeonly)
+{
+ int64 dirsize = 0;
+ char path[MAXPGPATH];
+ DIR *dir;
+ struct dirent *de;
+
+ snprintf(path, sizeof(path), "%s/%s", basedir, "pg_tblspc");
+ dir = opendir(path);
+
+ if (!dir)
+ {
+ pg_log_error("could not open directory \"%s\": %m", path);
+ exit(1);
+ }
+
+ /*
+ * Scan through the pg_tblspc contents. We expect either a symlink
+ * (or Windows junction) or a directory. The latter might exist due
+ * systems where administrators copy over the tablespace directory
+ * directly into PGDATA, e.g. on recovery systems.
+ */
+ while((de = readdir(dir)) != NULL)
+ {
+ char tblspc_path[MAXPGPATH];
+ char fn[MAXPGPATH];
+ struct stat st;
+
+ if (strcmp(de->d_name, ".") == 0 ||
+ strcmp(de->d_name, "..") == 0)
+ continue;
+
+ snprintf(fn, sizeof(fn), "%s/%s", path, de->d_name);
+ if (lstat(fn, &st) < 0)
+ {
+ pg_log_error("could not stat file \"%s\": %m", fn);
+ exit(1);
+ }
+
+ if (S_ISREG(st.st_mode))
+ {
+ pg_log_debug("ignoring file %s in pg_tblspc", de->d_name);
+ continue;
+ }
+
+#ifndef WIN32
+ if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
+#else
+ if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
+#endif
+ {
+ /*
+ * Every link or tablespace directory found here
+ * must have a TABLESPACE_VERSION_DIRECTORY. We build
+ * the direct path to it and check via lstat wether it exists.
+ */
+ snprintf(tblspc_path, sizeof(tblspc_path), "%s/%s",
+ fn, TABLESPACE_VERSION_DIRECTORY);
+
+ if (lstat(tblspc_path, &st) < 0)
+ {
+ pg_log_warning("directory or link \"%s\" doesn't contain a valid tablespace" , fn);
+ continue;
+ }
+
+ /* Seems okay, dive into and do the ordinary checksum checks. */
+ dirsize += scan_directory(fn, TABLESPACE_VERSION_DIRECTORY, sizeonly);
+ }
+ else
+ {
+ pg_log_debug("ignoring file %s in pg_tblspc", de->d_name);
+ }
+ }
+
+ return dirsize;
+}
+
/*
* Scan the given directory for items which can be checksummed and
* operate on each one of them. If "sizeonly" is true, the size of
@@ -438,6 +532,7 @@ main(int argc, char *argv[])
break;
case 'v':
verbose = true;
+ pg_logging_set_level(PG_LOG_DEBUG);
break;
case 'D':
DataDir = optarg;
@@ -553,12 +648,12 @@ main(int argc, char *argv[])
{
total_size = scan_directory(DataDir, "global", true);
total_size += scan_directory(DataDir, "base", true);
- total_size += scan_directory(DataDir, "pg_tblspc", true);
+ total_size += scan_tablespaces(DataDir, true);
}
(void) scan_directory(DataDir, "global", false);
(void) scan_directory(DataDir, "base", false);
- (void) scan_directory(DataDir, "pg_tblspc", false);
+ (void) scan_tablespaces(DataDir, false);
if (showprogress)
{
--
2.24.1
