Date: Wed, 4 Apr 2018 16:26:25 +0900
From: Michael Paquier <michael@paquier.xyz>
To: TipTop Labs <office@tiptop-labs.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Dmitry Dolgov <9erthalion6@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.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: <20180404072625.GB2208@paquier.xyz>
References: <20180305075431.GH2266@paquier.xyz>
 <CAD21AoDiHUUguQ7UUyK_Z4icHM3OiP8OEkpgDCsT3fg3B-P2LA@mail.gmail.com>
 <20180305090111.GI2266@paquier.xyz>
 <CAD21AoC3zSRrCdn6SBh4-whtXT0hCKCwjZM16uH1s1CU3DY+iQ@mail.gmail.com>
 <CA+q6zcVCfeYoAmPyW5R+uxZ0MD6=ZunsW=uP0AS=B=RxB8M7Tg@mail.gmail.com>
 <20180307014624.GC1744@paquier.xyz>
 <CA+q6zcWix34kmuB6PTgizy4-7Eq2Rn4FX3tpwnH5SdgEU7ugPw@mail.gmail.com>
 <30255.1522711675@sss.pgh.pa.us>
 <20180403024110.GB1621@paquier.xyz>
 <A89569FD-1CCB-4060-932B-B61DD0F756BF@tiptop-labs.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="LwW0XdcUbUexiWVK"
Content-Disposition: inline
In-Reply-To: <A89569FD-1CCB-4060-932B-B61DD0F756BF@tiptop-labs.com>
Precedence: bulk


--LwW0XdcUbUexiWVK
Content-Type: multipart/mixed; boundary="TRYliJ5NKNqkz5bu"
Content-Disposition: inline


--TRYliJ5NKNqkz5bu
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Apr 03, 2018 at 10:13:05PM +0200, TipTop Labs wrote:
> Truncation of the file was moved to the second loop. Truncation occurs
> there if chunks are written into files at offset 0. This is the case
> for FILE_ACTION_COPY. An additional SQL "ORDER BY path, begin" ensures
> that these chunks are processed first.

Yes.  I have spent a large portion of my day looking at that.  And
actually this is wrong as well.  First, please find attached a patch I
have written while testing your changes.  There is nothing much new into
it, except that I added more comments and a test (other tests fail as
well, that does not matter).

First, I have looked at a way to not rely on the tweak with chunkoff by
extending the temporary table used to register the chunks with an extra
column which tracks the type of action which is taken on the file.
Another one I looked at was to pass down the action type taken on the
entry directly to receiveFileChunks().  However both of them have been
grotty.

So after that I falled back to your patch and began testing it, which is
where I noticed that we can *never* give the insurance to recover a data
folder on which an error has happened in the middle of a pg_rewind.  The
reason for that is quite simple: even if the truncation has been moved
down to the moment where the first chunk of a file is received, you may
have already done work on some relation files.  Particularly, some of
them may have been truncated down to a given size without a new range of
blocks fetched from the source.  So the data folder would be in an
inconsistent state if trying to rewind it again.

If the control file from the source has been received and then a
read-only error is found, then in order to perform a subsequent rewind
you need to start and stop the target cluster cleanly, or update
manually the control file and mark it as cleanly stopped.  And this is a
recipe for data corruption as we now start a rewind based on the
guarantee that a target cluster has been able to cleanly stop, with a
shutdown checkpoint issued.  You could always try to start and stop the
cluster cleanly, but if your target instance replays WAL on data blocks
which have been truncated by a previous rewind, you would need to
re-create an instance using a new base backup, which is actually harder
to diagnose.

The patch I sent prevously which makes the difference between relation
files and "configuration" files helps a bit, still it makes me uneasy
because it will not be able to handle correctly failures on files which
are part of a relation but need to be fully copied.  So I think that we
should drop this approach as well for its complexity.

Another thing that could definitely help is to work on a patch which
checks that *all* files are writable before doing any operations for
files which are present on both the source and the target, and make the
rewind nicely fail with the source still intact.  That's quite a bit of
refactoring ahead for little benefit I think in the long run.

What we could simply do is to document the limitation.  Hence, if both
the target and the source have read-only files, then the user needs to
be sure to remove them from the target before doing the rewind, and do
the re-linking after the operation.  If an error is found while
processing the rewind, then a new base backup needs to be taken.

The refactoring I am mentioning two paragraphs above could definitely be
done to ease user's life, but I would treat that as a new feature and
not a bug.  Another approach, way more simple that we could do is to
scan the data folder of the target before doing anything and see if some
files are not writable, and then report this list back to the user.  If
we do that even for files which are on the target but not the source
then that would be a loss for some cases, however I would like to
imagine that when using pg_rewind the configuration file set is
consistent across nodes in a majority of cases, so a user could remove
those non-writable files manually.  This has also the advantage to be
non-intrusive, so a back-patch is definitely possible.

Thoughts?
--
Michael

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

diff --git a/src/bin/pg_rewind/libpq_fetch.c b/src/bin/pg_rewind/libpq_fetc=
h.c
index 5914b15017..821e4b0c81 100644
--- a/src/bin/pg_rewind/libpq_fetch.c
+++ b/src/bin/pg_rewind/libpq_fetch.c
@@ -337,7 +337,18 @@ 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);
+		/*
+		 * Truncate new files which are fully copied.  We are sure to not
+		 * truncate a file whose data has been partially fetched for two
+		 * reasons:
+		 * 1) The set of file ranges is ordered so as the chunks of each
+		 * file is processed sequentially.
+		 * 2) Only files fully copied register a chunk with an offset of
+		 * 0, which means that the beginning of a file is received, so it
+		 * should be truncated first.  Note that the truncation is lossy,
+		 * and may be tried on a file which does not exist locally.
+		 */
+		open_target_file(filename, chunkoff =3D=3D 0);
=20
 		write_target_range(chunk, chunkoff, chunksize);
