On Wed, Jun 24, 2015 at 3:43 PM, Michael Paquier wrote:
>> I just realized another problem: We recently learned the hard way that some
>> people have files in the data directory that are not writeable by the
>> 'postgres' user
>> (http://www.postgresql.org/message-id/20150523172627.ga24...@msg.df7cb.de).
>> pg_rewind will try to overwrite all files it doesn't recognize as relation
>> files, so it's going to fail on those. A straightforward fix would be to
>> first open the destination file in read-only mode, and compare its contents,
>> and only open the file in write mode if it has changed. It would still fail
>> when the files really differ, but I think that's acceptable.
>
> If I am missing nothing, two code paths need to be patched here:
> copy_file_range and receiveFileChunks. copy_file_range is
> straight-forward. Now wouldn't it be better to write the contents into
> a temporary file, compare their content, and then switch if necessary
> for receiveFileChunks?
After sleeping on it, I have been looking at this issue again and came
up with the patch attached. Instead of checking if the content of the
target and the source file are the same, meaning that we would still
need to fetch chunk content from the server in stream mode, I think
that it is more robust to check if the target file can be opened and
check for EACCES on failure, bypassing it if process does not have
permissions on it. the patch contains a test case as well, and is
independent on the rest sent upthread.
Thoughts?
--
Michael
diff --git a/src/bin/pg_rewind/copy_fetch.c b/src/bin/pg_rewind/copy_fetch.c
index 224fad1..ef830ac 100644
--- a/src/bin/pg_rewind/copy_fetch.c
+++ b/src/bin/pg_rewind/copy_fetch.c
@@ -174,7 +174,8 @@ 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))
+ return;
while (end - begin > 0)
{
diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c
index c2d8aa1..1f68855 100644
--- a/src/bin/pg_rewind/file_ops.c
+++ b/src/bin/pg_rewind/file_ops.c
@@ -40,17 +40,17 @@ 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.
*/
-void
+bool
open_target_file(const char *path, bool trunc)
{
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();
@@ -60,9 +60,19 @@ open_target_file(const char *path, bool trunc)
if (trunc)
mode |= O_TRUNC;
dstfd = open(dstpath, mode, 0600);
+
+ /* ignore errors for unreadable files */
+ if (dstfd < 0 && errno == EACCES)
+ {
+ pg_log(PG_PROGRESS, "could not open target file \"%s\", bypassing: %s\n",
+ dstpath, strerror(errno));
+ 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.h
index f68c71d..e437cb1 100644
--- a/src/bin/pg_rewind/file_ops.h
+++ b/src/bin/pg_rewind/file_ops.h
@@ -12,7 +12,7 @@
#include "filemap.h"
-extern void open_target_file(const char *path, bool trunc);
+extern bool open_target_file(const char *path, bool trunc);
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/libpq_fetch.c b/src/bin/pg_rewind/libpq_fetch.c
index df71069..41c096b 100644
--- a/src/bin/pg_rewind/libpq_fetch.c
+++ b/src/bin/pg_rewind/libpq_fetch.c
@@ -283,7 +283,8 @@ receiveFileChunks(const char *sql)
pg_log(PG_DEBUG, "received chunk for file \"%s\", offset %d, size %d\n",
filename, chunkoff, chunksize);
- open_target_file(filename, false);
+ if (!open_target_file(filename, false))
+ continue;
write_target_range(chunk, chunkoff, chunksize);
@@ -406,8 +407,8 @@ libpq_executeFileMap(filemap_t *map)
case FILE_ACTION_COPY:
/* Truncate the old file out of the way, if any */
- open_target_file(entry->path, true);
- fetch_file_range(entry->path, 0, entry->newsize);
+ if (open_target_file(entry->path, true))
+ fetch_file_range(entry->path, 0, entry->newsize);
break;
case FILE_ACTION_TRUNCATE:
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 032301f..77dddbe 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -496,7 +496,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 */
+ if (!open_target_file("backup_label", true)) /* BACKUP_LABEL_FILE */
+ pg_fatal("failed update of backup_label\n");
write_target_range(buf, 0, len);
}
@@ -557,7 +558,8 @@ updateControlFile(ControlFileData *ControlFile)
memset(buffer, 0, PG_CONTROL_SIZE);
memcpy(buffer, ControlFile, sizeof(ControlFileData));
- open_target_file("global/pg_control", false);
+ if (!open_target_file("global/pg_control", false))
+ pg_fatal("failed update of global/pg_control");
write_target_range(buffer, 0, PG_CONTROL_SIZE);
diff --git a/src/bin/pg_rewind/t/003_extrafiles.pl b/src/bin/pg_rewind/t/003_extrafiles.pl
index 9a95268..95b42de 100644
--- a/src/bin/pg_rewind/t/003_extrafiles.pl
+++ b/src/bin/pg_rewind/t/003_extrafiles.pl
@@ -51,6 +51,17 @@ sub run_test
"$test_master_datadir/tst_master_dir/master_subdir/master_file3",
"in master3";
+ # Create a read-only file on both nodes
+ append_to_file
+ "$test_standby_datadir/read_only_file",
+ "read-only file in both4";
+ chmod 0400, "$test_standby_datadir/read_only_file";
+ append_to_file
+ "$test_master_datadir/read_only_file",
+ "read-only file in both4";
+ chmod 0400, "$test_standby_datadir/read_only_file";
+ chmod 0400, "$test_master_datadir/read_only_file";
+
RewindTest::promote_standby();
RewindTest::run_pg_rewind($test_mode);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
