Date: Mon, 15 Jan 2018 16:25:37 +0900
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
Message-ID: <20180115072537.GC1724@paquier.xyz>
References: <20180104200633.17004.16377@wrigleys.postgresql.org>
 <CAB7nPqTyq3cjphJmoCPc0n8r90O4j1BpZC2BMaj8dKZ7g0-mtw@mail.gmail.com>
 <0D104D1E-6103-49F4-8AB8-B5CD92949E18@tiptop-labs.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="maH1Gajj2nflutpK"
Content-Disposition: inline
In-Reply-To: <0D104D1E-6103-49F4-8AB8-B5CD92949E18@tiptop-labs.com>
Precedence: bulk


--maH1Gajj2nflutpK
Content-Type: multipart/mixed; boundary="3Pql8miugIZX0722"
Content-Disposition: inline


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

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.
>>=20
>> 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.
>=20
> 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=20
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

--3Pql8miugIZX0722
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="rewind-readonly-fix-v1.patch"
Content-Transfer-Encoding: quoted-printable

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..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);
=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,22 @@ open_target_file(const char *path, bool trunc)
 	if (trunc)
 		mode |=3D O_TRUNC;
 	dstfd =3D open(dstpath, mode, 0600);
+
+	if (errno =3D=3D 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 =3D 0;
+		return false;
+	}
+
 	if (dstfd < 0)
 		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 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);
=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
@@ -452,9 +456,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 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;
=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..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 =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);

--3Pql8miugIZX0722--

--maH1Gajj2nflutpK
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCAAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlpcV3EACgkQnvQgOdby
QH0fHRAAh6y9KXJNSH4Z9G51QWNHyqaLxn9calCWGLbVv0I2MyzMdBpCtycsbZLJ
WIdu0/DHoqVo32t/klyrZKkBNqo/tOdXlfyUhrl7oDQZ5kt70UGGEmiPOjcNv70J
HYH/sTI/tZn1FIbihfX59963dGymVN40VvO53cDi1t/ul4/+Qk+4OW51LdO6wT2G
+nELg8HlWiUYqmGRaQMgnzFFnvDIE5TKj/ULbHKJ1DRsjC3TxADBr/EctqQNLfoM
Y3hoBoEO+MEFZW0JthhJbcZjeQwJrE1smHGezLh4Xfyg7wPHD0LbFnqnGipUAN6N
+BQo1fraDaCjU5I2r6F5IgsWtjedvSJFTDQxIUJKbTFIflr+OS/XcofSgIf6b94j
ICVP0GbgHDMTmT29dV6bzbXbsYKL8slpURbYafSMYOD/4liVO51kpmuZmN/QnA5Z
rce/sZueEQdSr0/owT++rg97dGvOxHK/DqsEtrVxJooQ84xFoRWuHAaVAbU7ZVhX
nZhZVW7npFljMUJiqeQFBgx+kxSA5a5heE9NmNnH7nNQOlFuvssCjEBkciol1nxB
IqQe24hoXPHMAI0mZoG9zIhjsntFUfaTySrderDxa65Z/zYkrPpbihVUNpxC4ONT
XXoCLFLFfGEaOKY5c26IhcDsCVtOzsftyOEfb9DCMseEpadCyDE=
=THEh
-----END PGP SIGNATURE-----

--maH1Gajj2nflutpK--