[Issue #238] in PG>=12 use postgresql.auto.conf for recovery parameters

src/pg_probackup.c

@@ -99,7 +99,7 @@ static pgRecoveryTarget *recovery_target_options = NULL;
 static pgRestoreParams *restore_params = NULL;
 
 time_t current_time = 0;
-bool restore_as_replica = false;
+static bool restore_as_replica = false;
 bool no_validate = false;
 IncrRestoreMode incremental_mode = INCR_NONE;
 
@@ -705,15 +705,14 @@ main(int argc, char *argv[])
 		if (force)
 			no_validate = true;
 
-		if (replication_slot != NULL)
-			restore_as_replica = true;
-
 		/* keep all params in one structure */
 		restore_params = pgut_new(pgRestoreParams);
 		restore_params->is_restore = (backup_subcmd == RESTORE_CMD);
 		restore_params->force = force;
 		restore_params->no_validate = no_validate;
 		restore_params->restore_as_replica = restore_as_replica;
+		restore_params->recovery_settings_mode = DEFAULT;
+
 		restore_params->primary_slot_name = replication_slot;
 		restore_params->skip_block_validation = skip_block_validation;
 		restore_params->skip_external_dirs = skip_external_dirs;

src/pg_probackup.h

@@ -146,6 +146,16 @@ typedef enum PartialRestoreType
 	EXCLUDE,
 } PartialRestoreType;
 
+typedef enum RecoverySettingsMode
+{
+	DEFAULT,	/* not set */
+	DONTWRITE,	/* explicitly forbid to update recovery settings */
+				//TODO Should we always clean/preserve old recovery settings,
+				// or make it configurable?
+	PITR_REQUESTED, /* can be set based on other parameters
+	                 * if not explicitly forbidden */
+} RecoverySettingsMode;
+
 typedef enum CompressAlg
 {
 	NOT_DEFINED_COMPRESS = 0,
@@ -490,6 +500,8 @@ typedef struct pgRestoreParams
 	bool	is_restore;
 	bool	no_validate;
 	bool	restore_as_replica;
+	//TODO maybe somehow add restore_as_replica as one of RecoverySettingsModes
+	RecoverySettingsMode recovery_settings_mode;
 	bool	skip_external_dirs;
 	bool	skip_block_validation; //Start using it
 	const char *restore_command;

src/restore.c

@@ -39,13 +39,29 @@ typedef struct
 	int			ret;
 } restore_files_arg;
 
+
+static void
+print_recovery_settings(FILE *fp, pgBackup *backup,
+							   pgRestoreParams *params, pgRecoveryTarget *rt);
+static void
+print_standby_settings_common(FILE *fp, pgBackup *backup, pgRestoreParams *params);
+
+#if PG_VERSION_NUM >= 120000
+static void
+update_recovery_options(pgBackup *backup,
+						pgRestoreParams *params, pgRecoveryTarget *rt);
+#else
+static void
+update_recovery_options_before_v12(pgBackup *backup,
+								   pgRestoreParams *params, pgRecoveryTarget *rt);
+#endif
+
 static void create_recovery_conf(time_t backup_id,
 								 pgRecoveryTarget *rt,
 								 pgBackup *backup,
 								 pgRestoreParams *params);
 static void *restore_files(void *arg);
 static void set_orphan_status(parray *backups, pgBackup *parent_backup);
-static void pg12_recovery_config(pgBackup *backup, bool add_include);
 
 static void restore_chain(pgBackup *dest_backup, parray *parent_chain,
 						  parray *dbOid_exclude_list, pgRestoreParams *params,
@@ -597,6 +613,7 @@ do_restore_or_validate(time_t target_backup_id, pgRecoveryTarget *rt,
 		restore_chain(dest_backup, parent_chain, dbOid_exclude_list,
 							params, instance_config.pgdata, no_sync);
 
+		//TODO rename and update comment
 		/* Create recovery.conf with given recovery target parameters */
 		create_recovery_conf(target_backup_id, rt, dest_backup, params);
 	}
@@ -1204,13 +1221,9 @@ create_recovery_conf(time_t backup_id,
 					 pgBackup *backup,
 					 pgRestoreParams *params)
 {
-	char		path[MAXPGPATH];
-	FILE	   *fp;
-	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))
@@ -1242,36 +1255,132 @@ create_recovery_conf(time_t backup_id,
 	 * 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 ||
+	if (!backup->stream || rt->time_string ||
 		rt->xid_string || rt->lsn_string || rt->target_name ||
-		target_immediate || target_latest || restore_command_provided;
+		target_immediate || target_latest || restore_command_provided)
-
+		params->recovery_settings_mode = PITR_REQUESTED;
-	/* No need to generate recovery.conf at all. */
-	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;
-	}
 
 	elog(LOG, "----------------------------------------");
