Improve handling and coverage of --no-ensure-shutdown in pg_rewind

This includes a couple of changes around the new behavior of pg_rewind
which enforces recovery to happen once on a cluster not shut down
cleanly:
- Some comments and documentation improvements.
- Shutdown the cluster to rewind with immediate mode in all the tests,
this allows to check after the forced recovery behavior which is wanted
as new default.
- Use -F for the forced recovery step, so as postgres does not use
fsync.  This was useless as a final sync is done once the tool is done.

Author: Michael Paquier
Reviewed-by: Alexey Kondratov
Discussion: https://postgr.es/m/20191004083721.GA1829@paquier.xyz

diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index fbf454803b5502394498b2dc3a7c3ed20569e0e6..42d29edd4e963ab2aeeca220f6119b4b1a654ded 100644 (file)
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -169,12 +169,14 @@ PostgreSQL documentation
       <term><option>--no-ensure-shutdown</option></term>
       <listitem>
        <para>
-        <application>pg_rewind</application> verifies that the target server
-        is cleanly shutdown before rewinding; by default, if it isn't, it
-        starts the server in single-user mode to complete crash recovery.
+        <application>pg_rewind</application> requires that the target server
+        is cleanly shut down before rewinding. By default, if the target server
+        is not shut down cleanly, <application>pg_rewind</application> starts
+        the target server in single-user mode to complete crash recovery first,
+        and stops it.
         By passing this option, <application>pg_rewind</application> skips
         this and errors out immediately if the server is not cleanly shut
-        down.  Users are expected to handle the situation themselves in that
+        down. Users are expected to handle the situation themselves in that
         case.
        </para>
       </listitem>

diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index fe1468b771aa71b5e8811234d089334154a8849f..875a43b219360197f3968533f6f946a025e0ddc0 100644 (file)
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -270,11 +270,12 @@ main(int argc, char **argv)
    pg_free(buffer);
 
    /*
-    * If the target instance was not cleanly shut down, run a single-user
-    * postgres session really quickly and reload the control file to get the
-    * new state. Note if no_ensure_shutdown is specified, pg_rewind won't do
-    * that automatically. That means users need to do themselves in advance,
-    * else pg_rewind will soon quit, see sanityChecks().
+    * If the target instance was not cleanly shut down, start and stop the
+    * target cluster once in single-user mode to enforce recovery to finish,
+    * ensuring that the cluster can be used by pg_rewind.  Note that if
+    * no_ensure_shutdown is specified, pg_rewind ignores this step, and users
+    * need to make sure by themselves that the target cluster is in a clean
+    * state.
     */
    if (!no_ensure_shutdown &&
        ControlFile_target.state != DB_SHUTDOWNED &&
@@ -847,8 +848,12 @@ ensureCleanShutdown(const char *argv0)
    if (dry_run)
        return;
 
-   /* finally run postgres in single-user mode */
-   snprintf(cmd, MAXCMDLEN, "\"%s\" --single -D \"%s\" template1 < \"%s\"",
+   /*
+    * Finally run postgres in single-user mode.  There is no need to use
+    * fsync here.  This makes the recovery faster, and the target data folder
+    * is synced at the end anyway.
+    */
+   snprintf(cmd, MAXCMDLEN, "\"%s\" --single -F -D \"%s\" template1 < \"%s\"",
             exec_path, datadir_target, DEVNULL);
 
    if (system(cmd) != 0)

diff --git a/src/bin/pg_rewind/t/005_same_timeline.pl b/src/bin/pg_rewind/t/005_same_timeline.pl
index 40dbc44caa3ac1077611ccb0944614ee84e1656d..df469d393939634a62070346936393223d47c6b8 100644 (file)
--- a/src/bin/pg_rewind/t/005_same_timeline.pl
+++ b/src/bin/pg_rewind/t/005_same_timeline.pl
@@ -1,3 +1,7 @@
+#
+# Test that running pg_rewind with the source and target clusters
+# on the same timeline runs successfully.
+#
 use strict;
 use warnings;
 use TestLib;

diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index c540722420e1710fef70425b2ff0f50462b8997d..82fa220ac8644199f5a9ef6027186d75fc21d6ce 100644 (file)
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -227,8 +227,10 @@ sub run_pg_rewind
    # Append the rewind-specific role to the connection string.
    $standby_connstr = "$standby_connstr user=rewind_user";
 
-   # Stop the master and be ready to perform the rewind
-   $node_master->stop;
+   # Stop the master and be ready to perform the rewind.  The cluster
+   # needs recovery to finish once, and pg_rewind makes sure that it
+   # happens automatically.
+   $node_master->stop('immediate');
 
    # At this point, the rewind processing is ready to run.
    # We now have a very simple scenario with a few diverged WAL record.
@@ -260,19 +262,21 @@ sub run_pg_rewind
    }
    elsif ($test_mode eq "remote")
    {
-
-       # Do rewind using a remote connection as source
+       # Do rewind using a remote connection as source, generating
+       # recovery configuration automatically.
        command_ok(
            [
                'pg_rewind',                      "--debug",
                "--source-server",                $standby_connstr,
-               "--target-pgdata=$master_pgdata", "-R",
-               "--no-sync"
+               "--target-pgdata=$master_pgdata", "--no-sync",
+               "--write-recovery-conf"
            ],
            'pg_rewind remote');
 
-       # Check that standby.signal has been created.
-       ok(-e "$master_pgdata/standby.signal");
+       # Check that standby.signal is here as recovery configuration
+       # was requested.
+       ok( -e "$master_pgdata/standby.signal",
+           'standby.signal created after pg_rewind');
 
        # Now, when pg_rewind apparently succeeded with minimal permissions,
        # add REPLICATION privilege.  So we could test that new standby