Attached new version of a patch with TAP test.

On 11/5/19 11:51 AM, Grigory Smolkin wrote:
Thank you for you interest in this topic!

On 11/5/19 10:41 AM, Kyotaro Horiguchi wrote:
Hello.

At Mon, 4 Nov 2019 16:03:38 +0300, Grigory Smolkin <g.smol...@postgrespro.ru> wrote in
Hello, hackers!

I`d like to propose a new argument for recovery_target parameter,
which will stand to recovering until all available WAL segments are
applied.

Current PostgreSQL recovery default behavior(when no recovery target
is provided) does exactly that, but there are several shortcomings:
   - without explicit recovery target standing for default behavior,
recovery_target_action is not coming to action at the end of recovery
   - with PG12 changes, the life of all backup tools became very hard,
because now recovery parameters can be set outside of single config
file(recovery.conf), so it is impossible to ensure, that default
recovery behavior, desired in some cases, will not be silently
overwritten by some recovery parameter forgotten by user.

Proposed path is very simple and solves the aforementioned problems by
introducing new argument "latest" for recovery_target parameter.
Does the tool remove or rename recovery.conf to cancel the settings?
And do you intend that the new option is used to override settings by
appending it at the end of postgresql.conf? If so, the commit
f2cbffc7a6 seems to break the assumption. PG12 rejects to start if it
finds two different kinds of recovery target settings.
Yes, previously it was possible to remove/rename old recovery.conf, but not anymore. My assumption is exactly that PG should reject to start because of multiple recovery targets. Failing to start is infinitely better that recovering to the wrong recovery target.

Old recovery behavior is still available if no recovery target is
provided. I`m not sure, whether it should it be left as it is now, or
not.

Another open question is what to do with recovery_target_inclusive if
recovery_target = "latest" is used.
Anyway inclusiveness doesn't affect to "immediate". If we had the
"latest" option, it would behave the same way.
Right, thank you.

regards.

--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 619ac8c50c..f0fa0b09c8 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3316,21 +3316,19 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
 
      <variablelist>
      <varlistentry id="guc-recovery-target" xreflabel="recovery_target">
-      <term><varname>recovery_target</varname><literal> = 'immediate'</literal>
+      <term><varname>recovery_target</varname> (<type>string</type>)
       <indexterm>
         <primary><varname>recovery_target</varname> configuration parameter</primary>
       </indexterm>
       </term>
       <listitem>
        <para>
-        This parameter specifies that recovery should end as soon as a
-        consistent state is reached, i.e. as early as possible. When restoring
-        from an online backup, this means the point where taking the backup
-        ended.
-       </para>
-       <para>
-        Technically, this is a string parameter, but <literal>'immediate'</literal>
-        is currently the only allowed value.
+        This parameter determines how far recovery should proceed. The value
+        <literal>immediate</literal> means that recovery should end as soon as a consistent
+        state is reached, i.e. as early as possible. When restoring from an online
+        backup, this means the point where taking the backup ended.
+        The second possible value <literal>latest</literal> means that recovery
+        should proceed to the end of the available WAL log.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 77ad765989..707ebe68a1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6350,6 +6350,9 @@ StartupXLOG(void)
 		else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
 			ereport(LOG,
 					(errmsg("starting point-in-time recovery to earliest consistent point")));
+		else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+			ereport(LOG,
+					(errmsg("starting point-in-time recovery until all available WAL is applied")));
 		else
 			ereport(LOG,
 					(errmsg("starting archive recovery")));
@@ -7261,6 +7264,13 @@ StartupXLOG(void)
 			 * end of main redo apply loop
 			 */
 
+			/*
+			 * If all avaialble WAL is replayed,
+			 * then RECOVERY_TARGET_LATEST is satisfied
+			 */
+			if (recoveryTarget == RECOVERY_TARGET_LATEST)
+				reachedStopPoint = true;
+
 			if (reachedStopPoint)
 			{
 				if (!reachedConsistency)
@@ -7462,6 +7472,8 @@ StartupXLOG(void)
 					 recoveryStopName);
 		else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
 			snprintf(reason, sizeof(reason), "reached consistency");
