Re: pause recovery if pitr target not reached - Mailing list pgsql-hackers

Hello.

At Mon, 27 Jan 2020 12:16:02 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in 
> On 2020-01-15 05:02, Kyotaro Horiguchi wrote:
> > FWIW, I restate this (perhaps) more clearly.
> > At Wed, 15 Jan 2020 11:02:24 +0900 (JST), Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote in
> >> recvoery_target_* is not cleared after startup. If a server crashed
> >> just after the last shutdown checkpoint, any recovery_target_* setting
> >> prevents the server from starting regardless of its value.
> > recvoery_target_* is not automatically cleared after a successful
> > archive recovery.  After that, if the server crashed just after the
> > last shutdown checkpoint, any recovery_target_* setting prevents the
> > server from starting regardless of its value.
> 
> Thank you for this clarification.  Here is a new patch that addresses
> that and also the other comments raised about my previous patch.

The code looks fine, renaming reachedStopPoint to
reachedRecoveryTarget looks very nice. Doc part looks fine, too.


PostgresNode.pm
+Is has_restoring is used, standby mode is used by default.  To use

"Is has_restoring used,", or "If has_restoring is used," ?


+    $params{standby} = 1 unless defined $params{standby};
..
-    $self->enable_restoring($root_node) if $params{has_restoring};
+    $self->enable_restoring($root_node, $params{standby}) if $params{has_restoring};
...
+    if ($standby)
+    {
+        $self->set_standby_mode();
+    }
+    else
+    {
+        $self->set_recovery_mode();
+    }

The change seems aiming not to break compatibility with external test
scripts, but it looks quite confusing to me.  The problem here is both
enable_streaming/restoring independently put trigger files, so don't
we separte placing of trigger files out of the functions?

As you know, set_standby_mode and set_recovery_mode are described as
"internal" but actually used by some test scripts. I think it's
preferable that the functions are added in POD rather than change the
callers not to used them.

The attached patch on top of yours does that. It might be too much but
what do you think about that?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 23fc36a84bf0f4bfce158b00399a5caa85bcc55f Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Tue, 28 Jan 2020 13:10:43 +0900
Subject: [PATCH] Refactor init_from_backup

To allow recovery mode in init_from_backup, refactor how parameters
are handled. On the way doing that, separate placing trigger files out
of enable_streaming and enable_restoring so that the function can
place trigger files properly.

Several tests using the internal functions enable_restoring and
enable_streaming so expose them in POD rather than change the callers
not to use the functions.
---
 src/test/perl/PostgresNode.pm               | 127 ++++++++++++++++----
 src/test/recovery/t/003_recovery_targets.pl |   2 +-
 2 files changed, 107 insertions(+), 22 deletions(-)

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index be44e8784f..7d10658eb0 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -653,8 +653,10 @@ Restoring WAL segments from archives using restore_command can be enabled
 by passing the keyword parameter has_restoring => 1. This is disabled by
 default.
 
-Is has_restoring is used, standby mode is used by default.  To use
-recovery mode instead, pass the keyword parameter standby => 0.
+If has_restoring or has_restoring is used, standby mode is used by
+default.  To use recovery mode instead, pass the keyword parameter
+recovery => 1.  Not to use recovery nor standby mode, pass the keyword
+parameter standby => 0.
 
 The backup is copied, leaving the original unmodified. pg_hba.conf is
 unconditionally set to enable replication connections.
@@ -669,10 +671,30 @@ sub init_from_backup
     my $port        = $self->port;
     my $node_name   = $self->name;
     my $root_name   = $root_node->name;
+    my $connstr       = $root_node->connstr;
+    my $streaming    = 0;
+    my $restoring    = 0;
+    my $standby        = $params{standby};
+    my $recovery    = $params{recovery};
 
-    $params{has_streaming} = 0 unless defined $params{has_streaming};
-    $params{has_restoring} = 0 unless defined $params{has_restoring};
-    $params{standby} = 1 unless defined $params{standby};
+    $streaming = 1 if ($params{has_streaming});
+    $restoring = 1 if ($params{has_restoring});
+
+    if ($recovery)
+    {
+        croak "### Standby mode is mutually exclusive with recovery mode.\n"
+          if ($standby);
+    }
+    elsif (defined $standby)
+    {
+        # follow explict instruction
+        $standby = $params{standby};
+    }
+    elsif ($streaming || $restoring)
+    {
+        # historically both streaming and restoring turn on standby mode
+        $standby = 1;
+    }
 
     print
       "# Initializing node \"$node_name\" from backup \"$backup_name\" of node \"$root_name\"\n";
