Date: Mon, 7 Oct 2019 10:06:51 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, Paul Guo <pguo@pivotal.io>,
	Thomas Munro <thomas.munro@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
	Jimmy Yih <jyih@pivotal.io>, Ashwin Agrawal <aagrawal@pivotal.io>
Subject: Re: Two pg_rewind patches (auto generate recovery conf and ensure
 clean shutdown)
Message-ID: <20191007010651.GD14532@paquier.xyz>
References: <20190930171349.GA21905@alvherre.pgsql>
 <7ca88204-3e0b-2f4c-c8af-acadc4b266e5@postgrespro.ru>
 <20191003030752.GE1586@paquier.xyz>
 <c62d6a22-2976-8491-8e66-bcbb33ac3eb0@postgrespro.ru>
 <20191004083721.GA1829@paquier.xyz>
 <ab5c15e0-773c-fbb4-f0a2-ca00a90035a6@postgrespro.ru>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="juZjCTNxrMaZdGZC"
Content-Disposition: inline
In-Reply-To: <ab5c15e0-773c-fbb4-f0a2-ca00a90035a6@postgrespro.ru>
Precedence: bulk


--juZjCTNxrMaZdGZC
Content-Type: multipart/mixed; boundary="/2994txjAzEdQwm5"
Content-Disposition: inline


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

On Fri, Oct 04, 2019 at 05:21:25PM +0300, Alexey Kondratov wrote:
> I've checked your patch, but it seems that it cannot be applied as is, since
> it e.g. adds a comment to 005_same_timeline.pl without actually changing the
> test. So I've slightly modified your patch and tried to fit both dry-run and
> ensureCleanShutdown testing together. It works just fine and fails
> immediately if any of recent fixes is reverted. I still think that dry-run
> testing is worth adding, since it helped to catch this v12 refactoring
> issue, but feel free to throw it way if it isn't commitable right now, of
> course.

I can guarantee the last patch I sent can be applied on top of HEAD:
https://www.postgresql.org/message-id/20191004083721.GA1829@paquier.xyz

It would be nice to add the --dry-run part, though I think that we
could just make that part of one of the existing tests, and stop the
target server first (got to think about that part, please see below).

> As for incompatible options and sanity checks testing, yes, I agree that it
> is a matter of different patch. I attached it as a separate WIP patch just
> for history. Maybe I will try to gather more cases there later.

Thanks.  I have applied the first patch for the various improvements
around --no-ensure-shutdown.

Regarding the rest, I have hacked my way through as per the attached.
The previous set of patches did the following, which looked either
overkill or not necessary:
- Why running test 005 with the remote mode?
- --dry-run coverage is basically the same with the local and remote
modes, so it seems like a waste of resource to run it for all the
tests and all the modes.  I tend to think that this would live better
as part of another existing test, only running for say the local mode.
It is also possible to group all your tests from patch 2 and
006_actions.pl in this area.
- There is no need for the script checking for options combinations to
initialize a data folder.  It is important to design the tests to be
cheap and meaningful.

Patch v3-0002 also had a test to make sure that the source server is
shut down cleanly before using it.  I have included that part as
well, as the flow feels right.

So, Alexey, what do you think?
--
Michael

--/2994txjAzEdQwm5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="rewind-new-tests.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_bas=
ic.pl
index c3293e93df..645dc24578 100644
--- a/src/bin/pg_rewind/t/001_basic.pl
+++ b/src/bin/pg_rewind/t/001_basic.pl
@@ -1,7 +1,7 @@
 use strict;
 use warnings;
 use TestLib;
-use Test::More tests =3D> 11;
+use Test::More tests =3D> 15;
=20
 use FindBin;
 use lib $FindBin::RealBin;
@@ -66,6 +66,76 @@ sub run_test
 	master_psql("DELETE FROM tail_tbl WHERE id > 10");
 	master_psql("VACUUM tail_tbl");