+		else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+			snprintf(reason, sizeof(reason), "applied all available WAL");
 		else
 			snprintf(reason, sizeof(reason), "no recovery target specified");
 
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f0ed326a1b..e7288a79b6 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3503,7 +3503,10 @@ static struct config_string ConfigureNamesString[] =
 
 	{
 		{"recovery_target", PGC_POSTMASTER, WAL_RECOVERY_TARGET,
-			gettext_noop("Set to \"immediate\" to end recovery as soon as a consistent state is reached."),
+			gettext_noop("Set to \"immediate\" to end recovery as "
+						 "soon as a consistent state is reached or "
+						 " to \"latest\" to end recovery after "
+						 "replaying all available WAL in the archive."),
 			NULL
 		},
 		&recovery_target_string,
@@ -11498,9 +11501,10 @@ error_multiple_recovery_targets(void)
 static bool
 check_recovery_target(char **newval, void **extra, GucSource source)
 {
-	if (strcmp(*newval, "immediate") != 0 && strcmp(*newval, "") != 0)
+	if (strcmp(*newval, "immediate") != 0 && strcmp(*newval, "latest") != 0 &&
+		strcmp(*newval, "") != 0)
 	{
-		GUC_check_errdetail("The only allowed value is \"immediate\".");
+		GUC_check_errdetail("The only allowed values are \"immediate\" and \"latest\".");
 		return false;
 	}
 	return true;
@@ -11510,11 +11514,17 @@ static void
 assign_recovery_target(const char *newval, void *extra)
 {
 	if (recoveryTarget != RECOVERY_TARGET_UNSET &&
-		recoveryTarget != RECOVERY_TARGET_IMMEDIATE)
+		recoveryTarget != RECOVERY_TARGET_IMMEDIATE &&
+		recoveryTarget != RECOVERY_TARGET_LATEST)
 		error_multiple_recovery_targets();
 
 	if (newval && strcmp(newval, "") != 0)
-		recoveryTarget = RECOVERY_TARGET_IMMEDIATE;
+	{
+		if (strcmp(newval, "immediate") == 0)
+			recoveryTarget = RECOVERY_TARGET_IMMEDIATE;
+		else if (strcmp(newval, "latest") == 0)
+			recoveryTarget = RECOVERY_TARGET_LATEST;
+	}
 	else
 		recoveryTarget = RECOVERY_TARGET_UNSET;
 }
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index d519252aad..55e7e4bd2c 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -84,7 +84,8 @@ typedef enum
 	RECOVERY_TARGET_TIME,
 	RECOVERY_TARGET_NAME,
 	RECOVERY_TARGET_LSN,
-	RECOVERY_TARGET_IMMEDIATE
+	RECOVERY_TARGET_IMMEDIATE,
+	RECOVERY_TARGET_LATEST
 } RecoveryTargetType;
 
 /*
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index d8fbd50011..01dab1aebf 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 8;
+use Test::More tests => 9;
 
 # Create and test a standby from given backup, with a certain recovery target.
 # Choose $until_lsn later than the transaction commit that causes the row
@@ -77,7 +77,7 @@ my $lsn3 =
   $node_master->safe_psql('postgres', "SELECT pg_current_wal_lsn();");
 my $recovery_time = $node_master->safe_psql('postgres', "SELECT now()");
 
-# Even more data, this time with a recovery target name
+# More data, with a recovery target name
 $node_master->safe_psql('postgres',
 	"INSERT INTO tab_int VALUES (generate_series(3001,4000))");
 my $recovery_name = "my_target";
@@ -86,14 +86,17 @@ my $lsn4 =
 $node_master->safe_psql('postgres',
 	"SELECT pg_create_restore_point('$recovery_name');");
 
-# And now for a recovery target LSN
+# Even more data, this time with a recovery target LSN
 $node_master->safe_psql('postgres',
 	"INSERT INTO tab_int VALUES (generate_series(4001,5000))");
 my $lsn5 = my $recovery_lsn =
   $node_master->safe_psql('postgres', "SELECT pg_current_wal_lsn()");
 
+# And now for a recovery target 'latest'
 $node_master->safe_psql('postgres',
 	"INSERT INTO tab_int VALUES (generate_series(5001,6000))");
+my $lsn6 =
+  $node_master->safe_psql('postgres', "SELECT pg_current_wal_lsn();");
 
 # Force archiving of WAL file
 $node_master->safe_psql('postgres', "SELECT pg_switch_wal()");
@@ -114,6 +117,9 @@ test_recovery_standby('name', 'standby_4', $node_master, \@recovery_params,
 @recovery_params = ("recovery_target_lsn = '$recovery_lsn'");
 test_recovery_standby('LSN', 'standby_5', $node_master, \@recovery_params,
 	"5000", $lsn5);
+@recovery_params = ("recovery_target = 'latest'");
+test_recovery_standby('latest target', 'standby_6', $node_master, \@recovery_params,
+	"6000", $lsn6);
 
 # Multiple targets
 #
@@ -126,9 +132,9 @@ test_recovery_standby('LSN', 'standby_5', $node_master, \@recovery_params,
 	"recovery_target_name = ''",
 	"recovery_target_time = '$recovery_time'");
 test_recovery_standby('multiple overriding settings',
-	'standby_6', $node_master, \@recovery_params, "3000", $lsn3);
+	'standby_7', $node_master, \@recovery_params, "3000", $lsn3);
 
-my $node_standby = get_new_node('standby_7');
+my $node_standby = get_new_node('standby_8');
 $node_standby->init_from_backup($node_master, 'my_backup',
 	has_restoring => 1);
 $node_standby->append_conf(

Reply via email to