From 194d38a967263171ad0d11f98dcad9bf21f50f21 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 5 Mar 2018 16:44:13 +0900 Subject: [PATCH] Improve error handling of configuration files in pg_rewind In a couple of Linux distributions like Debian and Ubuntu, the data folder can include links to files which are read-only for the user managing the PostgreSQL instance. When using pg_rewind, this can lead to unwanted corruption of data folders as a failure in writing a file causes a follow-up write to the control file to fail and making it to be truncated. Relation files which are unreadable should still result in a failure, as those are critical for the state of the cluster. However make the handling of other files smoother by filtering out of the rewind process files which are found on the source and target server, but are not readable on the target as the handling of such configuration files is likely to map between the source and the target. Bug report from Christian H. Discussion: https://postgr.es/m/20180104200633.17004.16377%40wrigleys.postgresql.org --- src/bin/pg_rewind/copy_fetch.c | 21 ++++++++++++----- src/bin/pg_rewind/file_ops.c | 32 ++++++++++++++++++++++---- src/bin/pg_rewind/file_ops.h | 3 ++- src/bin/pg_rewind/filemap.c | 15 ++++++++---- src/bin/pg_rewind/filemap.h | 5 +++- src/bin/pg_rewind/libpq_fetch.c | 30 ++++++++++++++++++++---- src/bin/pg_rewind/pg_rewind.c | 5 ++-- src/bin/pg_rewind/t/006_readonly.pl | 46 +++++++++++++++++++++++++++++++++++++ 8 files changed, 133 insertions(+), 24 deletions(-) create mode 100644 src/bin/pg_rewind/t/006_readonly.pl diff --git a/src/bin/pg_rewind/copy_fetch.c b/src/bin/pg_rewind/copy_fetch.c index 04db409675..2696972421 100644 --- a/src/bin/pg_rewind/copy_fetch.c +++ b/src/bin/pg_rewind/copy_fetch.c @@ -154,9 +154,11 @@ recurse_dir(const char *datadir, const char *parentpath, * Copy a file from source to target, between 'begin' and 'end' offsets. * * If 'trunc' is true, any existing file with the same name is truncated. + * If 'error_ok' is true, any failures when opening the file is ignored. */ static void -rewind_copy_file_range(const char *path, off_t begin, off_t end, bool trunc) +rewind_copy_file_range(const char *path, off_t begin, off_t end, + bool trunc, bool error_ok) { char buf[BLCKSZ]; char srcpath[MAXPGPATH]; @@ -172,7 +174,8 @@ rewind_copy_file_range(const char *path, off_t begin, off_t end, bool trunc) if (lseek(srcfd, begin, SEEK_SET) == -1) pg_fatal("could not seek in source file: %s\n", strerror(errno)); - open_target_file(path, trunc); + if (!open_target_file(path, trunc, error_ok ? PG_WARNING : PG_FATAL)) + return; while (end - begin > 0) { @@ -221,8 +224,14 @@ copy_executeFileMap(filemap_t *map) /* ok, do nothing.. */ break; - case FILE_ACTION_COPY: - rewind_copy_file_range(entry->path, 0, entry->newsize, true); + case FILE_ACTION_COPY_CONFIG: + rewind_copy_file_range(entry->path, 0, entry->newsize, true, + true); + break; + + case FILE_ACTION_COPY_DATA: + rewind_copy_file_range(entry->path, 0, entry->newsize, true, + false); break; case FILE_ACTION_TRUNCATE: @@ -231,7 +240,7 @@ copy_executeFileMap(filemap_t *map) case FILE_ACTION_COPY_TAIL: rewind_copy_file_range(entry->path, entry->oldsize, - entry->newsize, false); + entry->newsize, false, false); break; case FILE_ACTION_CREATE: @@ -258,7 +267,7 @@ execute_pagemap(datapagemap_t *pagemap, const char *path) while (datapagemap_next(iter, &blkno)) { offset = blkno * BLCKSZ; - rewind_copy_file_range(path, offset, offset + BLCKSZ, false); + rewind_copy_file_range(path, offset, offset + BLCKSZ, false, false); /* Ok, this block has now been copied from new data dir to old */ } pg_free(iter); diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c index 705383d184..e293c9b86d 100644 --- a/src/bin/pg_rewind/file_ops.c +++ b/src/bin/pg_rewind/file_ops.c @@ -37,19 +37,21 @@ static void remove_target_symlink(const char *path); /* * Open a target file for writing. If 'trunc' is true and the file already - * exists, it will be truncated. + * exists, it will be truncated. 'elevel' can be used to mark as non-fatal + * errors happening on non-readable files. Returns true if the file has been + * successfully opened, and false otherwise. */ -void -open_target_file(const char *path, bool trunc) +bool +open_target_file(const char *path, bool trunc, int elevel) { int mode; if (dry_run) - return; + return true; if (dstfd != -1 && !trunc && strcmp(path, &dstpath[strlen(datadir_target) + 1]) == 0) - return; /* already open */ + return true; /* already open */ close_target_file(); @@ -59,9 +61,29 @@ open_target_file(const char *path, bool trunc) if (trunc) mode |= O_TRUNC; dstfd = open(dstpath, mode, 0600); + if (dstfd < 0) + { + if (errno == EACCES) + { + /* + * Ignore errors trying to open unreadable files if caller + * to do so. + */ + pg_log(elevel, "could not open target file \"%s\": %s\n", + dstpath, strerror(errno)); + + /* Reset fd for successive callers */ + dstfd = -1; + return false; + } + + /* Fallback to fatal error in other cases */ pg_fatal("could not open target file \"%s\": %s\n", dstpath, strerror(errno)); + } + + return true; } /* diff --git a/src/bin/pg_rewind/file_ops.h b/src/bin/pg_rewind/file_ops.h index be580ee4db..17d0952e5f 100644 --- a/src/bin/pg_rewind/file_ops.h +++ b/src/bin/pg_rewind/file_ops.h @@ -12,7 +12,8 @@ #include "filemap.h" -extern void open_target_file(const char *path, bool trunc); +extern bool open_target_file(const char *path, bool trunc, int elevel); +extern bool is_target_file(const char *path); extern void write_target_range(char *buf, off_t begin, size_t size); extern void close_target_file(void); extern void truncate_target_file(const char *path, off_t newsize); diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c index 19da1ee7e0..fe273864b2 100644 --- a/src/bin/pg_rewind/filemap.c +++ b/src/bin/pg_rewind/filemap.c @@ -176,7 +176,8 @@ process_source_file(const char *path, file_type_t type, size_t newsize, } else { - action = FILE_ACTION_COPY; + action = isRelDataFile(path) ? FILE_ACTION_COPY_DATA : + FILE_ACTION_COPY_CONFIG; oldsize = 0; } } @@ -392,7 +393,8 @@ process_block_change(ForkNumber forknum, RelFileNode rnode, BlockNumber blkno) datapagemap_add(&entry->pagemap, blkno_inseg); break; - case FILE_ACTION_COPY: + case FILE_ACTION_COPY_DATA: + case FILE_ACTION_COPY_CONFIG: case FILE_ACTION_REMOVE: break; @@ -456,8 +458,10 @@ action_to_str(file_action_t action) { case FILE_ACTION_NONE: return "NONE"; - case FILE_ACTION_COPY: - return "COPY"; + case FILE_ACTION_COPY_DATA: + return "COPY_DATA"; + case FILE_ACTION_COPY_CONFIG: + return "COPY_CONFIG"; case FILE_ACTION_TRUNCATE: return "TRUNCATE"; case FILE_ACTION_COPY_TAIL: @@ -494,7 +498,8 @@ calculate_totals(void) map->total_size += entry->newsize; - if (entry->action == FILE_ACTION_COPY) + if (entry->action == FILE_ACTION_COPY_CONFIG || + entry->action == FILE_ACTION_COPY_DATA) { map->fetch_size += entry->newsize; continue; diff --git a/src/bin/pg_rewind/filemap.h b/src/bin/pg_rewind/filemap.h index 63ba32a6cd..28e48762ef 100644 --- a/src/bin/pg_rewind/filemap.h +++ b/src/bin/pg_rewind/filemap.h @@ -24,7 +24,10 @@ typedef enum { FILE_ACTION_CREATE, /* create local directory or symbolic link */ - FILE_ACTION_COPY, /* copy whole file, overwriting if exists */ + FILE_ACTION_COPY_CONFIG, /* copy whole configuration file, overwriting + * if exists */ + FILE_ACTION_COPY_DATA, /* copy whole relation file, overwriting if + * exists */ FILE_ACTION_COPY_TAIL, /* copy tail from 'oldsize' to 'newsize' */ FILE_ACTION_NONE, /* no action (we might still copy modified * blocks based on the parsed WAL) */ diff --git a/src/bin/pg_rewind/libpq_fetch.c b/src/bin/pg_rewind/libpq_fetch.c index 8f8d504455..bef5736256 100644 --- a/src/bin/pg_rewind/libpq_fetch.c +++ b/src/bin/pg_rewind/libpq_fetch.c @@ -333,7 +333,11 @@ receiveFileChunks(const char *sql) pg_log(PG_DEBUG, "received chunk for file \"%s\", offset %s, size %d\n", filename, chunkoff_str, chunksize); - open_target_file(filename, false); + /* + * File chunks are present for data files, so consider any failures + * as fatal. + */ + open_target_file(filename, false, PG_FATAL); write_target_range(chunk, chunkoff, chunksize); @@ -459,9 +463,27 @@ libpq_executeFileMap(filemap_t *map) /* nothing else to do */ break; - case FILE_ACTION_COPY: - /* Truncate the old file out of the way, if any */ - open_target_file(entry->path, true); + case FILE_ACTION_COPY_CONFIG: + /* + * Truncate the old file out of the way, if any. Any + * failure in opening the file is not critical here. We + * are dealing with a configuration file here, so + * ignore any failures if the file is unreadable in the + * target. + */ + if (!open_target_file(entry->path, true, PG_WARNING)) + break; + fetch_file_range(entry->path, 0, entry->newsize); + break; + + case FILE_ACTION_COPY_DATA: + /* + * Truncate the old file out of the way, if any. We + * are dealing with a relation file which normally + * exists on the source but not on the target, so + * consider any failures as fatal. + */ + open_target_file(entry->path, true, PG_FATAL); fetch_file_range(entry->path, 0, entry->newsize); break; diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c index 72ab2f8d5e..0997062d86 100644 --- a/src/bin/pg_rewind/pg_rewind.c +++ b/src/bin/pg_rewind/pg_rewind.c @@ -597,7 +597,8 @@ createBackupLabel(XLogRecPtr startpoint, TimeLineID starttli, XLogRecPtr checkpo pg_fatal("backup label buffer too small\n"); /* shouldn't happen */ /* TODO: move old file out of the way, if any. */ - open_target_file("backup_label", true); /* BACKUP_LABEL_FILE */ + open_target_file("backup_label", true, PG_FATAL); + write_target_range(buf, 0, len); close_target_file(); } @@ -675,7 +676,7 @@ updateControlFile(ControlFileData *ControlFile) memset(buffer, 0, PG_CONTROL_FILE_SIZE); memcpy(buffer, ControlFile, sizeof(ControlFileData)); - open_target_file("global/pg_control", false); + open_target_file("global/pg_control", false, PG_FATAL); write_target_range(buffer, 0, PG_CONTROL_FILE_SIZE); diff --git a/src/bin/pg_rewind/t/006_readonly.pl b/src/bin/pg_rewind/t/006_readonly.pl new file mode 100644 index 0000000000..7c0af7baf4 --- /dev/null +++ b/src/bin/pg_rewind/t/006_readonly.pl @@ -0,0 +1,46 @@ +# Test how pg_rewind reacts to read-only files in the data dirs. +# All such files should be ignored in the process. + +use strict; +use warnings; +use TestLib; +use Test::More tests => 2; + +use File::Find; + +use RewindTest; + + +sub run_test +{ + my $test_mode = shift; + + RewindTest::setup_cluster($test_mode); + RewindTest::start_master(); + RewindTest::create_standby($test_mode); + + # Create the same read-only file in standby and master + my $test_master_datadir = $node_master->data_dir; + my $test_standby_datadir = $node_standby->data_dir; + my $first_readonly_master = "$test_master_datadir/aaa_readonly_file"; + my $first_readonly_standby = "$test_standby_datadir/aaa_readonly_file"; + my $last_readonly_master = "$test_master_datadir/zzz_readonly_file"; + my $last_readonly_standby = "$test_standby_datadir/zzz_readonly_file"; + + append_to_file($first_readonly_master, "in master"); + append_to_file($first_readonly_standby, "in standby"); + append_to_file($last_readonly_master, "in master"); + append_to_file($last_readonly_standby, "in standby"); + chmod 0400, $first_readonly_master, $first_readonly_standby; + chmod 0400, $last_readonly_master, $last_readonly_standby; + + RewindTest::promote_standby(); + RewindTest::run_pg_rewind($test_mode); + RewindTest::clean_rewind_test(); +} + +# Run the test in both modes. +run_test('local'); +run_test('remote'); + +exit(0); -- 2.16.2