=20
+	# Before running pg_rewind, do a couple of extra tests with several
+	# option combinations.  As the code paths taken by those tests
+	# does not change for the "local" and "remote" modes, just run them
+	# in "local" mode for simplicity's sake.
+	if ($test_mode eq 'local')
+	{
+		my $master_pgdata  =3D $node_master->data_dir;
+		my $standby_pgdata =3D $node_standby->data_dir;
+
+		# First check that pg_rewind fails if the target cluster is
+		# not stopped as it fails to start up for the forced recovery
+		# step.
+		command_fails(
+			[
+				'pg_rewind',
+				"--debug",
+				"--source-pgdata=3D$standby_pgdata",
+				"--target-pgdata=3D$master_pgdata",
+				"--no-sync"
+			],
+			'pg_rewind with running target');
+
+		# Again with --no-ensure-shutdown, which should equally fail.
+		# This time pg_rewind complains without attempting to perform
+		# recovery once.
+		command_fails(
+			[
+				'pg_rewind',
+				"--debug",
+				"--source-pgdata=3D$standby_pgdata",
+				"--target-pgdata=3D$master_pgdata",
+				"--no-sync",
+				'--no-ensure-shutdown'
+			],
+			'pg_rewind --no-ensure-shutdown with running target');
+
+		# Stop the target, and attempt to run with a local source
+		# still running.  This fail as pg_rewind requires to have
+		# a source cleanly stopped.
+		$node_master->stop;
+		command_fails(
+			[
+				'pg_rewind',
+				"--debug",
+				"--source-pgdata=3D$standby_pgdata",
+				"--target-pgdata=3D$master_pgdata",
+				"--no-sync",
+				'--no-ensure-shutdown'
+			],
+			'pg_rewind with running source');
+
+		# Stop the target cluster cleanly, and run again pg_rewind
+		# with --dry-run mode.  If anything gets generated in the data
+		# folder, the follow-up run of pg_rewind will most likely fail,
+		# so keep this test as the last one of this subset.
+		$node_standby->stop;
+		command_ok(
+			[
+				'pg_rewind', "--debug",
+				"--source-pgdata=3D$standby_pgdata",
+				"--target-pgdata=3D$master_pgdata",
+				"--no-sync", '--dry-run'
+			],
+			'pg_rewind --dry-run with running target');
+
+		# Both clusters need to be alive moving forward.
+		$node_standby->start;
+		$node_master->start;
+	}
+
 	RewindTest::run_pg_rewind($test_mode);
=20
 	check_query(
diff --git a/src/bin/pg_rewind/t/006_options.pl b/src/bin/pg_rewind/t/006_o=
ptions.pl
new file mode 100644
index 0000000000..6b8e2ad765
--- /dev/null
+++ b/src/bin/pg_rewind/t/006_options.pl
@@ -0,0 +1,38 @@
+#
+# Test checking options of pg_rewind.
+#
+use strict;
+use warnings;
+use TestLib;
+use Test::More tests =3D> 12;
+
+program_help_ok('pg_rewind');
+program_version_ok('pg_rewind');
+program_options_handling_ok('pg_rewind');
+
+my $primary_pgdata =3D TestLib::tempdir;
+my $standby_pgdata =3D TestLib::tempdir;
+command_fails(
+	[
+		'pg_rewind',     '--target-pgdata',
+		$primary_pgdata, '--source-pgdata',
+		$standby_pgdata, 'extra_arg1'
+	],
+	'too many arguments');
+command_fails([ 'pg_rewind', '--target-pgdata', $primary_pgdata ],
+	'no source specified');
+command_fails(
+	[
+		'pg_rewind',     '--target-pgdata',
+		$primary_pgdata, '--source-pgdata',
+		$standby_pgdata, '--source-server',
+		'popo'
+	],
+	'both remote and local sources specified');
+command_fails(
+	[
+		'pg_rewind',       '--target-pgdata',
+		$primary_pgdata,   '--write-recovery-conf',
+		'--source-pgdata', $standby_pgdata
+	],
+	'no remote source with --write-recovery-conf');

--/2994txjAzEdQwm5--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl2aj6sACgkQnvQgOdby
QH3iRQ/+JbL/g5NsYaeRHGTp7AAnxLqufyxFu6TVnmX4pvhKlLJn3y+IZ8EP5eu3
Ioc7627zzz99WqSt489BP6TuT6pQPlOELkAisJ+xf1YueDr+yWV8WiVfvjCdgfH3
TGrIyJV/LlxPy28gVX7G6ewFaOTXEHTZB6S6OF3YkblopNvleovBoAP+hFZfjX6x
5JbLhe4GADHzDDM0YHoP0sGD0pfyKU9N00u8xA/s94berqzIiK1fJBjvZkrKv2n+
a94UMtWGXnTPdnFqvRbsjkTc0zhprd11922gY6GBYXvxhxDIk1IAeKA9ZjvWaIto
DNxnWlImCHS7OjeuqZ80rMPQEzBhizhd1ZIKYnnPG52cEfoH9gB4fpa0pTgf6sCZ
HP8x7KfV/Iv4y5WtCS6YJeocsD7aBiYIo9/ht+WVqO2k/ToVXR6+cFa0Hbf12H36
cHBJ7M2dQX50wWWBUROiPGk2h7iXqPPfo537KAdpty5aK3eEHKInawiofxvedcfN
NlLJkuHTo3Rd0buwE8eOek03jh5XP7EnR6Vdlw9jjeRtR8o1YLCXcyLICzSqFn/0
vLg6qf/R/r+o6SujOfG32JxA/HZiP2cvAlDXOY/86v8ggZ5R8RTaTMKfHhmjgoDV
TJG/w3aSitYAzepD8rlLeE7nGlYBhWVscnvBU5dP/uZW7kMwmHU=
=QrrH
-----END PGP SIGNATURE-----

--juZjCTNxrMaZdGZC--