Date: Mon, 9 Apr 2018 13:58:26 +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>,
	PostgreSQL mailing lists <pgsql-bugs@postgresql.org>
Subject: Re: BUG #14999: pg_rewind corrupts control file global/pg_control
Message-ID: <20180409045826.GA1740@paquier.xyz>
References: <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>
 <20180405013818.GC1732@paquier.xyz>
 <E1979209-A39E-4229-BBE0-59BC43FB4691@tiptop-labs.com>
 <20180405065840.GH1732@paquier.xyz>
 <15971.1523116292@sss.pgh.pa.us>
 <20180407222258.GA3345@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="3lcZGd9BuhuYXNfi"
Content-Disposition: inline
In-Reply-To: <20180407222258.GA3345@paquier.xyz>
Precedence: bulk


--3lcZGd9BuhuYXNfi
Content-Type: multipart/mixed; boundary="ikeVEW9yuYc//A+q"
Content-Disposition: inline


--ikeVEW9yuYc//A+q
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sun, Apr 08, 2018 at 07:22:58AM +0900, Michael Paquier wrote:
> Yes, that's one of the methods I mentioned in my previous email.  Now,
> do we want to fail the run immediately if such a file is found?  Or do
> we want to report at once all such files so as the user does not need to
> run multiple times pg_rewind.

So I have been playing with that part...

> The latter would be much more useful.

While that would be nice, the result is rather ugly and this needs to
generate the same error message in two code paths.  So I have chewed
things in such a way that one failure found makes the whole rewind to
just fail, and allows retries to work.  The attached has a test as well
as documentation.

Thoughts?

> A downside here is that the file from the source would still be fetched
> and copied on the target.  So a file which was in read-only would become
> basically writable.  Don't think that it is a big deal as the has
> superuser rights at this point, but that's worth mentioning.

That's mentioned in the docs of the attached.
--
Michael

--ikeVEW9yuYc//A+q
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="rewind-readonly-v4.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.s=
gml
index 520d843f0e..0952f3903e 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -95,6 +95,25 @@ 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>
+
+  <para>
+   Before rewriting any files on the target data directory,
+   <application>pg_rewind</application> checks if any files are writable,
+   failing immediately if that is not the case.  It is possible to
+   execute again a rewind after fixing the permission issues.  This can
+   happen for example for read-only SSL-related configuration files.
+   Note that if the file is copied from the source to the target, then
+   it may be necessary to rebuild the links properly as a rewind would
+   have caused those files to be created with newer permission sets.
+  </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>
+  </warning>
  </refsect1>
=20
  <refsect1>
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index c3fc519895..c0b58c76ce 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -189,7 +189,56 @@ process_source_file(const char *path, file_type_t type=
, size_t newsize,
 		exists =3D false;
 	}
 	else
-		exists =3D true;
+	{
+		int			fd;
+
+		/*
+		 * The file exists on the source and the target.  Now check if
+		 * the file is writable.
+		 */
+		fd =3D open(localpath, O_WRONLY);
+
+		if (fd < 0)
+		{
+			if (errno =3D=3D EISDIR)
+			{
+				/*
+				 * No need to worry about directories, those are not
+				 * valid targets for writes.
+				 */
+				exists =3D true;
+			}
+			else if (errno =3D=3D EACCES)
+			{
+				/*
+				 * The file exists on the source and the target but it is
+				 * not writable on target.  In order to avoid failures
+				 * mid-flight when processing the data directory, fail
+				 * immediately and let the user know.  This way, if anything
+				 * like a read-only file is found, then pg_rewind complains
+				 * and a successive run can be completed after doing some
+				 * cleanup on this target data directory.
+				 */
+				pg_fatal("file \"%s\" exists on both target and source server, but is =
not writable on target.\n"
+						 "Please check the permissions on \"%s\" before executing again pg_r=
ewind.\n",
+						 path, localpath);
+			}
+			else
+			{
+				/*
+				 * Complain about anything else, that should not happen,
+				 * even ENOENT which has already been checked above.
+				 */
+				pg_fatal("could not open file \"%s\": %s\n",
+						 localpath, strerror(errno));
+			}
+		}
+		else
+		{
+			exists =3D true;
+			close(fd);
+		}
+	}
=20
 	switch (type)
 	{
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..489e1f4593
--- /dev/null
+++ b/src/bin/pg_rewind/t/006_readonly.pl
@@ -0,0 +1,77 @@
+# 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');
+
+# 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);

--ikeVEW9yuYc//A+q--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlrK8vIACgkQnvQgOdby
QH0CUQ//Ys9XRrM153sDrhyw4x7XQKRb3Ni4/foB3m1653Fg6ti+FJedBec2ebeb
FCIU9Ar/LM02ZByukp1R+fIDrVeJmFBFBXAWvkHLCmz4jdIgMbWpGYej+qptdxS2
WXymSb93o0GqCHwihpdyfWRFt3+xBeI5fu6aQb4hthCLBcOVwbbcawxHEJT5MRZ9
1RGRaZE+g7B+Ll84ouMp6wnabGCiCauULOFMuh4AC59XtO6R7yKNyzE3uk6nvVG5
wHvYs1/xni3PbbGHehDEZPYs+yJxlX4yyylAe3L7XOrkzSyPIQe1K3ZOva0cosNU
Ls80JZay7TW4RWUxd8naT77qGbud71vkYkXY/SPSxHjbe+pKhHPvWJDvbE7qigYc
eYpMbO3cwy4Ba6qAs74Hk381lvqfJAI3vwaM3b9hQx+4GeMCBMDZXogQm7h5A82u
vnGf18Vi+xiLdlNWMPR6hMpSFBok6k3h/H78xYE6HLN5mSP/540U48pEav7qJq4z
lIhw4Q/Q/zCZmrBrVVfOrj86dZNGDbIST2qt/G6MfshGNAbG62au8o4+Z0sFc2EN
5WGZ1jMiYi5KzEeqqJiAnrNOMBLj8nQyYye9EAwb4g+JAk0UDVy7cT+VvSRzU0oI
s2NPDi34n75AZY3UOIDAVZvqlpvjL1WFq4rE3hgxe2ZhVJIum2c=
=3NNL
-----END PGP SIGNATURE-----

--3lcZGd9BuhuYXNfi--