From: Michael Paquier <michael.paquier@gmail.com>
To: TipTop Labs <office@tiptop-labs.com>
Cc: PostgreSQL mailing lists <pgsql-bugs@postgresql.org>
Subject: Re: BUG #14999: pg_rewind corrupts control file global/pg_control
Date: Mon, 15 Jan 2018 16:25:37 +0900
Message-ID: <20180115072537.GC1724@paquier.xyz> (raw)
In-Reply-To: <0D104D1E-6103-49F4-8AB8-B5CD92949E18@tiptop-labs.com>
References: <20180104200633.17004.16377@wrigleys.postgresql.org>
<CAB7nPqTyq3cjphJmoCPc0n8r90O4j1BpZC2BMaj8dKZ7g0-mtw@mail.gmail.com>
<0D104D1E-6103-49F4-8AB8-B5CD92949E18@tiptop-labs.com>
On Fri, Jan 05, 2018 at 04:36:22AM +0100, TipTop Labs wrote:
>> On Jan 5, 2018, at 2:26 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
>> On Fri, Jan 5, 2018 at 5:06 AM, PG Bug reporting form
>> <noreply@postgresql.org> wrote:
>>> I have encountered a bug in PostgreSQL 10.1: when the target directory for
>>> pg_rewind contains a read-only file (e.g. server.key), pg_rewind exits with
>>> "could not open target file" (legitimate) and corrupts the control file
>>> global/pg_control to size 0 (bug). From now on, pg_rewind always exits with
>>> "unexpected control file size 0, expected 8192" and a restore from
>>> pg_basebackup is needed.
>>
>> Likely that's reproducible down to 9.5 where pg_rewind has been
>> introduced. I agree that we should do better with failure handling
>> here. Corrupting the control file is not cool.
>
> I can already confirm that this also occurs with PostgreSQL 9.6.
As far as I can see, this happens because of the way the 'remote' mode
of pg_rewind considers a set of files to truncate or not. Attached is a
patch which does things in a more correct way. The key point here is to
make the difference between a relation file and a configuration file
when building the filemap for copying a file in full. When a
configuration file is not readable, then trying to open it should not be
a failure. When using a relation file, there should be failures. There
are still two things I am not completely happy about:
- pg_control is considered as a configuration file with this patch,
however we should fail it pg_control is not readable. In practice I
guess that this should not happen, and the patch produces a warning
message. I think that we should consider add a special handling for
things like pg_control, filenode.map. etc. so as you get a hard failure
if those are not readable, so they should enter in the category of
FILE_ACTION_COPY_DATA. This needs a bit more thoughts in
process_source_file().
- open_target_file() resets manually errno. This is necessary as this
gets called continuously when processing the same file in remote mode. I
tried as well to make open_target_file and close_target_file one-time
operations for each file, but the result was even more ugly, so I went
up with this solution.
I am adding that to the next CF to not forget about it. This approach
is back-patchable down to 9.5 in my opinion. I have added as well a TAP
test in the patch which is able to reproduce the problem.
Thoughts?
--
Michael
Attachments:rewind-readonly-fix-v1.patch (text/x-diff, 10.3K) downloaddiff --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..290a4a1c5b 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,22 @@ open_target_file(const char *path, bool trunc)
if (trunc)
mode |= O_TRUNC;
dstfd = open(dstpath, mode, 0600);
++ if (errno == EACCES)+ {+ /* Ignore errors trying to open unreadable files */+ pg_log(elevel, "could not open target file \"%s\": %s\n",+ dstpath, strerror(errno));++ /* Reset stack for successive callers */+ errno = 0;+ return false;+ }+
if (dstfd < 0)
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 d1726d1c74..c015603a93 100644--- a/src/bin/pg_rewind/libpq_fetch.c+++ b/src/bin/pg_rewind/libpq_fetch.c@@ -326,7 +326,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);
@@ -452,9 +456,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 files 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..5807110dad--- /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);
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@gmail.com, office@tiptop-labs.com
Subject: Re: BUG #14999: pg_rewind corrupts control file global/pg_control
In-Reply-To: <20180115072537.GC1724@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