+
 #if PG_VERSION_NUM >= 120000
-	elog(LOG, "creating probackup_recovery.conf");
+	update_recovery_options(backup, params, rt);
-	pg12_recovery_config(backup, true);
-	snprintf(path, lengthof(path), "%s/probackup_recovery.conf", instance_config.pgdata);
 #else
-	elog(LOG, "creating recovery.conf");
+	update_recovery_options_before_v12(backup, params, rt);
-	snprintf(path, lengthof(path), "%s/recovery.conf", instance_config.pgdata);
 #endif
+}
+
+
+/* TODO get rid of using global variables: instance_config, backup_path, instance_name */
+static void
+print_recovery_settings(FILE *fp, pgBackup *backup,
+							   pgRestoreParams *params, pgRecoveryTarget *rt)
+{
+	char restore_command_guc[16384];
+	fio_fprintf(fp, "## recovery settings\n");
+
+	/* If restore_command is provided, use it. Otherwise construct it from scratch. */
+	if (instance_config.restore_command &&
+		(pg_strcasecmp(instance_config.restore_command, "none") != 0))
+		sprintf(restore_command_guc, "%s", instance_config.restore_command);
+	else
+	{
+		/* default cmdline, ok for local restore */
+		sprintf(restore_command_guc, "%s archive-get -B %s --instance %s "
+				"--wal-file-path=%%p --wal-file-name=%%f",
+				PROGRAM_FULL_PATH ? PROGRAM_FULL_PATH : PROGRAM_NAME,
+				backup_path, instance_name);
+
+		/* append --remote-* parameters provided via --archive-* settings */
+		if (instance_config.archive.host)
+		{
+			strcat(restore_command_guc, " --remote-host=");
+			strcat(restore_command_guc, instance_config.archive.host);
+		}
+
+		if (instance_config.archive.port)
+		{
+			strcat(restore_command_guc, " --remote-port=");
+			strcat(restore_command_guc, instance_config.archive.port);
+		}
+
+		if (instance_config.archive.user)
+		{
+			strcat(restore_command_guc, " --remote-user=");
+			strcat(restore_command_guc, instance_config.archive.user);
+		}
+	}
+
+	/*
+	 * We've already checked that only one of the four following mutually
+	 * exclusive options is specified, so the order of calls is insignificant.
+	 */
+	if (rt->target_name)
+		fio_fprintf(fp, "recovery_target_name = '%s'\n", rt->target_name);
+
+	if (rt->time_string)
+		fio_fprintf(fp, "recovery_target_time = '%s'\n", rt->time_string);
+
+	if (rt->xid_string)
+		fio_fprintf(fp, "recovery_target_xid = '%s'\n", rt->xid_string);
+
+	if (rt->lsn_string)
+		fio_fprintf(fp, "recovery_target_lsn = '%s'\n", rt->lsn_string);
+
+	if (rt->target_stop && (strcmp(rt->target_stop, "immediate") == 0))
+		fio_fprintf(fp, "recovery_target = '%s'\n", rt->target_stop);
+
+	if (rt->inclusive_specified)
+		fio_fprintf(fp, "recovery_target_inclusive = '%s'\n",
+				rt->target_inclusive ? "true" : "false");
+
+	if (rt->target_tli)
+		fio_fprintf(fp, "recovery_target_timeline = '%u'\n", rt->target_tli);
+	else
+	{
+#if PG_VERSION_NUM >= 120000
+
+			/*
+			 * In PG12 default recovery target timeline was changed to 'latest', which
+			 * is extremely risky. Explicitly preserve old behavior of recovering to current
+			 * timneline for PG12.
+			 */
+			fio_fprintf(fp, "recovery_target_timeline = 'current'\n");
+#endif
+	}
+
+	if (rt->target_action)
+		fio_fprintf(fp, "recovery_target_action = '%s'\n", rt->target_action);
+	else
+		/* default recovery_target_action is 'pause' */
+		fio_fprintf(fp, "recovery_target_action = '%s'\n", "pause");
+
+	elog(LOG, "Setting restore_command to '%s'", restore_command_guc);
+	fio_fprintf(fp, "restore_command = '%s'\n", restore_command_guc);
+}
+
+static void
+print_standby_settings_common(FILE *fp, pgBackup *backup, pgRestoreParams *params)
+{
+	fio_fprintf(fp, "\n## standby settings\n");
+	if (params->primary_conninfo)
+		fio_fprintf(fp, "primary_conninfo = '%s'\n", params->primary_conninfo);
+	else if (backup->primary_conninfo)
+		fio_fprintf(fp, "primary_conninfo = '%s'\n", backup->primary_conninfo);
+
+	if (params->primary_slot_name != NULL)
+		fio_fprintf(fp, "primary_slot_name = '%s'\n", params->primary_slot_name);
+}
+
+#if PG_VERSION_NUM < 120000
+static void
+update_recovery_options_before_v12(pgBackup *backup,
+								   pgRestoreParams *params, pgRecoveryTarget *rt)
+{
+	FILE	   *fp;
+	char		path[MAXPGPATH];
+
+	elog(LOG, "update recovery settings in recovery.conf");
+	snprintf(path, lengthof(path), "%s/recovery.conf", instance_config.pgdata);
 
 	fp = fio_fopen(path, "w", FIO_DB_HOST);
 	if (fp == NULL)
@@ -1281,226 +1390,188 @@ create_recovery_conf(time_t backup_id,
 	if (fio_chmod(path, FILE_PERMISSION, FIO_DB_HOST) == -1)
 		elog(ERROR, "Cannot change mode of \"%s\": %s", path, strerror(errno));
 
-#if PG_VERSION_NUM >= 120000
-	fio_fprintf(fp, "# probackup_recovery.conf generated by pg_probackup %s\n",
-				PROGRAM_VERSION);
-#else
 	fio_fprintf(fp, "# recovery.conf generated by pg_probackup %s\n",
 				PROGRAM_VERSION);
-#endif
 
-	/* construct restore_command */
+	if (params->recovery_settings_mode == PITR_REQUESTED)
-	if (pitr_requested)
+		print_recovery_settings(fp, backup, params, rt);
-	{
-		fio_fprintf(fp, "\n## recovery settings\n");
-		/* 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
-		{
-			/* default cmdline, ok for local restore */
-			sprintf(restore_command_guc, "%s archive-get -B %s --instance %s "
-					"--wal-file-path=%%p --wal-file-name=%%f",
-					PROGRAM_FULL_PATH ? PROGRAM_FULL_PATH : PROGRAM_NAME,
-					backup_path, instance_name);
-
-			/* append --remote-* parameters provided via --archive-* settings */
-			if (instance_config.archive.host)
-			{
-				strcat(restore_command_guc, " --remote-host=");
-				strcat(restore_command_guc, instance_config.archive.host);
-			}
-
-			if (instance_config.archive.port)
-			{
-				strcat(restore_command_guc, " --remote-port=");
-				strcat(restore_command_guc, instance_config.archive.port);
-			}
-
-			if (instance_config.archive.user)
-			{
-				strcat(restore_command_guc, " --remote-user=");
-				strcat(restore_command_guc, instance_config.archive.user);
-			}
-		}
-
-		/*
-		 * We've already checked that only one of the four following mutually
-		 * exclusive options is specified, so the order of calls is insignificant.
-		 */
-		if (rt->target_name)
-			fio_fprintf(fp, "recovery_target_name = '%s'\n", rt->target_name);
-
-		if (rt->time_string)
-			fio_fprintf(fp, "recovery_target_time = '%s'\n", rt->time_string);
-
-		if (rt->xid_string)
-			fio_fprintf(fp, "recovery_target_xid = '%s'\n", rt->xid_string);
-
-		if (rt->lsn_string)
-			fio_fprintf(fp, "recovery_target_lsn = '%s'\n", rt->lsn_string);
-
-		if (rt->target_stop && target_immediate)
-			fio_fprintf(fp, "recovery_target = '%s'\n", rt->target_stop);
-
-		if (rt->inclusive_specified)
-			fio_fprintf(fp, "recovery_target_inclusive = '%s'\n",
-					rt->target_inclusive ? "true" : "false");
-
-		if (rt->target_tli)
-			fio_fprintf(fp, "recovery_target_timeline = '%u'\n", rt->target_tli);
-		else
-		{
-			/*
-			 * In PG12 default recovery target timeline was changed to 'latest', which
-			 * is extremely risky. Explicitly preserve old behavior of recovering to current
-			 * timneline for PG12.
-			 */
-#if PG_VERSION_NUM >= 120000
-			fio_fprintf(fp, "recovery_target_timeline = 'current'\n");
-#endif
-		}
-
-		if (rt->target_action)
-			fio_fprintf(fp, "recovery_target_action = '%s'\n", rt->target_action);
-		else
-			/* default recovery_target_action is 'pause' */
-			fio_fprintf(fp, "recovery_target_action = '%s'\n", "pause");
-	}
-
-	if (pitr_requested)
-	{
-		elog(LOG, "Setting restore_command to '%s'", restore_command_guc);
-		fio_fprintf(fp, "restore_command = '%s'\n", restore_command_guc);
-	}
 
 	if (params->restore_as_replica)
 	{
-		fio_fprintf(fp, "\n## standby settings\n");
+		print_standby_settings_common(fp, backup, params);
-	/* standby_mode was removed in PG12 */
-#if PG_VERSION_NUM < 120000
 		fio_fprintf(fp, "standby_mode = 'on'\n");
-#endif
-
-		if (params->primary_conninfo)
-			fio_fprintf(fp, "primary_conninfo = '%s'\n", params->primary_conninfo);
-		else if (backup->primary_conninfo)
-			fio_fprintf(fp, "primary_conninfo = '%s'\n", backup->primary_conninfo);
-
-		if (params->primary_slot_name != NULL)
-			fio_fprintf(fp, "primary_slot_name = '%s'\n", params->primary_slot_name);
 	}
 
 	if (fio_fflush(fp) != 0 ||
 		fio_fclose(fp))
 		elog(ERROR, "cannot write file \"%s\": %s", path,
 			 strerror(errno));
-
-#if PG_VERSION_NUM >= 120000
-	/*
-	 * 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);
-
-		fp = fio_fopen(path, "w", FIO_DB_HOST);
-		if (fp == NULL)
-			elog(ERROR, "cannot open file \"%s\": %s", path,
-				strerror(errno));
-
-		if (fio_fflush(fp) != 0 ||
-			fio_fclose(fp))
-			elog(ERROR, "cannot write file \"%s\": %s", path,
-				 strerror(errno));
-	}
-
-	if (params->restore_as_replica)
-	{
-		elog(LOG, "creating standby.signal file");
-		snprintf(path, lengthof(path), "%s/standby.signal", instance_config.pgdata);
-
-		fp = fio_fopen(path, "w", FIO_DB_HOST);
-		if (fp == NULL)
-			elog(ERROR, "cannot open file \"%s\": %s", path,
-				strerror(errno));
-
-		if (fio_fflush(fp) != 0 ||
-			fio_fclose(fp))
-			elog(ERROR, "cannot write file \"%s\": %s", path,
-				 strerror(errno));
-	}
-#endif
 }
+#endif
 
 /*
- * Create empty probackup_recovery.conf in PGDATA and
+ * Read postgresql.auto.conf, clean old recovery options,
- * add "include" directive to postgresql.auto.conf
+ * to avoid unexpected intersections.
-
+ * Write recovery options for this backup.
- * When restoring PG12 we always(!) must do this, even
- * when restoring STREAM backup without PITR or replica options
- * because restored instance may have been previously backed up
- * and restored again and user didn`t cleaned up postgresql.auto.conf.
-
- * So for recovery to work regardless of all this factors
- * we must always create empty probackup_recovery.conf file.
  */
-static void
-pg12_recovery_config(pgBackup *backup, bool add_include)
-{
 #if PG_VERSION_NUM >= 120000
-	char		probackup_recovery_path[MAXPGPATH];
+static void
+update_recovery_options(pgBackup *backup,
+						pgRestoreParams *params, pgRecoveryTarget *rt)
+
+{
 	char		postgres_auto_path[MAXPGPATH];
+	char		postgres_auto_path_tmp[MAXPGPATH];
+	char		path[MAXPGPATH];
 	FILE	   *fp;
+	FILE	   *fp_tmp;
+	struct stat st;
+	char		current_time_str[100];
+	/* postgresql.auto.conf parsing */
+	char		line[16384] = "\0";
+	char	   *buf = NULL;
+	int		    buf_len = 0;
+	int		    buf_len_max = 16384;
 
-	if (add_include)
+	elog(LOG, "update recovery settings in postgresql.auto.conf");
+
+	time2iso(current_time_str, lengthof(current_time_str), current_time, false);
+
+	snprintf(postgres_auto_path, lengthof(postgres_auto_path),
+				"%s/postgresql.auto.conf", instance_config.pgdata);
+
+	if (fio_stat(postgres_auto_path, &st, false, FIO_DB_HOST) < 0)
 	{
-		char		current_time_str[100];
+		/* file not found is not an error case */
+		if (errno != ENOENT)
+			elog(ERROR, "cannot stat file \"%s\": %s", postgres_auto_path,
+				 strerror(errno));
+	}
 
-		time2iso(current_time_str, lengthof(current_time_str), current_time, false);
+	fp = fio_open_stream(postgres_auto_path, FIO_DB_HOST);
+	if (fp == NULL && errno != ENOENT)
+		elog(ERROR, "cannot open \"%s\": %s", postgres_auto_path, strerror(errno));
 
-		snprintf(postgres_auto_path, lengthof(postgres_auto_path),
+	sprintf(postgres_auto_path_tmp, "%s.tmp", postgres_auto_path);
-					"%s/postgresql.auto.conf", instance_config.pgdata);
+	fp_tmp = fio_fopen(postgres_auto_path_tmp, "w", FIO_DB_HOST);
+	if (fp_tmp == NULL)
+		elog(ERROR, "cannot open \"%s\": %s", postgres_auto_path_tmp, strerror(errno));
 
+	while (fp && fgets(line, lengthof(line), fp))
+	{
+		/* ignore "include 'probackup_recovery.conf'" directive */
+		if (strstr(line, "include") &&
+			strstr(line, "probackup_recovery.conf"))
+		{
+			continue;
+		}
+
+		/* ignore already existing recovery options */
+		if (strstr(line, "restore_command") ||
+			strstr(line, "recovery_target"))
+		{
+			continue;
+		}
+
+		if (!buf)
+			buf = pgut_malloc(buf_len_max);
+
+		/* avoid buffer overflow */
+		if ((buf_len + strlen(line)) >= buf_len_max)
+		{
+			buf_len_max += (buf_len + strlen(line)) *2;
+			buf = pgut_realloc(buf, buf_len_max);
+		}
+
+		buf_len += snprintf(buf+buf_len, sizeof(line), "%s", line);
+	}
+
+	/* close input postgresql.auto.conf */
+	if (fp)
+		fio_close_stream(fp);
+
+	/* TODO: detect remote error */
+	if (buf_len > 0)
+		fio_fwrite(fp_tmp, buf, buf_len);
+
+	if (fio_fflush(fp_tmp) != 0 ||
+		fio_fclose(fp_tmp))
+			elog(ERROR, "Cannot write file \"%s\": %s", postgres_auto_path_tmp,
+				strerror(errno));
+	pg_free(buf);
+
+	if (fio_rename(postgres_auto_path_tmp, postgres_auto_path, FIO_DB_HOST) < 0)
+		elog(ERROR, "Cannot rename file \"%s\" to \"%s\": %s",
+					postgres_auto_path_tmp, postgres_auto_path, strerror(errno));
+
+	if (fio_chmod(postgres_auto_path, FILE_PERMISSION, FIO_DB_HOST) == -1)
+		elog(ERROR, "Cannot change mode of \"%s\": %s", postgres_auto_path, strerror(errno));
+
+	if (params)
+	{
 		fp = fio_fopen(postgres_auto_path, "a", FIO_DB_HOST);
 		if (fp == NULL)
-			elog(ERROR, "cannot write to file \"%s\": %s", postgres_auto_path,
+			elog(ERROR, "cannot open file \"%s\": %s", postgres_auto_path,
 				strerror(errno));
 
-		// TODO: check if include 'probackup_recovery.conf' already exists
+		fio_fprintf(fp, "\n# recovery settings added by pg_probackup restore of backup %s at '%s'\n",
-		fio_fprintf(fp, "\n# created by pg_probackup restore of backup %s at '%s'\n",
+				base36enc(backup->start_time), current_time_str);
-			base36enc(backup->start_time), current_time_str);
+
-		fio_fprintf(fp, "include '%s'\n", "probackup_recovery.conf");
+		if (params->recovery_settings_mode == PITR_REQUESTED)
+			print_recovery_settings(fp, backup, params, rt);
+
+		if (params->restore_as_replica)
+			print_standby_settings_common(fp, backup, params);
 
 		if (fio_fflush(fp) != 0 ||
 			fio_fclose(fp))
-			elog(ERROR, "cannot write to file \"%s\": %s", postgres_auto_path,
+			elog(ERROR, "cannot write file \"%s\": %s", postgres_auto_path,
 				strerror(errno));
+
+		/*
+		* 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 (params->recovery_settings_mode == PITR_REQUESTED)
+		{
+			elog(LOG, "creating recovery.signal file");
+			snprintf(path, lengthof(path), "%s/recovery.signal", instance_config.pgdata);
+
+			fp = fio_fopen(path, PG_BINARY_W, FIO_DB_HOST);
+			if (fp == NULL)
+				elog(ERROR, "cannot open file \"%s\": %s", path,
+					strerror(errno));
+
+			if (fio_fflush(fp) != 0 ||
+				fio_fclose(fp))
+				elog(ERROR, "cannot write file \"%s\": %s", path,
+					strerror(errno));
+		}
+
+		if (params->restore_as_replica)
+		{
+			elog(LOG, "creating standby.signal file");
+			snprintf(path, lengthof(path), "%s/standby.signal", instance_config.pgdata);
+
+			fp = fio_fopen(path, PG_BINARY_W, FIO_DB_HOST);
+			if (fp == NULL)
+				elog(ERROR, "cannot open file \"%s\": %s", path,
+					strerror(errno));
+
+			if (fio_fflush(fp) != 0 ||
+				fio_fclose(fp))
+				elog(ERROR, "cannot write file \"%s\": %s", path,
+					strerror(errno));
+		}
 	}
-
-	/* Create empty probackup_recovery.conf */
-	snprintf(probackup_recovery_path, lengthof(probackup_recovery_path),
-		"%s/probackup_recovery.conf", instance_config.pgdata);
-	fp = fio_fopen(probackup_recovery_path, "w", FIO_DB_HOST);
-	if (fp == NULL)
-		elog(ERROR, "cannot open file \"%s\": %s", probackup_recovery_path,
-			strerror(errno));
-
-	if (fio_fflush(fp) != 0 ||
-		fio_fclose(fp))
-		elog(ERROR, "cannot write to file \"%s\": %s", probackup_recovery_path,
-			 strerror(errno));
-#endif
-	return;
 }
+#endif
 
 /*
  * Try to read a timeline's history file.

tests/helpers/ptrack_helpers.py

@@ -246,18 +246,6 @@ class ProbackupTest(object):
             print('pg_probackup binary is not found')
             exit(1)
 
-        self.probackup_version = None
-
-        try:
-            self.probackup_version_output = subprocess.check_output(
-                [self.probackup_path, "--version"],
-                stderr=subprocess.STDOUT,
-                ).decode('utf-8')
-        except subprocess.CalledProcessError as e:
-            raise ProbackupException(e.output.decode('utf-8'))
-
-        self.probackup_version = re.search(r"\d+\.\d+\.\d+", self.probackup_version_output).group(0)
-
         if os.name == 'posix':
             self.EXTERNAL_DIRECTORY_DELIMITER = ':'
             os.environ['PATH'] = os.path.dirname(
@@ -280,6 +268,32 @@ class ProbackupTest(object):
                 if self.verbose:
                     print('PGPROBACKUPBIN_OLD is not an executable file')
 
+        self.probackup_version = None
+        self.old_probackup_version = None
+
+        try:
+            self.probackup_version_output = subprocess.check_output(
+                [self.probackup_path, "--version"],
+                stderr=subprocess.STDOUT,
+                ).decode('utf-8')
+        except subprocess.CalledProcessError as e:
+            raise ProbackupException(e.output.decode('utf-8'))
+
+        if self.probackup_old_path:
+            old_probackup_version_output = subprocess.check_output(
+                [self.probackup_old_path, "--version"],
+                stderr=subprocess.STDOUT,
+                ).decode('utf-8')
+            self.old_probackup_version = re.search(
+                r"\d+\.\d+\.\d+",
+                subprocess.check_output(
+                    [self.probackup_old_path, "--version"],
+                    stderr=subprocess.STDOUT,
+                    ).decode('utf-8')
+                ).group(0)
+
+        self.probackup_version = re.search(r"\d+\.\d+\.\d+", self.probackup_version_output).group(0)
+
         self.remote = False
         self.remote_host = None
         self.remote_port = None
@@ -1142,8 +1156,9 @@ class ProbackupTest(object):
         out_dict = {}
 
         if self.get_version(node) >= self.version_to_num('12.0'):
-            recovery_conf_path = os.path.join(
+            recovery_conf_path = os.path.join(node.data_dir, 'postgresql.auto.conf')
-                node.data_dir, 'probackup_recovery.conf')
+            with open(recovery_conf_path, 'r') as f:
+                print(f.read())
         else:
             recovery_conf_path = os.path.join(node.data_dir, 'recovery.conf')
 

tests/restore.py

@@ -51,8 +51,11 @@ class RestoreTest(ProbackupTest, unittest.TestCase):
                 repr(self.output), self.cmd))
 
         # 2 - Test that recovery.conf was created
+        # TODO update test
         if self.get_version(node) >= self.version_to_num('12.0'):
-            recovery_conf = os.path.join(node.data_dir, 'probackup_recovery.conf')
+            recovery_conf = os.path.join(node.data_dir, 'postgresql.auto.conf')
+            with open(recovery_conf, 'r') as f:
+                print(f.read())
         else:
             recovery_conf = os.path.join(node.data_dir, 'recovery.conf')
         self.assertEqual(os.path.isfile(recovery_conf), True)
@@ -1807,8 +1810,11 @@ class RestoreTest(ProbackupTest, unittest.TestCase):
 
         pgdata = self.pgdata_content(node.data_dir)
 
+        # TODO update test
         if self.get_version(node) >= self.version_to_num('12.0'):
-            recovery_conf = os.path.join(node.data_dir, 'probackup_recovery.conf')
+            recovery_conf = os.path.join(node.data_dir, 'postgresql.auto.conf')
+            with open(recovery_conf, 'r') as f:
+                print(f.read())
         else:
             recovery_conf = os.path.join(node.data_dir, 'recovery.conf')
 
@@ -1862,8 +1868,11 @@ class RestoreTest(ProbackupTest, unittest.TestCase):
 
         pgdata = self.pgdata_content(node.data_dir)
 
+        # TODO update test
         if self.get_version(node) >= self.version_to_num('12.0'):
-            recovery_conf = os.path.join(node.data_dir, 'probackup_recovery.conf')
+            recovery_conf = os.path.join(node.data_dir, 'postgresql.auto.conf')
+            with open(recovery_conf, 'r') as f:
+                print(f.read())
         else:
             recovery_conf = os.path.join(node.data_dir, 'recovery.conf')
 
@@ -1912,7 +1921,7 @@ class RestoreTest(ProbackupTest, unittest.TestCase):
         self.backup_node(backup_dir, 'node', node)
 
         if self.get_version(node) >= self.version_to_num('12.0'):
-            recovery_conf = os.path.join(node.data_dir, 'probackup_recovery.conf')
+            recovery_conf = os.path.join(node.data_dir, 'postgresql.auto.conf')
         else:
             recovery_conf = os.path.join(node.data_dir, 'recovery.conf')
 
@@ -1924,23 +1933,35 @@ class RestoreTest(ProbackupTest, unittest.TestCase):
         #     open(recovery_conf, 'rb').read()).hexdigest()
 
         with open(recovery_conf, 'r') as f:
-            content_1 = f.read()
+            content_1 = ''
+            while True:
+                line = f.readline()
+
+                if not line:
+                    break
+                if line.startswith("#"):
+                    continue
+                content_1 += line
 
-        # restore
         node.cleanup()
-
         self.restore_node(backup_dir, 'node', node, options=['--recovery-target=latest'])
 
         # hash_2 = hashlib.md5(
         #     open(recovery_conf, 'rb').read()).hexdigest()
 
         with open(recovery_conf, 'r') as f:
-            content_2 = f.read()
+            content_2 = ''
+            while True:
+                line = f.readline()
+
+                if not line:
+                    break
+                if line.startswith("#"):
+                    continue
+                content_2 += line
 
         self.assertEqual(content_1, content_2)
 
-        # self.assertEqual(hash_1, hash_2)
-
         # Clean after yourself
         self.del_test_dir(module_name, fname)
 
@@ -1965,8 +1986,11 @@ class RestoreTest(ProbackupTest, unittest.TestCase):
         # Take FULL
         self.backup_node(backup_dir, 'node', node)
 
+        # TODO update test
         if self.get_version(node) >= self.version_to_num('12.0'):
-            recovery_conf = os.path.join(node.data_dir, 'probackup_recovery.conf')
+            recovery_conf = os.path.join(node.data_dir, 'postgresql.auto.conf')
+            with open(recovery_conf, 'r') as f:
+                print(f.read())
         else:
             recovery_conf = os.path.join(node.data_dir, 'recovery.conf')
 
@@ -3264,8 +3288,11 @@ class RestoreTest(ProbackupTest, unittest.TestCase):
         self.set_archiving(backup_dir, 'node', node)
         node.slow_start()
 
+        # TODO update test
         if self.get_version(node) >= self.version_to_num('12.0'):
-            recovery_conf = os.path.join(node.data_dir, 'probackup_recovery.conf')
+            recovery_conf = os.path.join(node.data_dir, 'postgresql.auto.conf')
+            with open(recovery_conf, 'r') as f:
+                print(f.read())
         else:
             recovery_conf = os.path.join(node.data_dir, 'recovery.conf')
 
@@ -3352,8 +3379,11 @@ class RestoreTest(ProbackupTest, unittest.TestCase):
                 os.path.isfile(standby_signal),
                 "File '{0}' do not exists".format(standby_signal))
 
+        # TODO update test
         if self.get_version(node) >= self.version_to_num('12.0'):
-            recovery_conf = os.path.join(replica.data_dir, 'probackup_recovery.conf')
+            recovery_conf = os.path.join(replica.data_dir, 'postgresql.auto.conf')
+            with open(recovery_conf, 'r') as f:
+                print(f.read())
         else:
             recovery_conf = os.path.join(replica.data_dir, 'recovery.conf')
 
@@ -3479,3 +3509,169 @@ class RestoreTest(ProbackupTest, unittest.TestCase):
 
         # Clean after yourself
         self.del_test_dir(module_name, fname)
+
+    def test_pg_12_probackup_recovery_conf_compatibility(self):
+        """
+        https://github.com/postgrespro/pg_probackup/issues/249
+
+        pg_probackup version must be 12 or greater
+        """
+
+        if self.old_probackup_version:
+            if self.version_to_num(self.old_probackup_version) >= self.version_to_num('2.4.5'):
+                return unittest.skip('You need pg_probackup < 2.4.5 for this test')
+
+        if self.pg_config_version < self.version_to_num('12.0'):
+           return unittest.skip('You need PostgreSQL >= 12 for this test')
+
+        fname = self.id().split('.')[3]
+        backup_dir = os.path.join(self.tmp_path, module_name, fname, 'backup')
+        node = self.make_simple_node(
+            base_dir=os.path.join(module_name, fname, 'node'),
+            initdb_params=['--data-checksums'],
+            pg_options={'autovacuum': 'off'})
+
+        self.init_pb(backup_dir)
+        self.add_instance(backup_dir, 'node', node)
+        self.set_archiving(backup_dir, 'node', node)
+        node.slow_start()
+
+        # FULL backup
+        self.backup_node(backup_dir, 'node', node, old_binary=True)
+
+        node.pgbench_init(scale=5)
+
+        node.safe_psql(
+            'postgres',
+            'CREATE TABLE t1 as SELECT * from pgbench_accounts where aid > 200000 and aid < 450000')
+
+        time = node.safe_psql(
+            'SELECT current_timestamp(0)::timestamptz;').decode('utf-8').rstrip()
+
+        node.safe_psql(
+            'postgres',
+            'DELETE from pgbench_accounts where aid > 200000 and aid < 450000')
+
+        node.cleanup()
+
+        self.restore_node(
+            backup_dir, 'node',node,
+            options=[
+                    "--recovery-target-time={0}".format(time),
+                    "--recovery-target-action=promote"],
+            old_binary=True)
+
+        node.slow_start()
+
+        self.backup_node(backup_dir, 'node', node, old_binary=True)
+
+        node.pgbench_init(scale=5)
+
+        xid = node.safe_psql(
+            'SELECT txid_current()').decode('utf-8').rstrip()
+        node.pgbench_init(scale=1)
+
+        node.cleanup()
+
+        self.restore_node(
+            backup_dir, 'node',node,
+            options=[
+                    "--recovery-target-xid={0}".format(xid),
+                    "--recovery-target-action=promote"])
+
+        node.slow_start()
+
+        # Clean after yourself
+        self.del_test_dir(module_name, fname)
+
+    def test_drop_postgresql_auto_conf(self):
+        """
+        https://github.com/postgrespro/pg_probackup/issues/249
+
+        pg_probackup version must be 12 or greater
+        """
+
+        if self.pg_config_version < self.version_to_num('12.0'):
+           return unittest.skip('You need PostgreSQL >= 12 for this test')
+
+        fname = self.id().split('.')[3]
+        backup_dir = os.path.join(self.tmp_path, module_name, fname, 'backup')
+        node = self.make_simple_node(
+            base_dir=os.path.join(module_name, fname, 'node'),
+            initdb_params=['--data-checksums'],
+            pg_options={'autovacuum': 'off'})
+
+        self.init_pb(backup_dir)
+        self.add_instance(backup_dir, 'node', node)
+        self.set_archiving(backup_dir, 'node', node)
+        node.slow_start()
+
+        # FULL backup
+        self.backup_node(backup_dir, 'node', node)
+
+        # drop postgresql.auto.conf
+        auto_path = os.path.join(node.data_dir, "postgresql.auto.conf")
+        os.remove(auto_path)
+
+        self.backup_node(backup_dir, 'node', node, backup_type='page')
+
+        node.cleanup()
+
+        self.restore_node(
+            backup_dir, 'node',node,
+            options=[
+                    "--recovery-target=latest",
+                    "--recovery-target-action=promote"])
+
+        node.slow_start()
+
+        self.assertTrue(os.path.exists(auto_path))
+
+        # Clean after yourself
+        self.del_test_dir(module_name, fname)
+
+    def test_truncate_postgresql_auto_conf(self):
+        """
+        https://github.com/postgrespro/pg_probackup/issues/249
+
+        pg_probackup version must be 12 or greater
+        """
+
+        if self.pg_config_version < self.version_to_num('12.0'):
+           return unittest.skip('You need PostgreSQL >= 12 for this test')
+
+        fname = self.id().split('.')[3]
+        backup_dir = os.path.join(self.tmp_path, module_name, fname, 'backup')
+        node = self.make_simple_node(
+            base_dir=os.path.join(module_name, fname, 'node'),
+            initdb_params=['--data-checksums'],
+            pg_options={'autovacuum': 'off'})
+
+        self.init_pb(backup_dir)
+        self.add_instance(backup_dir, 'node', node)
+        self.set_archiving(backup_dir, 'node', node)
+        node.slow_start()
+
+        # FULL backup
+        self.backup_node(backup_dir, 'node', node)
+
+        # truncate postgresql.auto.conf
+        auto_path = os.path.join(node.data_dir, "postgresql.auto.conf")
+        with open(auto_path, "w+") as f:
+            f.truncate()
+
+        self.backup_node(backup_dir, 'node', node, backup_type='page')
+
+        node.cleanup()
+
+        self.restore_node(
+            backup_dir, 'node',node,
+            options=[
+                    "--recovery-target=latest",
+                    "--recovery-target-action=promote"])
+        node.slow_start()
+
+        self.assertTrue(os.path.exists(auto_path))
+
+        # Clean after yourself
+        self.del_test_dir(module_name, fname)