Hi all, This is a follow-up of the following thread: https://www.postgresql.org/message-id/20181012010411.re53cwcistcpi...@alap3.anarazel.de
In a nutshell, b34e84f1 has added TAP tests for pg_verify_checksums, and the buildfarm has immediately complained about files generated by EXEC_BACKEND missing, causing pg_verify_checksums to fail for such builds. An extra issue has been noticed by Andres in the tool: it misses to check for temporary files, hence files like base/pgsql_tmp/pgsql_tmp$PID.NN.sharedfileset/1.1 can also cause the tool to fail. After a crash, those files would not be removed, and stopping the instance would still let them around. pg_verify_checksums used first a blacklist to decide if files have checksums or not. So with this approach all files should have checksums except files like pg_control, pg_filenode.map, PG_VERSION, etc. d55241a has added a first fix to silence the buildfarm for EXEC_BACKEND builds. After discussion, Andres has pointed out that some extensions may want to drop files within global/ or base/ as part of their logic. cstore_fdw was mentioned but if you look at their README that's not the case. However, I think that Andres argument is pretty good as with pluggable storage we should allow extensions to put custom files for different tablespaces. So this commit has changed the logic of pg_verify_checksums to use a whitelist, which assumes that only normal relation files have checksums: <digits> <digits>.<segment> <digits>_<forkname> <digits>_<forkname>.<segment After more discussion on the thread mentioned above, Stephen has pointed out that base backups use the same blacklist logic when verifying checksums. This has the same problem with EXEC_BACKEND-specific files, resulting in spurious warnings (that's an example!) which are confusing for the user: $ pg_basebackup -D popo WARNING: cannot verify checksum in file "./global/config_exec_params", block 0: read buffer size 5 and page size 8192 differ Folks on this thread agreed that both pg_verify_checksums and checksum verification for base backups should use the same logic. It seems to me that using a whitelist-based approach for both is easier to maintain as the patterns of files supporting checksums is more limited than files which do not support checksums. So this way we allow extensions bundling custom files to still work with pg_verify_checksums and checksum verification in base backups. Something else which has been discussed on this thread is that we should have a dedicated API to decide if a file has checksums or not, and if it should be skipped in both cases. That's work only for HEAD though, so we need to do something for HEAD and v11, which is simple. Attached is a patch I have cooked for this purpose. I have studied the file patterns opened by the backend, and we actually need to only skip temporary files and folders as those can include legit relation file names (like 1.1 for example). At the same time I have found about parse_filename_for_nontemp_relation() which is a copycat of the recently-added isRelFileName(), so we can easily shave some code by reusing it in both cases. This also takes care of the issues for temporary files, and it also fixes an extra bug coming from the original implementation which checked the file patterns without looking at the file type first, so the tool could miss some cascading paths. This should be applied to v11 and HEAD. Please feel free to comment. Thanks for reading. -- Michael
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index b20f6c379c..207e2facb8 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -72,7 +72,6 @@ static void parse_basebackup_options(List *options, basebackup_options *opt);
static void SendXlogRecPtrResult(XLogRecPtr ptr, TimeLineID tli);
static int compareWalFileNames(const void *a, const void *b);
static void throttle(size_t increment);
-static bool is_checksummed_file(const char *fullpath, const char *filename);
/* Was the backup currently in-progress initiated in recovery mode? */
static bool backup_started_in_recovery = false;
@@ -187,17 +186,6 @@ static const char *excludeFiles[] =
NULL
};
-/*
- * List of files excluded from checksum validation.
- */
-static const char *const noChecksumFiles[] = {
- "pg_control",
- "pg_filenode.map",
- "pg_internal.init",
- "PG_VERSION",
- NULL,
-};
-
/*
* Called when ERROR or FATAL happens in perform_base_backup() after
@@ -1101,8 +1089,7 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces,
/* Exclude all forks for unlogged tables except the init fork */
if (isDbDir &&
- parse_filename_for_nontemp_relation(de->d_name, &relOidChars,
- &relForkNum))
+ ParseRelationFileName(de->d_name, &relOidChars, &relForkNum))
{
/* Never exclude init forks */
if (relForkNum != INIT_FORKNUM)
@@ -1312,32 +1299,6 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces,
return size;
}
-/*
- * Check if a file should have its checksum validated.
- * We validate checksums on files in regular tablespaces
- * (including global and default) only, and in those there
- * are some files that are explicitly excluded.
- */
-static bool
-is_checksummed_file(const char *fullpath, const char *filename)
-{
- const char *const *f;
-
- /* Check that the file is in a tablespace */
- if (strncmp(fullpath, "./global/", 9) == 0 ||
- strncmp(fullpath, "./base/", 7) == 0 ||
- strncmp(fullpath, "/", 1) == 0)
- {
- /* Compare file against noChecksumFiles skiplist */
- for (f = noChecksumFiles; *f; f++)
- if (strcmp(*f, filename) == 0)
- return false;
-
- return true;
- }
- else
- return false;
-}
/*****
* Functions for handling tar file format
@@ -1391,13 +1352,15 @@ sendFile(const char *readfilename, const char *tarfilename, struct stat *statbuf
char *filename;
/*
- * Get the filename (excluding path). As last_dir_separator()
- * includes the last directory separator, we chop that off by
- * incrementing the pointer.
+ * Check if a given file should have its checksums verified or not.
+ * Only relation files should have this property now. First get the
+ * filename (excluding path). As last_dir_separator() includes the
+ * last directory separator, we chop that off by incrementing the
+ * pointer.
*/
filename = last_dir_separator(readfilename) + 1;
- if (is_checksummed_file(readfilename, filename))
+ if (ParseRelationFileName(filename, NULL, NULL))
{
verify_checksum = true;
diff --git a/src/backend/storage/file/reinit.c b/src/backend/storage/file/reinit.c
index 74ff6c359b..eae32e9a5d 100644
--- a/src/backend/storage/file/reinit.c
+++ b/src/backend/storage/file/reinit.c
@@ -186,8 +186,7 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
unlogged_relation_entry ent;
/* Skip anything that doesn't look like a relation data file. */
- if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars,
- &forkNum))
+ if (!ParseRelationFileName(de->d_name, &oidchars, &forkNum))
continue;
/* Also skip it unless this is the init fork. */
@@ -228,8 +227,7 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
unlogged_relation_entry ent;
/* Skip anything that doesn't look like a relation data file. */
- if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars,
- &forkNum))
+ if (!ParseRelationFileName(de->d_name, &oidchars, &forkNum))
continue;
/* We never remove the init fork. */
@@ -284,8 +282,7 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
char dstpath[MAXPGPATH];
/* Skip anything that doesn't look like a relation data file. */
- if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars,
- &forkNum))
+ if (!ParseRelationFileName(de->d_name, &oidchars, &forkNum))
continue;
/* Also skip it unless this is the init fork. */
@@ -326,8 +323,7 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
char mainpath[MAXPGPATH];
/* Skip anything that doesn't look like a relation data file. */
- if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars,
- &forkNum))
+ if (!ParseRelationFileName(de->d_name, &oidchars, &forkNum))
continue;
/* Also skip it unless this is the init fork. */
@@ -357,59 +353,3 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
fsync_fname(dbspacedirname, true);
}
}
-
-/*
- * Basic parsing of putative relation filenames.
- *
- * This function returns true if the file appears to be in the correct format
- * for a non-temporary relation and false otherwise.
- *
- * NB: If this function returns true, the caller is entitled to assume that
- * *oidchars has been set to the a value no more than OIDCHARS, and thus
- * that a buffer of OIDCHARS+1 characters is sufficient to hold the OID
- * portion of the filename. This is critical to protect against a possible
- * buffer overrun.
- */
-bool
-parse_filename_for_nontemp_relation(const char *name, int *oidchars,
- ForkNumber *fork)
-{
- int pos;
-
- /* Look for a non-empty string of digits (that isn't too long). */
- for (pos = 0; isdigit((unsigned char) name[pos]); ++pos)
- ;
- if (pos == 0 || pos > OIDCHARS)
- return false;
- *oidchars = pos;
-
- /* Check for a fork name. */
- if (name[pos] != '_')
- *fork = MAIN_FORKNUM;
- else
- {
- int forkchar;
-
- forkchar = forkname_chars(&name[pos + 1], fork);
- if (forkchar <= 0)
- return false;
- pos += forkchar + 1;
- }
-
- /* Check for a segment number. */
- if (name[pos] == '.')
- {
- int segchar;
-
- for (segchar = 1; isdigit((unsigned char) name[pos + segchar]); ++segchar)
- ;
- if (segchar <= 1)
- return false;
- pos += segchar;
- }
-
- /* Now we should be at the end. */
- if (name[pos] != '\0')
- return false;
- return true;
-}
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index f0e09bea20..eeca532227 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -21,6 +21,7 @@
#include "storage/bufpage.h"
#include "storage/checksum.h"
#include "storage/checksum_impl.h"
+#include "storage/fd.h"
static int64 files = 0;
@@ -50,71 +51,6 @@ usage(void)
printf(_("Report bugs to <pgsql-b...@postgresql.org>.\n"));
}
-/*
- * isRelFileName
- *
- * Check if the given file name is authorized for checksum verification.
- */
-static bool
-isRelFileName(const char *fn)
-{
- int pos;
-
- /*----------
- * Only files including data checksums are authorized for verification.
- * This is guessed based on the file name by reverse-engineering
- * GetRelationPath() so make sure to update both code paths if any
- * updates are done. The following file name formats are allowed:
- * <digits>
- * <digits>.<segment>
- * <digits>_<forkname>
- * <digits>_<forkname>.<segment>
- *
- * Note that temporary files, beginning with 't', are also skipped.
- *
- *----------
- */
-
- /* A non-empty string of digits should follow */
- for (pos = 0; isdigit((unsigned char) fn[pos]); ++pos)
- ;
- /* leave if no digits */
- if (pos == 0)
- return false;
- /* good to go if only digits */
- if (fn[pos] == '\0')
- return true;
-
- /* Authorized fork files can be scanned */
- if (fn[pos] == '_')
- {
- int forkchar = forkname_chars(&fn[pos + 1], NULL);
-
- if (forkchar <= 0)
- return false;
-
- pos += forkchar + 1;
- }
-
- /* Check for an optional segment number */
- if (fn[pos] == '.')
- {
- int segchar;
-
- for (segchar = 1; isdigit((unsigned char) fn[pos + segchar]); ++segchar)
- ;
-
- if (segchar <= 1)
- return false;
- pos += segchar;
- }
-
- /* Now this should be the end */
- if (fn[pos] != '\0')
- return false;
- return true;
-}
-
static void
scan_file(const char *fn, BlockNumber segmentno)
{
@@ -189,9 +125,22 @@ scan_directory(const char *basedir, const char *subdir)
char fn[MAXPGPATH];
struct stat st;
- if (!isRelFileName(de->d_name))
+ if (strcmp(de->d_name, ".") == 0 ||
+ strcmp(de->d_name, "..") == 0)
continue;
+ /* Skip temporary files */
+ if (strncmp(de->d_name,
+ PG_TEMP_FILE_PREFIX,
+ strlen(PG_TEMP_FILE_PREFIX)) == 0)
+ continue;
+
+ /* Skip temporary folders */
+ if (strncmp(de->d_name,
+ PG_TEMP_FILES_DIR,
+ strlen(PG_TEMP_FILES_DIR)) == 0)
+ return;
+
snprintf(fn, sizeof(fn), "%s/%s", path, de->d_name);
if (lstat(fn, &st) < 0)
{
@@ -206,6 +155,13 @@ scan_directory(const char *basedir, const char *subdir)
*segmentpath;
BlockNumber segmentno = 0;
+ /*
+ * Only normal relation files can be analyzed. Note that this
+ * skips temporary relations.
+ */
+ if (!ParseRelationFileName(de->d_name, NULL, NULL))
+ continue;
+
/*
* Cut off at the segment boundary (".") to get the segment number
* in order to mix it into the checksum. Then also cut off at the
diff --git a/src/common/relpath.c b/src/common/relpath.c
index e8170ed712..be4d64b819 100644
--- a/src/common/relpath.c
+++ b/src/common/relpath.c
@@ -205,3 +205,64 @@ GetRelationPath(Oid dbNode, Oid spcNode, Oid relNode,
}
return path;
}
+
+/*
+ * ParseRelationFileName
+ *
+ * Basic parsing of putative relation filenames.
+ *
+ * This function returns true if the file appears to be in the correct format
+ * for a non-temporary relation and false otherwise.
+ *
+ * NB: If this function returns true, the caller is entitled to assume that
+ * *oidchars has been set to the a value no more than OIDCHARS, and thus
+ * that a buffer of OIDCHARS+1 characters is sufficient to hold the OID
+ * portion of the filename. This is critical to protect against a possible
+ * buffer overrun.
+ */
+bool
+ParseRelationFileName(const char *name, int *oidchars, ForkNumber *fork)
+{
+ int pos;
+
+ /* Look for a non-empty string of digits (that isn't too long). */
+ for (pos = 0; isdigit((unsigned char) name[pos]); ++pos)
+ ;
+ if (pos == 0 || pos > OIDCHARS)
+ return false;
+ if (oidchars)
+ *oidchars = pos;
+
+ /* Check for a fork name. */
+ if (name[pos] != '_')
+ {
+ if (fork)
+ *fork = MAIN_FORKNUM;
+ }
+ else
+ {
+ int forkchar;
+
+ forkchar = forkname_chars(&name[pos + 1], fork);
+ if (forkchar <= 0)
+ return false;
+ pos += forkchar + 1;
+ }
+
+ /* Check for a segment number. */
+ if (name[pos] == '.')
+ {
+ int segchar;
+
+ for (segchar = 1; isdigit((unsigned char) name[pos + segchar]); ++segchar)
+ ;
+ if (segchar <= 1)
+ return false;
+ pos += segchar;
+ }
+
+ /* Now we should be at the end. */
+ if (name[pos] != '\0')
+ return false;
+ return true;
+}
diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
index 82d817a53c..6c6c694e39 100644
--- a/src/include/common/relpath.h
+++ b/src/include/common/relpath.h
@@ -69,6 +69,12 @@ extern char *GetDatabasePath(Oid dbNode, Oid spcNode);
extern char *GetRelationPath(Oid dbNode, Oid spcNode, Oid relNode,
int backendId, ForkNumber forkNumber);
+/*
+ * Stuff to parse and validate file names for relations.
+ */
+extern bool ParseRelationFileName(const char *name, int *oidchars,
+ ForkNumber *fork);
+
/*
* Wrapper macros for GetRelationPath. Beware of multiple
* evaluation of the RelFileNode or RelFileNodeBackend argument!
diff --git a/src/include/storage/reinit.h b/src/include/storage/reinit.h
index a62703c647..f702d598dd 100644
--- a/src/include/storage/reinit.h
+++ b/src/include/storage/reinit.h
@@ -19,8 +19,6 @@
extern void ResetUnloggedRelations(int op);
-extern bool parse_filename_for_nontemp_relation(const char *name,
- int *oidchars, ForkNumber *fork);
#define UNLOGGED_RELATION_CLEANUP 0x0001
#define UNLOGGED_RELATION_INIT 0x0002
signature.asc
Description: PGP signature
