Date: Mon, 5 Mar 2018 16:54:31 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: TipTop Labs <office@tiptop-labs.com>,
	Michael Paquier <michael.paquier@gmail.com>,
	PostgreSQL mailing lists <pgsql-bugs@postgresql.org>
Subject: Re: BUG #14999: pg_rewind corrupts control file global/pg_control
Message-ID: <20180305075431.GH2266@paquier.xyz>
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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="GmiNL4+5WUWrod5m"
Content-Disposition: inline
In-Reply-To: 
 <CA+q6zcWeFmGc=uC5CwL7HznL25nCqh0CvepgtyuGQo1duStE+w@mail.gmail.com>
Precedence: bulk


--GmiNL4+5WUWrod5m
Content-Type: multipart/mixed; boundary="3MMMIZFJzhAsRj/+"
Content-Disposition: inline


--3MMMIZFJzhAsRj/+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 i=
t to
> avoid its value being carried from previous calls of `open_target_file`? =
In
> this case if you put the code with `errno =3D=3D EACCESS` under the if co=
ndition
> `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.=20
--
Michael

--3MMMIZFJzhAsRj/+
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0001-Improve-error-handling-of-configuration-files-in-pg_.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 194d38a967263171ad0d11f98dcad9bf21f50f21 Mon Sep 17 00:00:00 2001
=46rom: 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.postg=
resql.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 *parentpat=
h,
  * 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 trun=
c)
+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, o=
ff_t end, bool trunc)
 	if (lseek(srcfd, begin, SEEK_SET) =3D=3D -1)
 		pg_fatal("could not seek in source file: %s\n", strerror(errno));
=20
-	open_target_file(path, trunc);
+	if (!open_target_file(path, trunc, error_ok ? PG_WARNING : PG_FATAL))
+		return;
=20
 	while (end - begin > 0)
 	{
@@ -221,8 +224,14 @@ copy_executeFileMap(filemap_t *map)
 				/* ok, do nothing.. */
 				break;
=20
-			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;
=20
 			case FILE_ACTION_TRUNCATE:
@@ -231,7 +240,7 @@ copy_executeFileMap(filemap_t *map)
=20
 			case FILE_ACTION_COPY_TAIL:
 				rewind_copy_file_range(entry->path, entry->oldsize,
-									   entry->newsize, false);
+									   entry->newsize, false, false);
 				break;
=20
 			case FILE_ACTION_CREATE:
@@ -258,7 +267,7 @@ execute_pagemap(datapagemap_t *pagemap, const char *pat=
h)
 	while (datapagemap_next(iter, &blkno))
 	{
 		offset =3D 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);
=20
 /*
  * 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 be=
en
+ * 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;
=20
 	if (dry_run)
-		return;
+		return true;
=20
 	if (dstfd !=3D -1 && !trunc &&
 		strcmp(path, &dstpath[strlen(datadir_target) + 1]) =3D=3D 0)
-		return;					/* already open */
+		return true;			/* already open */
=20
 	close_target_file();
=20
@@ -59,9 +61,29 @@ open_target_file(const char *path, bool trunc)
 	if (trunc)
 		mode |=3D O_TRUNC;
 	dstfd =3D open(dstpath, mode, 0600);
+
 	if (dstfd < 0)
+	{
+		if (errno =3D=3D 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 =3D -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;
 }
=20
 /*
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 @@
=20
 #include "filemap.h"
=20
-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 =3D FILE_ACTION_COPY;
+					action =3D isRelDataFile(path) ? FILE_ACTION_COPY_DATA :
+						FILE_ACTION_COPY_CONFIG;
 					oldsize =3D 0;
 				}
 			}
@@ -392,7 +393,8 @@ process_block_change(ForkNumber forknum, RelFileNode rn=
ode, BlockNumber blkno)
 					datapagemap_add(&entry->pagemap, blkno_inseg);
 				break;
=20
-			case FILE_ACTION_COPY:
+			case FILE_ACTION_COPY_DATA:
+			case FILE_ACTION_COPY_CONFIG:
 			case FILE_ACTION_REMOVE:
 				break;
=20
@@ -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)
=20
 		map->total_size +=3D entry->newsize;
=20
-		if (entry->action =3D=3D FILE_ACTION_COPY)
+		if (entry->action =3D=3D FILE_ACTION_COPY_CONFIG ||
+			entry->action =3D=3D FILE_ACTION_COPY_DATA)
 		{
 			map->fetch_size +=3D 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_fetc=
h.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);
=20
-		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);
=20
 		write_target_range(chunk, chunkoff, chunksize);
=20
@@ -459,9 +463,27 @@ libpq_executeFileMap(filemap_t *map)
 				/* nothing else to do */
 				break;
=20
-			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;
=20
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 sta=
rttli, XLogRecPtr checkpo
 		pg_fatal("backup label buffer too small\n");	/* shouldn't happen */
=20
 	/* 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));
=20
-	open_target_file("global/pg_control", false);
+	open_target_file("global/pg_control", false, PG_FATAL);
=20
 	write_target_range(buffer, 0, PG_CONTROL_FILE_SIZE);
=20
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 =3D> 2;
+
+use File::Find;
+
+use RewindTest;
+
+
+sub run_test
+{
+	my $test_mode =3D 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 =3D $node_master->data_dir;
+	my $test_standby_datadir =3D $node_standby->data_dir;
+	my $first_readonly_master =3D "$test_master_datadir/aaa_readonly_file";
+	my $first_readonly_standby =3D "$test_standby_datadir/aaa_readonly_file";
+	my $last_readonly_master =3D "$test_master_datadir/zzz_readonly_file";
+	my $last_readonly_standby =3D "$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);
--=20
2.16.2


--3MMMIZFJzhAsRj/+--

--GmiNL4+5WUWrod5m
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlqc97cACgkQnvQgOdby
QH2KfQ/+Pbj69nRtcHU6J24TSj8umKniBmhbr8VXaOMqcfEB34tCKS2wZ26+XXaT
064bvwdyv2xjhfBQBeMUEVxJTgOEqhBMKgI7keRvY4BQ3QW8zx8qx79UJkmljhOF
MLcKqu1a7vUIFvbtDa084NYymWrfvZDGdGLFvgceOc6wpBTuWTx7CGjJo5YZXDB9
F8B8DpW7lQA8zaC6GDsBO18dl4dpagW/qgHErOCv9f5yP9ST31nNlT9pHhSfTsAn
hR7UaVDWOqb/O0LxfL4FbM9+DW0xW0snp0HL1TQmzLhKeK64uqpqwlMkLwv+YPvb
/SFfPuMBURmYwaUaghMc3oNTJZ5P7JvtDGUBNmKL/vjxKTYtvdPLiYjpTm6nlwdU
0W/ftXp5FVSt+WT16ia+ca0Efbq5piHsvq6gcUp50Q5x46X3wML17nck7XwO9CUp
JkvH03OH8f8cwZDVAQoG6YnjzsT+kr+TijnuT505fAXJ8o+AoRhZsP0iH52rdaxl
CCBYtZizkCkjFgUrWayaUcbxUxZyL7ixGrnQCi13y212OyLNgn76spZmEmm++aUz
NWBZIRKsNp5+7PUrdL1FHzVFfbJFtPP+VZThAQtSEe8cdlLsItqS4YR0ntdqW/oV
SOCQ95Oh830xxT26yPKgIlcJOIevjwHFgrubyNdRgQnGUx0Jty8=
=znLH
-----END PGP SIGNATURE-----

--GmiNL4+5WUWrod5m--