=20
@@ -464,8 +475,6 @@ libpq_executeFileMap(filemap_t *map)
 				break;
=20
 			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);
 				break;
=20
@@ -501,12 +510,17 @@ libpq_executeFileMap(filemap_t *map)
=20
 	/*
 	 * We've now copied the list of file ranges that we need to fetch to the
-	 * temporary table. Now, actually fetch all of those ranges.
+	 * temporary table. Now, actually fetch all of those ranges.  The elements
+	 * fetched are ordered so as the truncation of files fully copied from the
+	 * source server is done after receiving their first chunk.  This also
+	 * gives a better sequential performance to the operations, especially
+	 * when working on large files.
 	 */
 	sql =3D
 		"SELECT path, begin,\n"
 		"  pg_read_binary_file(path, begin, len, true) AS chunk\n"
-		"FROM fetchchunks\n";
+		"FROM fetchchunks\n"
+		"ORDER BY path, begin\n";
=20
 	receiveFileChunks(sql);
 }
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..51db93a9a0
--- /dev/null
+++ b/src/bin/pg_rewind/t/006_readonly.pl
@@ -0,0 +1,85 @@
+# 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::Copy;
+use File::Find;
+
+use RewindTest;
+
+my $test_mode =3D "remote";
+
+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 $readonly_master =3D "$test_master_datadir/readonly_file";
+my $readonly_standby =3D "$test_standby_datadir/readonly_file";
+
+append_to_file($readonly_master, "in master");
+append_to_file($readonly_standby, "in standby");
+chmod 0400, $readonly_master, $readonly_standby;
+
+RewindTest::promote_standby();
+
+# Stop the master and run pg_rewind.
+$node_master->stop;
+
+my $master_pgdata   =3D $node_master->data_dir;
+my $standby_pgdata  =3D $node_standby->data_dir;
+my $standby_connstr =3D $node_standby->connstr('postgres');
+my $tmp_folder      =3D TestLib::tempdir;
+
+# Keep a temporary postgresql.conf for master node or it would be
+# overwritten during the rewind.
+copy(
+	"$master_pgdata/postgresql.conf",
+	"$tmp_folder/master-postgresql.conf.tmp");
+
+# Including read-only data in the source and the target will
+# cause pg_rewind to fail.
+my $rewind_command =3D [   'pg_rewind',       "--debug",
+		"--source-server", $standby_connstr,
+		"--target-pgdata=3D$master_pgdata" ];
+
+command_fails($rewind_command, 'pg_rewind fails with read-only');
+
+# Need to put the server back into a clean state for next rewind.
+# Move back postgresql.conf with old settings to allow the node to
+# start and stop, allowing the follow-up rewind to work properly.
+move("$tmp_folder/master-postgresql.conf.tmp",
+	 "$master_pgdata/postgresql.conf");
+$node_master->start;
+$node_master->stop;
+
+# Now remove the read-only data on both sides, the data folder
+# from the previous attempt should still be able to work.
+unlink($readonly_master);
+unlink($readonly_standby);
+command_ok($rewind_command, 'pg_rewind passes without read-only');
+
+# Now move back postgresql.conf with old settings
+move("$tmp_folder/master-postgresql.conf.tmp",
+	 "$master_pgdata/postgresql.conf");
+
+# Plug-in rewound node to the now-promoted standby node
+my $port_standby =3D $node_standby->port;
+$node_master->append_conf('recovery.conf', qq(
+primary_conninfo=3D'port=3D$port_standby'
+standby_mode=3Don
+recovery_target_timeline=3D'latest'
+));
+
+# Restart the master to check that rewind went correctly.
+$node_master->start;
+
+RewindTest::clean_rewind_test();
+
+exit(0);

--TRYliJ5NKNqkz5bu--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlrEfiEACgkQnvQgOdby
QH0Cig/7B5QOy9H3xitWwk7qM18Eqo7V+ZkJPybORlGkfLRvPcpU3SORe1f8NYf9
Glam4Xt63rKwDZaCbUjL9kztqp0wwGqTLnq5STouAiEkktZFae8gKnez7aG8OoSF
MevnkcYMlNL/7FEuscM/pRBl+E/TefknSMS7LScf5bS5CLVZo5uNJXxIAy//Fz8B
Ii4hiPshbTtltx5psJEhylcuNtF5mMpH9tvTwV9d1AS3vw6wRl1Isw43Bp+IetaL
8clHmpfc5Ws5P5F0cRJ9zKGImeyM/6jj+vO1v9+R/RC2yMlA52PVNMLGOLBvqouk
kBV54EsTntPs8WIylpUMKs7m2BhrJtRDDgJCgF/OnYOzZIpM9+YpSFdyHGJvhU7M
V6AB8aWRDod/3eIq3VBtqHJ4Hlu8Dh4/z6tKl5hgSaSQOG3xPaxBJcQW1SVu7Asq
QbmfjvmGMVUGE7G3DKI80wAv4Kgz/c9sKjLpQMXXHmKmKriafP47YxMOz9IM3LVe
bkfcbQoQxiD7sIlhV/BG96VZ8phyEdu5/TxLM6hKmOOk2tZ+SHmL4BkzXHB6wv2a
OwLb3ypeEyzMoZyUzG3rtvHeHyVtO/ISgkfmbhv+M+YnoYwvAAHqjQQEUqr1EOM3
DZkfV5tH+jyRBrPknDBG5lxBNiB21jCHiARvjMa+lMyVsW3G+uM=
=RC6L
-----END PGP SIGNATURE-----

--LwW0XdcUbUexiWVK--