From 7244ee3b86ffc10b8312e1f8755543c817ed4bde Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Wed, 30 Oct 2019 18:16:00 +0300 Subject: [PATCH] [Issue #138] always set "restore_command" in recovery.conf if "--restore-command" option is provided --- src/restore.c | 75 +++++++++++++++++++++++++++++++++++++----------- tests/restore.py | 46 ++++++++++++++++------------- 2 files changed, 85 insertions(+), 36 deletions(-) diff --git a/src/restore.c b/src/restore.c index 9591f7ed..8b2924dd 100644 --- a/src/restore.c +++ b/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); diff --git a/tests/restore.py b/tests/restore.py index b1634b51..02a3bd6d 100644 --- a/tests/restore.py +++ b/tests/restore.py @@ -3247,8 +3247,8 @@ class RestoreTest(ProbackupTest, unittest.TestCase): # 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 @@ class RestoreTest(ProbackupTest, unittest.TestCase): 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') -# 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)) + timeline_id = node.safe_psql( + 'postgres', + 'select timeline_id from pg_control_checkpoint()').rstrip() + + self.assertEqual('2', timeline_id) # Clean after yourself self.del_test_dir(module_name, fname) \ No newline at end of file