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.ga1...@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
diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.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 => 11;
+use Test::More tests => 15;
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");
+ # 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 = $node_master->data_dir;
+ my $standby_pgdata = $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=$standby_pgdata",
+ "--target-pgdata=$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=$standby_pgdata",
+ "--target-pgdata=$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=$standby_pgdata",
+ "--target-pgdata=$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=$standby_pgdata",
+ "--target-pgdata=$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);
check_query(
diff --git a/src/bin/pg_rewind/t/006_options.pl b/src/bin/pg_rewind/t/006_options.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 => 12;
+
+program_help_ok('pg_rewind');
+program_version_ok('pg_rewind');
+program_options_handling_ok('pg_rewind');
+
+my $primary_pgdata = TestLib::tempdir;
+my $standby_pgdata = 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');
signature.asc
Description: PGP signature
