Date: Thu, 5 Apr 2018 10:38:18 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: TipTop Labs <office@tiptop-labs.com>,
	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: <20180405013818.GC1732@paquier.xyz>
References: <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>
 <20180404072625.GB2208@paquier.xyz>
 <22961.1522867812@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="g7w8+K/95kPelPD2"
Content-Disposition: inline
In-Reply-To: <22961.1522867812@sss.pgh.pa.us>
Precedence: bulk


--g7w8+K/95kPelPD2
Content-Type: multipart/mixed; boundary="hYooF8G/hrfVAmum"
Content-Disposition: inline


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

On Wed, Apr 04, 2018 at 02:50:12PM -0400, Tom Lane wrote:
> When I first started looking at this thread, I wondered if maybe somebody
> had had in mind to create an active defense against starting a postmaster
> in an inconsistent target cluster, by dint of intentionally truncating
> pg_control before the transfer starts and not making it valid again till
> the very end.  It's now clear from looking at the code that that's not
> what's going on :-(.  But I wonder how hard it would be to make it so,
> and whether that'd be worth doing if it's not too hard.

One safeguard I can see is to add an extra loop just before processing
the file map to check if a file planned for copy can be sanely opened or
not.  That's simple to do with open_target_file(), followed by
close_target_file().  If there is an EACCES error, then we complain at
this early stage, and the data folder can be reused after cleaning up
the data folder from those files.  That's not actually hard to do at
quick glance, but I may be missing something so let's be careful.

Another approach that we could consider, HEAD-only though, is to extend
pg_stat_file so as an entry's st_mode is available and we could use that
to not copy the file's data from the start.  That would be actually
quite clean, particularly for large read-only files.

> Actually, probably a safer way to attack that would be to remove or
> rename the topmost PG_VERSION file, and then put it back afterwards.
> That'd be far easier to recover from manually, if need be, than
> clobbering pg_control.
>=20
> In any case, that seems separate from the question of what to do with
> read-only files in the data directory.  Should we push forward with
> committing Michael's previous patch, and leave that issue for later?

That one is not good either as long as we don't make the difference in
the data folder between three types of files:
1) Relation files.
2) System files which are critical at diverse degrees for the system.
3) Custom configuration files.

pg_rewind is able to consider 1) as a separate category as it usually
needs to fetch a range set of blocks, and puts 2) and 3) in the same
bucket.  It could be possible to split 2) and 3) but the maintenance
cost is high as for each new system file added in Postgres we would need
to maintain an extra filtering logic in pg_rewind, something which is
most likely going to rot, leading to potential corruption problems.

My point is that I have seen on some VMware test beds a kernel going
rogue because of ESX servers, causing a PostgreSQL-related partition to
go in read-only mode.  So if you use my previous patch, and that the
partition where the target server's data folder is located turns
all-of-a-sudden to read-only, there is a risk of silencing real
failures.  Even if the fetched data is ordered, paths like pg_xact and=20
pg_twophase are processed after all relation files, so failing to write
them correctly would silently break a set of transactions :(

So please let me suggest a set of two patches:
1) 0001 is a documentation patch which should be back-patched.  This
tells two things:
1-1) If pg_rewind fails in the middle of processing, then the best thing
to do is to re-create the data folder using a new fresh backup.
1-2) Be careful of files that the process running pg_rewind is not able
to write to.  If those are located on both the target and the source,
then pg_rewind will copy it from the source to the target, leading to an
immediate failure.  After removing those unwritable files, be sure to
clean up anything which has been copied from the source and should be
read-only, then rebuild the links.
2) 0002 is a patch for HEAD to order the set of chunks fetched, because
this makes the data writes sequentials which is good for performance and
limits the number of open()/close() on the files of the target's data
folder.

I think that this is the best answer we can give now as there are a
couple of ways to improving the handling of any checks, however there
are side effects to any of them.  This also feels a bit like a new
feature, while the documentation in pg_rewind sucks now for such
details.  I have also spent some time detailing the commit message of
the first patch about all that, that would be nice to keep in the git
history.

If the extra checks I am mentioning at the top of this email are worth
adding, then it is possible to drop 1-2) partially, rewriting it to
mention that pg_rewind tells the user at early stages about the failure
and that a data folder can be again reused.  This does not change the
fact that a pg_rewind breaking mid-flight should result in a new base
backup, so documentation is mandatory anyway.  And there are a couple of
approaches that we could consider here.  At least I saw two of them.
Other folks may see better approaches than I did, who knows..

Thanks,
--
Michael

--hYooF8G/hrfVAmum
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0001-Add-note-in-pg_rewind-documentation-about-read-only-.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 4b2718d09d41c9ea8445e467e276947bffd0cdf1 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Thu, 5 Apr 2018 10:11:34 +0900
Subject: [PATCH 1/2] Add note in pg_rewind documentation about read-only fi=
les

When performing pg_rewind, the presence of a read-only file which is not
accessible for writes will cause a failure while processing.  This can
cause the control file of the target data folder to be truncated,
causing it to not be reusable with a successive run.

We have discussed on the problem's thread a couple of ways to deal with
the problem:
1) Consider EACCES failures on relation files as critical not not on
non-relation files, which goes down to custom configuration files as
well as FSM or VM files.  Being able to make the difference between
custom files and the ones critical for the system requires additional
maintenance with the addition of new filtering rules, which is costly in
the long term.
2) Order the fetched block ranges and delay the truncation of a file
only when its first range chunk is received.  That's actually not
reliable either, as when facing a failure some of the relation files may
have been already manipulated.  If by chance one if able to start and
stop the target's server after a failed rewind, they have good chances
to have already a broken instance.

