On Thu, Jan 30, 2020 at 06:11:22PM +0100, Michael Banck wrote: > The other question is whether it is possible to end up with a > pg_internal.init.$PID file in a running cluster. E.g. if an instance > crashes and gets started up again - are those cleaned up during crash > recovery, or should pg_checksums ignore them? Right now pg_checksums > only checks against a list of filenames and only skips on exact matches > not prefixes so that might take a bit of work.
Indeed, with a bad timing and a crash in the middle of write_relcache_init_file(), it could be possible to have such a file left around in the data folder. Having a past tablespace version left around after an upgrade is a pilot error in my opinion because pg_upgrade generates a script to cleanup past tablespaces, no? So your patch does not look like a good idea to me. And now that I look at it, if we happen to leave behind a temporary file for pg_internal.init, backups fail with the following error: 2020-01-31 13:26:18.345 JST [102076] 010_pg_basebackup.pl ERROR: invalid segment number 0 in file "pg_internal.init.123" So, I think that it would be better to change basebackup.c, pg_checksums and pg_rewind so as files are excluded if there is a prefix match with the exclude lists. Please see the attached. -- Michael
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index dea8aab45e..37e4adb57b 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -168,7 +168,8 @@ static const char *const excludeDirContents[] =
};
/*
- * List of files excluded from backups.
+ * List of files excluded from backups. Files are excluded if their
+ * prefix match.
*/
static const char *const excludeFiles[] =
{
@@ -198,7 +199,8 @@ static const char *const excludeFiles[] =
};
/*
- * List of files excluded from checksum validation.
+ * List of files excluded from checksum validation. Files are excluded
+ * if their prefix match.
*
* Note: this list should be kept in sync with what pg_checksums.c
* includes.
@@ -210,7 +212,6 @@ static const char *const noChecksumFiles[] = {
"PG_VERSION",
#ifdef EXEC_BACKEND
"config_exec_params",
- "config_exec_params.new",
#endif
NULL,
};
@@ -1084,7 +1085,8 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces,
excludeFound = false;
for (excludeIdx = 0; excludeFiles[excludeIdx] != NULL; excludeIdx++)
{
- if (strcmp(de->d_name, excludeFiles[excludeIdx]) == 0)
+ if (strncmp(de->d_name, excludeFiles[excludeIdx],
+ strlen(excludeFiles[excludeIdx])) == 0)
{
elog(DEBUG1, "file \"%s\" excluded from backup", de->d_name);
excludeFound = true;
@@ -1326,7 +1328,7 @@ is_checksummed_file(const char *fullpath, const char *filename)
{
/* Compare file against noChecksumFiles skiplist */
for (f = noChecksumFiles; *f; f++)
- if (strcmp(*f, filename) == 0)
+ if (strncmp(*f, filename, strlen(*f)) == 0)
return false;
return true;
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index b7d36b65dd..1c14317099 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -6,7 +6,7 @@ use File::Basename qw(basename dirname);
use File::Path qw(rmtree);
use PostgresNode;
use TestLib;
-use Test::More tests => 106;
+use Test::More tests => 107;
program_help_ok('pg_basebackup');
program_version_ok('pg_basebackup');
@@ -65,7 +65,8 @@ $node->restart;
# Write some files to test that they are not copied.
foreach my $filename (
- qw(backup_label tablespace_map postgresql.auto.conf.tmp current_logfiles.tmp)
+ qw(backup_label tablespace_map postgresql.auto.conf.tmp
+ current_logfiles.tmp global/pg_internal.init.123)
)
{
open my $file, '>>', "$pgdata/$filename";
@@ -135,7 +136,7 @@ foreach my $dirname (
# These files should not be copied.
foreach my $filename (
qw(postgresql.auto.conf.tmp postmaster.opts postmaster.pid tablespace_map current_logfiles.tmp
- global/pg_internal.init))
+ global/pg_internal.init global/pg_internal.init.123))
{
ok(!-f "$tempdir/backup/$filename", "$filename not copied");
}
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 46ee1f1dc3..cd1a4bcd60 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -92,7 +92,8 @@ usage(void)
}
/*
- * List of files excluded from checksum validation.
+ * List of files excluded from checksum validation. Files are excluded
+ * if their prefix match.
*
* Note: this list should be kept in sync with what basebackup.c includes.
*/
@@ -103,7 +104,6 @@ static const char *const skip[] = {
"PG_VERSION",
#ifdef EXEC_BACKEND
"config_exec_params",
- "config_exec_params.new",
#endif
NULL,
};
@@ -160,7 +160,7 @@ skipfile(const char *fn)
const char *const *f;
for (f = skip; *f; f++)
- if (strcmp(*f, fn) == 0)
+ if (strncmp(*f, fn, strlen(*f)) == 0)
return true;
return false;
diff --git a/src/bin/pg_checksums/t/002_actions.pl b/src/bin/pg_checksums/t/002_actions.pl
index 59228b916c..933cf72a7c 100644
--- a/src/bin/pg_checksums/t/002_actions.pl
+++ b/src/bin/pg_checksums/t/002_actions.pl
@@ -112,6 +112,8 @@ append_to_file "$pgdata/global/99999_vm.123", "";
append_to_file "$pgdata/global/pgsql_tmp_123", "foo";
mkdir "$pgdata/global/pgsql_tmp";
append_to_file "$pgdata/global/pgsql_tmp/1.1", "foo";
+append_to_file "$pgdata/global/pg_internal.init", "foo";
+append_to_file "$pgdata/global/pg_internal.init.123", "foo";
# Enable checksums.
command_ok([ 'pg_checksums', '--enable', '--no-sync', '-D', $pgdata ],
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index fd14844eec..7e55f74e8b 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -78,7 +78,8 @@ static const char *excludeDirContents[] =
};
/*
- * List of files excluded from filemap processing.
+ * List of files excluded from filemap processing. Files are excluded
+ * if their prefix match.
*/
static const char *excludeFiles[] =
{
@@ -503,7 +504,8 @@ check_file_excluded(const char *path, bool is_source)
filename = path;
else
filename++;
- if (strcmp(filename, excludeFiles[excludeIdx]) == 0)
+ if (strncmp(filename, excludeFiles[excludeIdx],
+ strlen(excludeFiles[excludeIdx])) == 0)
{
if (is_source)
pg_log_debug("entry \"%s\" excluded from source file list",
signature.asc
Description: PGP signature
