From: Michael Paquier <michael@paquier.xyz>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: TipTop Labs <office@tiptop-labs.com>
Cc: Michael Paquier <michael.paquier@gmail.com>
Cc: PostgreSQL mailing lists <pgsql-bugs@postgresql.org>
Subject: Re: BUG #14999: pg_rewind corrupts control file global/pg_control
Date: Mon, 5 Mar 2018 16:54:31 +0900
Message-ID: <20180305075431.GH2266@paquier.xyz> (raw)
In-Reply-To: <CA+q6zcWeFmGc=uC5CwL7HznL25nCqh0CvepgtyuGQo1duStE+w@mail.gmail.com>
References: <20180104200633.17004.16377@wrigleys.postgresql.org>
<CAB7nPqTyq3cjphJmoCPc0n8r90O4j1BpZC2BMaj8dKZ7g0-mtw@mail.gmail.com>
<0D104D1E-6103-49F4-8AB8-B5CD92949E18@tiptop-labs.com>
<20180115072537.GC1724@paquier.xyz>
<D2684F73-AE17-453B-ACB5-9CF0A7F6856E@tiptop-labs.com>
<20180227012441.GB3255@paquier.xyz>
<CA+q6zcWeFmGc=uC5CwL7HznL25nCqh0CvepgtyuGQo1duStE+w@mail.gmail.com>
On Fri, Mar 02, 2018 at 04:35:07PM +0100, Dmitry Dolgov wrote:
> It maybe a stupid question, but why do you need to reset errno here? Is it to
> avoid its value being carried from previous calls of `open_target_file`? In
> this case if you put the code with `errno == EACCESS` under the if condition
> `if (dstfd < 0)`, then as far as I remember you should always get relevant
> errno. At the same time maybe it's valid to reset `errno` before `open`, like
> with `getpriority`:
[ ... reads and feels stupid ...]
Of course all the checks should be where dstno is negative...
I have done a second pass on the patch, and attached is a new version.
This fixes this handling of errno and addresses some typos. I have also
fixed the test case where one of the read-only files was not properly
switched to do so. I have also added a commit log message.
--
Michael
Attachments:0001-Improve-error-handling-of-configuration-files-in-pg_.patch (text/x-diff, 12.1K) download
From 194d38a967263171ad0d11f98dcad9bf21f50f21 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
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.cindex 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.cindex 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.hindex 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.cindex 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.hindex 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.cindex 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.cindex 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.plnew file mode 100644index 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
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, 9erthalion6@gmail.com, office@tiptop-labs.com, michael.paquier@gmail.com
Subject: Re: BUG #14999: pg_rewind corrupts control file global/pg_control
In-Reply-To: <20180305075431.GH2266@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox