[Issue #138] always set "restore_command" in recovery.conf if "--restore-command" option is provided

Author: gsmolk

src/restore.c

@@ -842,7 +842,10 @@ restore_files(void *arg)
 	return NULL;
 }
 
-/* Create recovery.conf with given recovery target parameters */
+/*
+ * Create recovery.conf (probackup_recovery.conf in case of PG12)
+ * with given recovery target parameters
+ */
 static void
 create_recovery_conf(time_t backup_id,
 					 pgRecoveryTarget *rt,
@@ -851,9 +854,17 @@ create_recovery_conf(time_t backup_id,
 {
 	char		path[MAXPGPATH];
 	FILE	   *fp;
-	bool		need_restore_conf;
+	bool		pitr_requested;
 	bool		target_latest;
 	bool		target_immediate;
+	bool 		restore_command_provided = false;
+	char restore_command_guc[16384];
+
+	if (instance_config.restore_command &&
+		(pg_strcasecmp(instance_config.restore_command, "none") != 0))
+	{
+		restore_command_provided = true;
+	}
 
 	/* restore-target='latest' support */
 	target_latest = rt->target_stop != NULL &&
@@ -862,16 +873,39 @@ create_recovery_conf(time_t backup_id,
 	target_immediate = rt->target_stop != NULL &&
 		strcmp(rt->target_stop, "immediate") == 0;
 
-	need_restore_conf = !backup->stream || rt->time_string ||
+	/*
+	 * Note that setting restore_command alone interpreted
+	 * as PITR with target - "until all available WAL is replayed".
+	 * We do this because of the following case:
+	 * The user is restoring STREAM backup as replica but
+	 * also relies on WAL archive to catch-up with master.
+	 * If restore_command is provided, then it should be
+	 * added to recovery config.
+	 * In this scenario, "would be" replica will replay
+	 * all WAL segments available in WAL archive, after that
+	 * it will try to connect to master via repprotocol.
+	 *
+	 * The risk is obvious, what if masters current state is
+	 * in "the past" relatively to latest state in the archive?
+	 * We will get a replica that is "in the future" to the master.
+	 * We accept this risk because its probability is low.
+	 */
+	pitr_requested = !backup->stream || rt->time_string ||
 		rt->xid_string || rt->lsn_string || rt->target_name ||
-		target_immediate || target_latest;
+		target_immediate || target_latest || restore_command_provided;
 
 	/* No need to generate recovery.conf at all. */
-	if (!(need_restore_conf || params->restore_as_replica))
+	if (!(pitr_requested || params->restore_as_replica))
 	{
 		/*
 		 * Restoring STREAM backup without PITR and not as replica,
 		 * recovery.signal and standby.signal for PG12 are not needed
+		 *
+		 * We do not add "include" option in this case because
+		 * here we are creating empty "probackup_recovery.conf"
+		 * to handle possible already existing "include"
+		 * directive pointing to "probackup_recovery.conf".
+		 * If don`t do that, recovery will fail.
 		 */
 		pg12_recovery_config(backup, false);
 		return;
@@ -901,13 +935,10 @@ create_recovery_conf(time_t backup_id,
 #endif
 
 	/* construct restore_command */
-	if (need_restore_conf)
+	if (pitr_requested)
 	{
-		char restore_command_guc[16384];
-
-		/* If restore_command is provided, use it */
-		if (instance_config.restore_command &&
-			(pg_strcasecmp(instance_config.restore_command, "none") != 0))
+		/* If restore_command is provided, use it. Otherwise construct it from scratch. */
+		if (restore_command_provided)
 			sprintf(restore_command_guc, "%s", instance_config.restore_command);
 		else
 		{
@@ -937,10 +968,6 @@ create_recovery_conf(time_t backup_id,
 			}
 		}
 
-		elog(LOG, "Setting restore_command to '%s'", restore_command_guc);
-		fio_fprintf(fp, "restore_command = '%s'\n",
-				restore_command_guc);
-
 		/*
 		 * We've already checked that only one of the four following mutually
 		 * exclusive options is specified, so the order of calls is insignificant.
@@ -996,13 +1023,29 @@ create_recovery_conf(time_t backup_id,
 			fio_fprintf(fp, "primary_conninfo = '%s'\n", backup->primary_conninfo);
 	}
 
+	if (pitr_requested)
+	{
+		elog(LOG, "Setting restore_command to '%s'", restore_command_guc);
+		fio_fprintf(fp, "restore_command = '%s'\n", restore_command_guc);
+	}
+
 	if (fio_fflush(fp) != 0 ||
 		fio_fclose(fp))
 		elog(ERROR, "cannot write file \"%s\": %s", path,
 			 strerror(errno));
 
 #if PG_VERSION_NUM >= 120000
-	if (need_restore_conf)
+	/*
+	 * Create "recovery.signal" to mark this recovery as PITR for PostgreSQL.
+	 * In older versions presense of recovery.conf alone was enough.
+	 * To keep behaviour consistent with older versions,
+	 * we are forced to create "recovery.signal"
+	 * even when only restore_command is provided.
+	 * Presense of "recovery.signal" by itself determine only
+	 * one thing: do PostgreSQL must switch to a new timeline
+	 * after successfull recovery or not?
+	 */
+	if (pitr_requested)
 	{
 		elog(LOG, "creating recovery.signal file");
 		snprintf(path, lengthof(path), "%s/recovery.signal", instance_config.pgdata);

tests/restore.py

@@ -3247,8 +3247,8 @@ def test_missing_database_map(self):
         # Clean after yourself
         self.del_test_dir(module_name, fname)
 
-    @unittest.skip("skip")
-    def test_stream_restore_command_options(self):
+    # @unittest.skip("skip")
+    def test_stream_restore_command_option(self):
         """
         correct handling of restore command options
         when restoring STREAM backup
@@ -3291,40 +3291,46 @@ def test_stream_restore_command_options(self):
 
         node.pgbench_init(scale=1)
 
+        node.safe_psql(
+            'postgres',
+            'CHECKPOINT')
+
+        node.safe_psql(
+            'postgres',
+            'create table t1()')
+
         # restore backup
         node.cleanup()
         shutil.rmtree(os.path.join(node.logs_dir))
 
-#        self.restore_node(backup_dir, 'node', node)
         restore_cmd = self.get_restore_command(backup_dir, 'node', node)
 
         self.restore_node(
             backup_dir, 'node', node,
             options=[
-                '--restore-command={0}'.format(restore_cmd),
-                '--recovery-target-action=pause',
-                '--recovery-target=latest'])
+                '--restore-command={0}'.format(restore_cmd)])
 
         self.assertTrue(
             os.path.isfile(recovery_conf),
-            "File {0} do not exists".format(recovery_conf))
+            "File '{0}' do not exists".format(recovery_conf))
+
+        if self.get_version(node) >= self.version_to_num('12.0'):
+            recovery_signal = os.path.join(node.data_dir, 'recovery.signal')
+            self.assertTrue(
+                os.path.isfile(recovery_signal),
+                "File '{0}' do not exists".format(recovery_signal))
 
         node.slow_start()
 
-        exit(1)
+        node.safe_psql(
+            'postgres',
+            'select * from t1')
+
+        timeline_id = node.safe_psql(
+            'postgres',
+            'select timeline_id from pg_control_checkpoint()').rstrip()
 
-#        self.set_config(
-#            backup_dir ,'node',
-#            options=['--restore-command="cp {0}/%f %p"'.format(wal_dir)])
-#
-#        # restore delta backup
-#        node.cleanup()
-#        self.restore_node(
-#            backup_dir, 'node', node, options=['--recovery-target=immediate'])
-#
-#        self.assertTrue(
-#            os.path.isfile(recovery_conf),
-#            "File {0} do not exists".format(recovery_conf))
+        self.assertEqual('2', timeline_id)
 
         # Clean after yourself
         self.del_test_dir(module_name, fname)