[PBCKP-259] fix for 'ERROR: Cannot create directory for older backup'… (#526)

* [PBCKP-259] fix for 'ERROR: Cannot create directory for older backup', rewrite --start_time implementation
* rewritten 5f2283c
* fixes for several tests
* disabled tests.merge.MergeTest.test_merge_backup_from_future and tests.restore.RestoreTest.test_restore_backup_from_future as incorrect for now

Co-authored-by: d.lepikhova <d.lepikhova@postgrespro.ru>

Author: kulaginm

src/backup.c

@@ -692,15 +692,22 @@ pgdata_basic_setup(ConnectionOptions conn_opt, PGNodeInfo *nodeInfo)
 
 /*
  * Entry point of pg_probackup BACKUP subcommand.
+ *
+ * if start_time == INVALID_BACKUP_ID then we can generate backup_id
  */
 int
 do_backup(InstanceState *instanceState, pgSetBackupParams *set_backup_params,
 		  bool no_validate, bool no_sync, bool backup_logs, time_t start_time)
 {
 	PGconn		*backup_conn = NULL;
 	PGNodeInfo	nodeInfo;
+	time_t		latest_backup_id = INVALID_BACKUP_ID;
 	char		pretty_bytes[20];
 
+	if (!instance_config.pgdata)
+		elog(ERROR, "required parameter not specified: PGDATA "
+						 "(-D, --pgdata)");
+
 	/* Initialize PGInfonode */
 	pgNodeInit(&nodeInfo);
 
@@ -709,12 +716,55 @@ do_backup(InstanceState *instanceState, pgSetBackupParams *set_backup_params,
 		(pg_strcasecmp(instance_config.external_dir_str, "none") != 0))
 		current.external_dir_str = instance_config.external_dir_str;
 
-	/* Create backup directory and BACKUP_CONTROL_FILE */
-	pgBackupCreateDir(&current, instanceState, start_time);
+	/* Find latest backup_id */
+	{
+		parray	*backup_list =  catalog_get_backup_list(instanceState, INVALID_BACKUP_ID);
 
-	if (!instance_config.pgdata)
-		elog(ERROR, "required parameter not specified: PGDATA "
-						 "(-D, --pgdata)");
+		if (parray_num(backup_list) > 0)
+			latest_backup_id = ((pgBackup *)parray_get(backup_list, 0))->backup_id;
+
+		parray_walk(backup_list, pgBackupFree);
+		parray_free(backup_list);
+	}
+
+	/* Try to pick backup_id and create backup directory with BACKUP_CONTROL_FILE */
+	if (start_time != INVALID_BACKUP_ID)
+	{
+		/* If user already choosed backup_id for us, then try to use it. */
+		if (start_time <= latest_backup_id)
+			/* don't care about freeing base36enc_dup memory, we exit anyway */
+			elog(ERROR, "Can't assign backup_id from requested start_time (%s), "
+						"this time must be later that backup %s",
+				base36enc_dup(start_time), base36enc_dup(latest_backup_id));
+
+		current.backup_id = start_time;
+		pgBackupInitDir(&current, instanceState->instance_backup_subdir_path);
+	}
+	else
+	{
+		/* We can generate our own unique backup_id
+		 * Sometimes (when we try to backup twice in one second)
+		 * backup_id will be duplicated -> try more times.
+		 */
+		int	attempts = 10;
+
+		if (time(NULL) < latest_backup_id)
+			elog(ERROR, "Can't assign backup_id, there is already a backup in future (%s)",
+				base36enc(latest_backup_id));
+
+		do
+		{
+			current.backup_id = time(NULL);
+			pgBackupInitDir(&current, instanceState->instance_backup_subdir_path);
+			if (current.backup_id == INVALID_BACKUP_ID)
+				sleep(1);
+		}
+		while (current.backup_id == INVALID_BACKUP_ID && attempts-- > 0);
+	}
+
+	/* If creation of backup dir was unsuccessful, there will be WARNINGS in logs already */
+	if (current.backup_id == INVALID_BACKUP_ID)
+		elog(ERROR, "Can't create backup directory");
 
 	/* Update backup status and other metainfo. */
 	current.status = BACKUP_STATUS_RUNNING;

src/catalog.c

@@ -23,7 +23,7 @@ static pgBackup* get_closest_backup(timelineInfo *tlinfo);
 static pgBackup* get_oldest_backup(timelineInfo *tlinfo);
 static const char *backupModes[] = {"", "PAGE", "PTRACK", "DELTA", "FULL"};
 static pgBackup *readBackupControlFile(const char *path);
-static void create_backup_dir(pgBackup *backup, const char *backup_instance_path);
+static int create_backup_dir(pgBackup *backup, const char *backup_instance_path);
 
 static bool backup_lock_exit_hook_registered = false;
 static parray *locks = NULL;
@@ -969,6 +969,7 @@ catalog_get_backup_list(InstanceState *instanceState, time_t requested_backup_id
 		}
 		else if (strcmp(base36enc(backup->start_time), data_ent->d_name) != 0)
 		{
+			/* TODO there is no such guarantees */
 			elog(WARNING, "backup ID in control file \"%s\" doesn't match name of the backup folder \"%s\"",
 				 base36enc(backup->start_time), backup_conf_path);
 		}
@@ -1411,22 +1412,34 @@ get_multi_timeline_parent(parray *backup_list, parray *tli_list,
 	return NULL;
 }
 
-/* Create backup directory in $BACKUP_PATH
- * Note, that backup_id attribute is updated,
- * so it is possible to get diffrent values in
+/*
+ * Create backup directory in $BACKUP_PATH
+ * (with proposed backup->backup_id)
+ * and initialize this directory.
+ * If creation of directory fails, then
+ * backup_id will be cleared (set to INVALID_BACKUP_ID).
+ * It is possible to get diffrent values in
  * pgBackup.start_time and pgBackup.backup_id.
  * It may be ok or maybe not, so it's up to the caller
  * to fix it or let it be.
  */
 
 void
-pgBackupCreateDir(pgBackup *backup, InstanceState *instanceState, time_t start_time)
+pgBackupInitDir(pgBackup *backup, const char *backup_instance_path)
 {
-	int		i;
-	parray *subdirs = parray_new();
-	parray * backups;
-	pgBackup *target_backup;
+	int	i;
+	char	temp[MAXPGPATH];
+	parray *subdirs;
 
+	/* Try to create backup directory at first */
+	if (create_backup_dir(backup, backup_instance_path) != 0)
+	{
+		/* Clear backup_id as indication of error */
+		backup->backup_id = INVALID_BACKUP_ID;
+		return;
+	}
+
+	subdirs = parray_new();
 	parray_append(subdirs, pg_strdup(DATABASE_DIR));
 
 	/* Add external dirs containers */
@@ -1438,38 +1451,13 @@ pgBackupCreateDir(pgBackup *backup, InstanceState *instanceState, time_t start_t
 													 false);
 		for (i = 0; i < parray_num(external_list); i++)
 		{
-			char		temp[MAXPGPATH];
 			/* Numeration of externaldirs starts with 1 */
 			makeExternalDirPathByNum(temp, EXTERNAL_DIR, i+1);
 			parray_append(subdirs, pg_strdup(temp));
 		}
 		free_dir_list(external_list);
 	}
 
-	/* Get list of all backups*/
-	backups = catalog_get_backup_list(instanceState, INVALID_BACKUP_ID);
-	if (parray_num(backups) > 0)
-	{
-		target_backup = (pgBackup *) parray_get(backups, 0);
-		if (start_time > target_backup->backup_id)
-		{
-			backup->backup_id = start_time;
-			create_backup_dir(backup, instanceState->instance_backup_subdir_path);
-		}
-		else
-		{
-			elog(ERROR, "Cannot create directory for older backup");
-		}
-	}
-	else
-	{
-		backup->backup_id = start_time;
-		create_backup_dir(backup, instanceState->instance_backup_subdir_path);
-	}
-
-	if (backup->backup_id == 0)
-		elog(ERROR, "Cannot create backup directory: %s", strerror(errno));
-
 	backup->database_dir = pgut_malloc(MAXPGPATH);
 	join_path_components(backup->database_dir, backup->root_dir, DATABASE_DIR);
 
@@ -1479,10 +1467,8 @@ pgBackupCreateDir(pgBackup *backup, InstanceState *instanceState, time_t start_t
 	/* create directories for actual backup files */
 	for (i = 0; i < parray_num(subdirs); i++)
 	{
-		char	path[MAXPGPATH];
-
-		join_path_components(path, backup->root_dir, parray_get(subdirs, i));
-		fio_mkdir(path, DIR_PERMISSION, FIO_BACKUP_HOST);
+		join_path_components(temp, backup->root_dir, parray_get(subdirs, i));
+		fio_mkdir(temp, DIR_PERMISSION, FIO_BACKUP_HOST);
 	}
 
 	free_dir_list(subdirs);
@@ -1491,34 +1477,26 @@ pgBackupCreateDir(pgBackup *backup, InstanceState *instanceState, time_t start_t
 /*
  * Create root directory for backup,
  * update pgBackup.root_dir if directory creation was a success
+ * Return values (same as dir_create_dir()):
+ *  0 - ok
+ * -1 - error (warning message already emitted)
  */
-void
+int
 create_backup_dir(pgBackup *backup, const char *backup_instance_path)
 {
-	int     attempts = 10;
+	int    rc;
+	char   path[MAXPGPATH];
 
-	while (attempts--)
-	{
-		int    rc;
-		char   path[MAXPGPATH];
-
-		join_path_components(path, backup_instance_path, base36enc(backup->backup_id));
+	join_path_components(path, backup_instance_path, base36enc(backup->backup_id));
 
-		/* TODO: add wrapper for remote mode */
-		rc = dir_create_dir(path, DIR_PERMISSION, true);
-
-		if (rc == 0)
-		{
-			backup->root_dir = pgut_strdup(path);
-			return;
-		}
-		else
-		{
-			elog(WARNING, "Cannot create directory \"%s\": %s", path, strerror(errno));
-			sleep(1);
-		}
-	}
+	/* TODO: add wrapper for remote mode */
+	rc = dir_create_dir(path, DIR_PERMISSION, true);
 
+	if (rc == 0)
+		backup->root_dir = pgut_strdup(path);
+	else
+		elog(WARNING, "Cannot create directory \"%s\": %s", path, strerror(errno));
+	return rc;
 }
 
 /*

src/pg_probackup.c

@@ -78,7 +78,7 @@ pid_t       my_pid = 0;
 __thread int  my_thread_num = 1;
 bool		progress = false;
 bool		no_sync = false;
-time_t		start_time = 0;
+time_t		start_time = INVALID_BACKUP_ID;
 #if PG_VERSION_NUM >= 100000
 char	   *replication_slot = NULL;
 bool		temp_slot = false;
@@ -202,7 +202,6 @@ static ConfigOption cmd_options[] =
 	{ 's', 'i', "backup-id",		&backup_id_string,	SOURCE_CMD_STRICT },
 	{ 'b', 133, "no-sync",			&no_sync,			SOURCE_CMD_STRICT },
 	{ 'b', 134, "no-color",			&no_color,			SOURCE_CMD_STRICT },
-	{ 'U', 241, "start-time",		&start_time,		SOURCE_CMD_STRICT },
 	/* backup options */
 	{ 'b', 180, "backup-pg-log",	&backup_logs,		SOURCE_CMD_STRICT },
 	{ 'f', 'b', "backup-mode",		opt_backup_mode,	SOURCE_CMD_STRICT },
@@ -217,6 +216,7 @@ static ConfigOption cmd_options[] =
 	{ 'b', 184, "merge-expired",	&merge_expired,		SOURCE_CMD_STRICT },
 	{ 'b', 185, "dry-run",			&dry_run,			SOURCE_CMD_STRICT },
 	{ 's', 238, "note",				&backup_note,		SOURCE_CMD_STRICT },
+	{ 'U', 241, "start-time",		&start_time,		SOURCE_CMD_STRICT },
 	/* catchup options */
 	{ 's', 239, "source-pgdata",		&catchup_source_pgdata,	SOURCE_CMD_STRICT },
 	{ 's', 240, "destination-pgdata",	&catchup_destination_pgdata,	SOURCE_CMD_STRICT },
@@ -975,9 +975,7 @@ main(int argc, char *argv[])
 		case BACKUP_CMD:
 			{
 				current.stream = stream_wal;
-				if (start_time == 0)
-					start_time = current_time;
-				else
+				if (start_time != INVALID_BACKUP_ID)
 					elog(WARNING, "Please do not use the --start-time option to start backup. "
 							"This is a service option required to work with other extensions. "
 							"We do not guarantee future support for this flag.");

src/pg_probackup.h

@@ -450,7 +450,10 @@ struct pgBackup
 {
 	BackupMode		backup_mode; /* Mode - one of BACKUP_MODE_xxx above*/
 	time_t			backup_id;	 /* Identifier of the backup.
-								  * Currently it's the same as start_time */
+								  * By default it's the same as start_time
+								  * but can be increased if same backup_id
+								  * already exists. It can be also set by
+								  * start_time parameter */
 	BackupStatus	status;		/* Status - one of BACKUP_STATUS_xxx above*/
 	TimeLineID		tli; 		/* timeline of start and stop backup lsns */
 	XLogRecPtr		start_lsn;	/* backup's starting transaction log location */
@@ -985,7 +988,7 @@ extern void write_backup_filelist(pgBackup *backup, parray *files,
 								  const char *root, parray *external_list, bool sync);
 
 
-extern void pgBackupCreateDir(pgBackup *backup, InstanceState *instanceState, time_t start_time);
+extern void pgBackupInitDir(pgBackup *backup, const char *backup_instance_path);
 extern void pgNodeInit(PGNodeInfo *node);
 extern void pgBackupInit(pgBackup *backup);
 extern void pgBackupFree(void *backup);