There are some solutions which could be considered:
1) Add pre-checks making sure that a set of files which are going to be
processed for copy can be sanely written to, and complain about it
before writing any data on the target's data folder.  This has the
disadvantage that a user would still need to rebuild the links used for
what was previously a set of read-only files.
2) Prevent the data of read-only files to be fetched from the source,
which would save some bandwidth, but requires the backend's
pg_stat_file() to be extended with an entry's st_mode.  This can only
happen on HEAD.
3) Allow callers to specify custom exclusion rules.

1) is the one causing the less code churn, still it is not clear to me
if any of those are worth considering anyway as read-only files would
need to be most likely-rebuilt after a rewind.  Hence the most simple
solution is to just document the behavior and tell users to not do
that.  We could always consider new solutions in future releases to ease
the handling of such files, but that may not be worth it.

Also, when pg_rewind fails midflight, there is likely no way to be able
to recover the target data folder anyway, in which case a new base
backup is the best option.  A note is added in the documentation as well.

Discussion: https://postgr.es/m/20180104200633.17004.16377%40wrigleys.postg=
resql.org
---
 doc/src/sgml/ref/pg_rewind.sgml | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.s=
gml
index 520d843f0e..ee35ce18b0 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -95,6 +95,26 @@ PostgreSQL documentation
    are currently on by default.  <xref linkend=3D"guc-full-page-writes"/>
    must also be set to <literal>on</literal>, but is enabled by default.
   </para>
+
+  <warning>
+   <para>
+    If <application>pg_rewind</application> fails while processing, then
+    the data folder of the target is likely not in a state that can be
+    recovered.  In such a case, taking a new fresh backup is recommended.
+   </para>
+
+   <para>
+    <application>pg_rewind</application> will fail immediately if it finds
+    files it cannot write directly to.  This can happen for example when
+    the source and the target server use the same file mapping for read-on=
ly
+    SSL keys and certificates.  If such files are present on the target se=
rver
+    it is recommended to remove them before running
+    <application>pg_rewind</application>.  After doing the rewind, some of
+    those files may have been copied from the source, in which case it may
+    be necessary to remove the data copied and restore back the set of lin=
ks
+    used before the rewind.
+   </para>
+  </warning>
  </refsect1>
=20
  <refsect1>
--=20
2.16.3


--hYooF8G/hrfVAmum
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0002-Enforce-ordering-of-data-chunks-fetched-in-pg_rewind.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 812ced024a7edea7dda1634fe926936c20a22f93 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Thu, 5 Apr 2018 10:31:53 +0900
Subject: [PATCH 2/2] Enforce ordering of data chunks fetched in pg_rewind f=
or
 libpq transfer

This is good for performance, as this gives a sequential behavior to the
rewind operation, and also limits the number of files opened and closed
when applying data chunks.

Author: Christian H.
Author: Michael Paquier

Discussion: https://postgr.es/m/20180104200633.17004.16377%40wrigleys.postg=
resql.org
---
 src/bin/pg_rewind/libpq_fetch.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_rewind/libpq_fetch.c b/src/bin/pg_rewind/libpq_fetc=
h.c
index 5914b15017..880c6553ea 100644
--- a/src/bin/pg_rewind/libpq_fetch.c
+++ b/src/bin/pg_rewind/libpq_fetch.c
@@ -501,12 +501,18 @@ 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 to ensure that the set of chunks applying to one
+	 * are processed successively.  This gives a sequential behavior to the
+	 * rewind, which is good for performance especially on large files, and
+	 * limites the back-and-forth movements to open and close multiple times
+	 * the same file.
 	 */
 	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);
 }
--=20
2.16.3


--hYooF8G/hrfVAmum--

--g7w8+K/95kPelPD2
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlrFfgoACgkQnvQgOdby
QH2AjA//dcDZh/R3nPU3Wr2bVJd1BPA23aGpGbnmJzpMC5Xnbs4Rovq16EdnWbjE
P47JYIcuARxUG5Z2nVJSNvqb0/d13cvXb1mgRnr5djqiv7JXLlzIWfXjHa8AMWD6
j5My62W1LERIjE2zSrSZqAg/JqxL030WWeDykPGkCSsF5mNiSf4SQBRwf3OiNVu3
cPeSdetoqwf0D9R+oKAbgk1Pw+5OJMGaMhA8I8LrYKSOCjciJ5QhmRofrEKme+Gy
GirKkLChjwpDswxRJv5kRCgGkEJcY3rLeryMHd5OHgbs+e6K7uHSFD/HZp1plALN
J3B1vlCa134RzzkVU6yrJ/bFTyAcM84Df+yy7UAQrDtzxa2iUuXe+sIHFyLf+EFi
WN5fHd6BWmUk2RR3Ti2GX4FOCyy+qLREHNGWpFnS1pmiEHRYb+0B2BqkkcOghC6j
EhDKJ2Vtxvec8SP6P9aGoL2s2HZO6/RbXKRSK41VUnVri3ByHP5g6Y5XpGLJZTu5
AFGLl2D0UiWmRcGCmRj3ghw7VepnKlAnTFfdIqyDKQcD+uRg2+BN8Y4ImOVXe216
GpgqBXgR9FqKK7r0/fp6cd9KsWmj3+RKD6tiyUyv3Q7OH0QSYNRvNL/menw+iyYk
L5/3ODmVO2ZLnMX0ki3QxMmWidJ89xZiP3c+m/kxlF+kNGqiQmY=
=MVHx
-----END PGP SIGNATURE-----

--g7w8+K/95kPelPD2--