From da2a7180aa0ed18d4e283a934d3f789912fd99c5 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 24 Feb 2020 18:14:22 +0900 Subject: [PATCH] Add prefix checks in exclude lists for pg_rewind, pg_checksums and base backups An instance of PostgreSQL crashing with a bad timing could leave behind temporary pg_internal.init files, potentially causing failures when verifying checksums. As the same exclusion lists are used between pg_rewind, pg_checksums and basebackup.c, all those tools are extended with prefix checks to keep everything in sync, with dedicated checks added for pg_internal.init. Backpatch down to 11, where pg_checksums (pg_verify_checksums in 11) and checksum verification for base backups have been introduced. Reported-by: Michael Banck Author: Michael Paquier Reviewed-by: Kyotaro Horiguchi, David Steele Discussion: https://postgr.es/m/62031974fd8e941dd8351fbc8c7eff60d59c5338.camel@credativ.de Backpatch-through: 11 --- src/backend/replication/basebackup.c | 75 ++++++++++++------- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 8 +- src/bin/pg_rewind/filemap.c | 43 ++++++++--- .../pg_verify_checksums/pg_verify_checksums.c | 39 +++++++--- 4 files changed, 113 insertions(+), 52 deletions(-) diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index f46e73544b..2eba0dc21a 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -123,6 +123,18 @@ static int64 total_checksum_failures; /* Do not verify checksums. */ static bool noverify_checksums = false; +/* + * Definition of one element part of an exclusion list, used for paths part + * of checksum validation or base backups. "name" is the name of the file + * or path to check for exclusion. If "match_prefix" is true, any items + * matching the name as prefix are excluded. + */ +struct exclude_list_item +{ + const char *name; + bool match_prefix; +}; + /* * The contents of these directories are removed or recreated during server * start so they are not included in backups. The directories themselves are @@ -172,16 +184,19 @@ static const char *excludeDirContents[] = /* * List of files excluded from backups. */ -static const char *excludeFiles[] = +static const struct exclude_list_item excludeFiles[] = { /* Skip auto conf temporary file. */ - PG_AUTOCONF_FILENAME ".tmp", + {PG_AUTOCONF_FILENAME ".tmp", false}, /* Skip current log file temporary file */ - LOG_METAINFO_DATAFILE_TMP, + {LOG_METAINFO_DATAFILE_TMP, false}, - /* Skip relation cache because it is rebuilt on startup */ - RELCACHE_INIT_FILENAME, + /* + * Skip relation cache because it is rebuilt on startup. This includes + * temporary files. + */ + {RELCACHE_INIT_FILENAME, true}, /* * If there's a backup_label or tablespace_map file, it belongs to a @@ -189,14 +204,14 @@ static const char *excludeFiles[] = * for this backup. Our backup_label/tablespace_map is injected into the * tar separately. */ - BACKUP_LABEL_FILE, - TABLESPACE_MAP, + {BACKUP_LABEL_FILE, false}, + {TABLESPACE_MAP, false}, - "postmaster.pid", - "postmaster.opts", + {"postmaster.pid", false}, + {"postmaster.opts", false}, /* end of list */ - NULL + {NULL, false} }; /* @@ -205,16 +220,15 @@ static const char *excludeFiles[] = * Note: this list should be kept in sync with what pg_verify_checksums.c * includes. */ -static const char *noChecksumFiles[] = { - "pg_control", - "pg_filenode.map", - "pg_internal.init", - "PG_VERSION", +static const struct exclude_list_item noChecksumFiles[] = { + {"pg_control", false}, + {"pg_filenode.map", false}, + {"pg_internal.init", true}, + {"PG_VERSION", false}, #ifdef EXEC_BACKEND - "config_exec_params", - "config_exec_params.new", + {"config_exec_params", true}, #endif - NULL, + {NULL, false} }; @@ -1107,9 +1121,13 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces, /* Scan for files that should be excluded */ excludeFound = false; - for (excludeIdx = 0; excludeFiles[excludeIdx] != NULL; excludeIdx++) + for (excludeIdx = 0; excludeFiles[excludeIdx].name != NULL; excludeIdx++) { - if (strcmp(de->d_name, excludeFiles[excludeIdx]) == 0) + int cmplen = strlen(excludeFiles[excludeIdx].name); + + if (!excludeFiles[excludeIdx].match_prefix) + cmplen++; + if (strncmp(de->d_name, excludeFiles[excludeIdx].name, cmplen) == 0) { elog(DEBUG1, "file \"%s\" excluded from backup", de->d_name); excludeFound = true; @@ -1342,17 +1360,24 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces, static bool is_checksummed_file(const char *fullpath, const char *filename) { - const char **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) + int excludeIdx; + + /* Compare file against noChecksumFiles skip list */ + for (excludeIdx = 0; noChecksumFiles[excludeIdx].name != NULL; excludeIdx++) + { + int cmplen = strlen(noChecksumFiles[excludeIdx].name); + + if (!noChecksumFiles[excludeIdx].match_prefix) + cmplen++; + if (strncmp(filename, noChecksumFiles[excludeIdx].name, + cmplen) == 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 000e09e0a7..851a7db52f 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,8 +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"; print $file "DONOTCOPY"; @@ -135,7 +135,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_rewind/filemap.c b/src/bin/pg_rewind/filemap.c index d70f118938..197163d554 100644 --- a/src/bin/pg_rewind/filemap.c +++ b/src/bin/pg_rewind/filemap.c @@ -32,6 +32,18 @@ static int final_filemap_cmp(const void *a, const void *b); static void filemap_list_to_array(filemap_t *map); static bool check_file_excluded(const char *path, bool is_source); +/* + * Definition of one element part of an exclusion list, used to exclude + * contents when rewinding. "name" is the name of the file or path to + * check for exclusion. If "match_prefix" is true, any items matching + * the name as prefix are excluded. + */ +struct exclude_list_item +{ + const char *name; + bool match_prefix; +}; + /* * The contents of these directories are removed or recreated during server * start so they are not included in data processed by pg_rewind. @@ -80,32 +92,34 @@ 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[] = +static const struct exclude_list_item excludeFiles[] = { /* Skip auto conf temporary file. */ - "postgresql.auto.conf.tmp", /* defined as PG_AUTOCONF_FILENAME */ + {"postgresql.auto.conf.tmp", false}, /* defined as PG_AUTOCONF_FILENAME */ /* Skip current log file temporary file */ - "current_logfiles.tmp", /* defined as LOG_METAINFO_DATAFILE_TMP */ + {"current_logfiles.tmp", false}, /* defined as + * LOG_METAINFO_DATAFILE_TMP */ /* Skip relation cache because it is rebuilt on startup */ - "pg_internal.init", /* defined as RELCACHE_INIT_FILENAME */ + {"pg_internal.init", true}, /* defined as RELCACHE_INIT_FILENAME */ /* * If there's a backup_label or tablespace_map file, it belongs to a * backup started by the user with pg_start_backup(). It is *not* correct * for this backup. Our backup_label is written later on separately. */ - "backup_label", /* defined as BACKUP_LABEL_FILE */ - "tablespace_map", /* defined as TABLESPACE_MAP */ + {"backup_label", false}, /* defined as BACKUP_LABEL_FILE */ + {"tablespace_map", false}, /* defined as TABLESPACE_MAP */ - "postmaster.pid", - "postmaster.opts", + {"postmaster.pid", false}, + {"postmaster.opts", false}, /* end of list */ - NULL + {NULL, false} }; /* @@ -498,14 +512,19 @@ check_file_excluded(const char *path, bool is_source) const char *filename; /* check individual files... */ - for (excludeIdx = 0; excludeFiles[excludeIdx] != NULL; excludeIdx++) + for (excludeIdx = 0; excludeFiles[excludeIdx].name != NULL; excludeIdx++) { + int cmplen = strlen(excludeFiles[excludeIdx].name); + filename = last_dir_separator(path); if (filename == NULL) filename = path; else filename++; - if (strcmp(filename, excludeFiles[excludeIdx]) == 0) + + if (!excludeFiles[excludeIdx].match_prefix) + cmplen++; + if (strncmp(filename, excludeFiles[excludeIdx].name, cmplen) == 0) { if (is_source) pg_log(PG_DEBUG, "entry \"%s\" excluded from source file list\n", diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c index 9316835995..a33ac6924f 100644 --- a/src/bin/pg_verify_checksums/pg_verify_checksums.c +++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c @@ -50,31 +50,48 @@ usage(void) printf(_("Report bugs to .\n")); } +/* + * Definition of one element part of an exclusion list, used for files + * to exclude from checksum validation. "name" is the name of the file + * or path to check for exclusion. If "match_prefix" is true, any items + * matching the name as prefix are excluded. + */ +struct exclude_list_item +{ + const char *name; + bool match_prefix; +}; + /* * List of files excluded from checksum validation. * * Note: this list should be kept in sync with what basebackup.c includes. */ -static const char *const skip[] = { - "pg_control", - "pg_filenode.map", - "pg_internal.init", - "PG_VERSION", +static const struct exclude_list_item skip[] = { + {"pg_control", false}, + {"pg_filenode.map", false}, + {"pg_internal.init", true}, + {"PG_VERSION", false}, #ifdef EXEC_BACKEND - "config_exec_params", - "config_exec_params.new", + {"config_exec_params", true}, #endif - NULL, + {NULL, false} }; static bool skipfile(const char *fn) { - const char *const *f; + int excludeIdx; - for (f = skip; *f; f++) - if (strcmp(*f, fn) == 0) + for (excludeIdx = 0; skip[excludeIdx].name != NULL; excludeIdx++) + { + int cmplen = strlen(skip[excludeIdx].name); + + if (!skip[excludeIdx].match_prefix) + cmplen++; + if (strncmp(skip[excludeIdx].name, fn, cmplen) == 0) return true; + } return false; }