@@ -702,8 +724,31 @@ port = $port
         $self->append_conf('postgresql.conf',
             "unix_socket_directories = '$host'");
     }
-    $self->enable_streaming($root_node) if $params{has_streaming};
-    $self->enable_restoring($root_node, $params{standby}) if $params{has_restoring};
+
+    if ($restoring)
+    {
+        print "### Enabling WAL restore for node \"$node_name\"\n";
+        $self->setup_restore_command($root_node);
+    }
+
+    if ($streaming)
+    {
+        print "### Enabling primary conninfo for node \"$node_name\"\n";
+        $self->setup_primary_conninfo($root_node);
+    }
+
+    # recovery mode supercedes standby mode
+    if ($recovery)
+    {
+        print "### Enabling recovery mode for node \"$node_name\"\n";
+        $self->set_recovery_mode();
+    }
+    elsif ($standby)
+    {
+        print "### Enabling standby mode for node \"$node_name\"\n";
+        $self->set_standby_mode();
+    }
+
     return;
 }
 
@@ -924,7 +969,13 @@ sub logrotate
     return;
 }
 
-# Internal routine to enable streaming replication on a standby node.
+=pod
+
+=item PostgresNode->enable_streaming(root_node)
+
+Set up primary_conninfo according to root_node then turns on standby mode.
+=cut
+
 sub enable_streaming
 {
     my ($self, $root_node) = @_;
@@ -932,15 +983,34 @@ sub enable_streaming
     my $name         = $self->name;
 
     print "### Enabling streaming replication for node \"$name\"\n";
-    $self->append_conf(
-        'postgresql.conf', qq(
-primary_conninfo='$root_connstr'
-));
+    $self->setup_primary_conninfo($root_node);
     $self->set_standby_mode();
     return;
 }
 
-# Internal routine to enable archive recovery command on a standby node
+# Internal routine to set up primary_conninfo
+sub setup_primary_conninfo
+{
+    my ($self, $root_node) = @_;
+    my $root_connstr = $root_node->connstr;
+    my $name         = $self->name;
+
+    $self->append_conf(
+        'postgresql.conf', qq(
+primary_conninfo='$root_connstr'
+));
+    return;
+}
+
+=pod
+
+=item PostgresNode->enable_restoring(root_node [, standby])
+
+Set up restore_command according to root_node. Also turns on standby
+mode by default. To use recovery mode instead, pass the keyword
+parameter standby => 0.
+
+=cut
 sub enable_restoring
 {
     my ($self, $root_node, $standby) = @_;
@@ -949,6 +1019,29 @@ sub enable_restoring
 
     print "### Enabling WAL restore for node \"$name\"\n";
 
+    $self->setup_restore_command($root_node);
+
+    if (!defined $standby || $standby)
+    {
+        print "### Enabling standby mode for node \"$name\"\n";
+        $self->set_standby_mode();
+    }
+    else
+    {
+        print "### Enabling recovery mode for node \"$name\"\n";
+        $self->set_recovery_mode();
+    }
+
+    return;
+}
+
+# Internal routine to set up restore_command
+sub setup_restore_command
+{
+    my ($self, $root_node, $standby) = @_;
+    my $path = TestLib::perl2host($root_node->archive_dir);
+    my $name = $self->name;
+
     # On Windows, the path specified in the restore command needs to use
     # double back-slashes to work properly and to be able to detect properly
     # the file targeted by the copy command, so the directory value used
@@ -965,14 +1058,6 @@ sub enable_restoring
         'postgresql.conf', qq(
 restore_command = '$copy_command'
 ));
-    if ($standby)
-    {
-        $self->set_standby_mode();
-    }
-    else
-    {
-        $self->set_recovery_mode();
-    }
     return;
 }
 
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index fd14bab208..c76e060d72 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -151,7 +151,7 @@ ok($logfile =~ qr/multiple recovery targets specified/,
 
 $node_standby = get_new_node('standby_8');
 $node_standby->init_from_backup($node_master, 'my_backup',
-                                has_restoring => 1, standby => 0);
+                                has_restoring => 1, recovery => 1);
 $node_standby->append_conf('postgresql.conf',
                            "recovery_target_name = 'does_not_exist'");
 
-- 
2.18.2