[Issue #231] make several attempts when creating backup directory, so two consecutive backups didn`t get the same backup ID

Author: gsmolk

src/backup.c

@@ -95,7 +95,6 @@ static void
 do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo, bool no_sync, bool backup_logs)
 {
 	int			i;
-	char		database_path[MAXPGPATH];
 	char		external_prefix[MAXPGPATH]; /* Temp value. Used as template */
 	char		dst_backup_path[MAXPGPATH];
 	char		label[1024];
@@ -265,18 +264,15 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo, bool no_sync, bool
 	/* Update running backup meta with START LSN */
 	write_backup(&current, true);
 
-	pgBackupGetPath(&current, database_path, lengthof(database_path),
-					DATABASE_DIR);
-	pgBackupGetPath(&current, external_prefix, lengthof(external_prefix),
-					EXTERNAL_DIR);
+	join_path_components(external_prefix, current.database_dir, EXTERNAL_DIR);
 
 	/* initialize backup's file list */
 	backup_files_list = parray_new();
 
 	/* start stream replication */
 	if (stream_wal)
 	{
-		join_path_components(dst_backup_path, database_path, PG_XLOG_DIR);
+		join_path_components(dst_backup_path, current.database_dir, PG_XLOG_DIR);
 		fio_mkdir(dst_backup_path, DIR_PERMISSION, FIO_BACKUP_HOST);
 
 		start_WAL_streaming(backup_conn, dst_backup_path, &instance_config.conn_opt,
@@ -441,7 +437,7 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo, bool no_sync, bool
 				join_path_components(dirpath, temp, file->rel_path);
 			}
 			else
-				join_path_components(dirpath, database_path, file->rel_path);
+				join_path_components(dirpath, current.database_dir, file->rel_path);
 
 			elog(VERBOSE, "Create directory '%s'", dirpath);
 			fio_mkdir(dirpath, DIR_PERMISSION, FIO_BACKUP_HOST);
@@ -475,7 +471,7 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo, bool no_sync, bool
 
 		arg->nodeInfo = nodeInfo;
 		arg->from_root = instance_config.pgdata;
-		arg->to_root = database_path;
+		arg->to_root = current.database_dir;
 		arg->external_prefix = external_prefix;
 		arg->external_dirs = external_dirs;
 		arg->files_list = backup_files_list;
@@ -552,7 +548,7 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo, bool no_sync, bool
 			elog(ERROR, "Failed to find file \"%s\" in backup filelist.",
 							XLOG_CONTROL_FILE);
 
-		set_min_recovery_point(pg_control, database_path, current.stop_lsn);
+		set_min_recovery_point(pg_control, current.database_dir, current.stop_lsn);
 	}
 
 	/* close and sync page header map */
@@ -609,7 +605,7 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo, bool no_sync, bool
 
 			/* construct fullpath */
 			if (file->external_dir_num == 0)
-				join_path_components(to_fullpath, database_path, file->rel_path);
+				join_path_components(to_fullpath, current.database_dir, file->rel_path);
 			else
 			{
 				char 	external_dst[MAXPGPATH];
@@ -726,8 +722,8 @@ pgdata_basic_setup(ConnectionOptions conn_opt, PGNodeInfo *nodeInfo)
  * Entry point of pg_probackup BACKUP subcommand.
  */
 int
-do_backup(time_t start_time, pgSetBackupParams *set_backup_params,
-			bool no_validate, bool no_sync, bool backup_logs)
+do_backup(pgSetBackupParams *set_backup_params,
+		  bool no_validate, bool no_sync, bool backup_logs)
 {
 	PGconn		*backup_conn = NULL;
 	PGNodeInfo	nodeInfo;
@@ -736,13 +732,16 @@ do_backup(time_t start_time, pgSetBackupParams *set_backup_params,
 	/* Initialize PGInfonode */
 	pgNodeInit(&nodeInfo);
 
+	/* Create backup directory and BACKUP_CONTROL_FILE */
+	pgBackupCreateDir(&current, backup_instance_path);
+
 	if (!instance_config.pgdata)
 		elog(ERROR, "required parameter not specified: PGDATA "
 						 "(-D, --pgdata)");
 
 	/* Update backup status and other metainfo. */
 	current.status = BACKUP_STATUS_RUNNING;
-	current.start_time = start_time;
+	current.start_time = current.backup_id;
 
 	StrNCpy(current.program_version, PROGRAM_VERSION,
 			sizeof(current.program_version));
@@ -757,16 +756,13 @@ do_backup(time_t start_time, pgSetBackupParams *set_backup_params,
 
 	elog(INFO, "Backup start, pg_probackup version: %s, instance: %s, backup ID: %s, backup mode: %s, "
 			"wal mode: %s, remote: %s, compress-algorithm: %s, compress-level: %i",
-			PROGRAM_VERSION, instance_name, base36enc(start_time), pgBackupGetBackupMode(&current),
+			PROGRAM_VERSION, instance_name, base36enc(current.backup_id), pgBackupGetBackupMode(&current),
 			current.stream ? "STREAM" : "ARCHIVE", IsSshProtocol()  ? "true" : "false",
 			deparse_compress_alg(current.compress_alg), current.compress_level);
 
-	/* Create backup directory and BACKUP_CONTROL_FILE */
-	if (pgBackupCreateDir(&current))
-		elog(ERROR, "Cannot create backup directory");
 	if (!lock_backup(&current, true, true))
 		elog(ERROR, "Cannot lock backup %s directory",
-			 base36enc(current.start_time));
+			 base36enc(current.backup_id));
 	write_backup(&current, true);
 
 	/* set the error processing function for the backup process */
@@ -781,7 +777,7 @@ do_backup(time_t start_time, pgSetBackupParams *set_backup_params,
 	backup_conn = pgdata_basic_setup(instance_config.conn_opt, &nodeInfo);
 
 	if (current.from_replica)
-		elog(INFO, "Backup %s is going to be taken from standby", base36enc(start_time));
+		elog(INFO, "Backup %s is going to be taken from standby", base36enc(current.backup_id));
 
 	/* TODO, print PostgreSQL full version */
 	//elog(INFO, "PostgreSQL version: %s", nodeInfo.server_version_str);

src/catalog.c

@@ -23,6 +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 time_t create_backup_dir(pgBackup *backup, const char *backup_instance_path);
 
 static bool backup_lock_exit_hook_registered = false;
 static parray *lock_files = NULL;
@@ -1136,12 +1137,18 @@ get_multi_timeline_parent(parray *backup_list, parray *tli_list,
 	return NULL;
 }
 
-/* create backup directory in $BACKUP_PATH */
-int
-pgBackupCreateDir(pgBackup *backup)
+/* Create backup directory in $BACKUP_PATH
+ * Note, that backup_id attribute is updated,
+ * so 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, const char *backup_instance_path)
 {
 	int		i;
-	char	path[MAXPGPATH];
 	parray *subdirs = parray_new();
 
 	parray_append(subdirs, pg_strdup(DATABASE_DIR));
@@ -1163,13 +1170,10 @@ pgBackupCreateDir(pgBackup *backup)
 		free_dir_list(external_list);
 	}
 
-	pgBackupGetPath(backup, path, lengthof(path), NULL);
-
-	if (!dir_is_empty(path, FIO_BACKUP_HOST))
-		elog(ERROR, "backup destination is not empty \"%s\"", path);
+	backup->backup_id = create_backup_dir(backup, backup_instance_path);
 
-	fio_mkdir(path, DIR_PERMISSION, FIO_BACKUP_HOST);
-	backup->root_dir = pgut_strdup(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);
@@ -1180,11 +1184,47 @@ pgBackupCreateDir(pgBackup *backup)
 	/* 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);
 	}
 
 	free_dir_list(subdirs);
+}
+
+/*
+ * Create root directory for backup,
+ * update pgBackup.root_dir if directory creation was a success
+ */
+time_t
+create_backup_dir(pgBackup *backup, const char *backup_instance_path)
+{
+	int     attempts = 10;
+
+	while (attempts--)
+	{
+		int    rc;
+		char   path[MAXPGPATH];
+		time_t backup_id = time(NULL);
+
+		join_path_components(path, backup_instance_path, base36enc(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 backup_id;
+		}
+		else
+		{
+			elog(WARNING, "Cannot create directory \"%s\": %s", path, strerror(errno));
+			sleep(1);
+		}
+	}
+
 	return 0;
 }
 

src/dir.c

@@ -138,9 +138,13 @@ static TablespaceList external_remap_list = {NULL, NULL};
 
 /*
  * Create directory, also create parent directories if necessary.
+ * In strict mode treat already existing directory as error.
+ * Return values:
+ *  0 - ok
+ * -1 - error (check errno)
  */
 int
-dir_create_dir(const char *dir, mode_t mode)
+dir_create_dir(const char *dir, mode_t mode, bool strict)
 {
 	char		parent[MAXPGPATH];
 
@@ -149,14 +153,14 @@ dir_create_dir(const char *dir, mode_t mode)
 
 	/* Create parent first */
 	if (access(parent, F_OK) == -1)
-		dir_create_dir(parent, mode);
+		dir_create_dir(parent, mode, false);
 
 	/* Create directory */
 	if (mkdir(dir, mode) == -1)
 	{
-		if (errno == EEXIST)	/* already exist */
+		if (errno == EEXIST && !strict)	/* already exist */
 			return 0;
-		elog(ERROR, "cannot create directory \"%s\": %s", dir, strerror(errno));
+		return -1;
 	}
 
 	return 0;

src/init.c

@@ -34,15 +34,15 @@ do_init(void)
 	}
 
 	/* create backup catalog root directory */
-	dir_create_dir(backup_path, DIR_PERMISSION);
+	dir_create_dir(backup_path, DIR_PERMISSION, false);
 
 	/* create backup catalog data directory */
 	join_path_components(path, backup_path, BACKUPS_DIR);
-	dir_create_dir(path, DIR_PERMISSION);
+	dir_create_dir(path, DIR_PERMISSION, false);
 
 	/* create backup catalog wal directory */
 	join_path_components(arclog_path_dir, backup_path, "wal");
-	dir_create_dir(arclog_path_dir, DIR_PERMISSION);
+	dir_create_dir(arclog_path_dir, DIR_PERMISSION, false);
 
 	elog(INFO, "Backup catalog '%s' successfully inited", backup_path);
 	return 0;
@@ -91,8 +91,8 @@ do_add_instance(InstanceConfig *instance)
 				instance->name, instance->arclog_path);
 
 	/* Create directory for data files of this specific instance */
-	dir_create_dir(instance->backup_instance_path, DIR_PERMISSION);
-	dir_create_dir(instance->arclog_path, DIR_PERMISSION);
+	dir_create_dir(instance->backup_instance_path, DIR_PERMISSION, false);
+	dir_create_dir(instance->arclog_path, DIR_PERMISSION, false);
 
 	/*
 	 * Write initial configuration file.

src/merge.c

@@ -639,7 +639,7 @@ merge_chain(parray *parent_chain, pgBackup *full_backup, pgBackup *dest_backup)
 			makeExternalDirPathByNum(new_container, full_external_prefix,
 									 file->external_dir_num);
 			join_path_components(dirpath, new_container, file->rel_path);
-			dir_create_dir(dirpath, DIR_PERMISSION);
+			dir_create_dir(dirpath, DIR_PERMISSION, false);
 		}
 
 		pg_atomic_init_flag(&file->lock);

src/pg_probackup.c

@@ -799,16 +799,14 @@ main(int argc, char *argv[])
 			return do_init();
 		case BACKUP_CMD:
 			{
-				time_t	start_time = time(NULL);
-
 				current.stream = stream_wal;
 
 				/* sanity */
 				if (current.backup_mode == BACKUP_MODE_INVALID)
 					elog(ERROR, "required parameter not specified: BACKUP_MODE "
 						 "(-b, --backup-mode)");
 
-				return do_backup(start_time, set_backup_params, no_validate, no_sync, backup_logs);
+				return do_backup(set_backup_params, no_validate, no_sync, backup_logs);
 			}
 		case RESTORE_CMD:
 			return do_restore_or_validate(current.backup_id,

src/pg_probackup.h

@@ -781,7 +781,7 @@ extern char** commands_args;
 extern const char *pgdata_exclude_dir[];
 
 /* in backup.c */
-extern int do_backup(time_t start_time, pgSetBackupParams *set_backup_params,
+extern int do_backup(pgSetBackupParams *set_backup_params,
 					 bool no_validate, bool no_sync, bool backup_logs);
 extern void do_checkdb(bool need_amcheck, ConnectionOptions conn_opt,
 				  char *pgdata);
@@ -916,7 +916,7 @@ extern void pgBackupGetPath2(const pgBackup *backup, char *path, size_t len,
 extern void pgBackupGetPathInInstance(const char *instance_name,
 				 const pgBackup *backup, char *path, size_t len,
 				 const char *subdir1, const char *subdir2);
-extern int pgBackupCreateDir(pgBackup *backup);
+extern void pgBackupCreateDir(pgBackup *backup, const char *backup_instance_path);
 extern void pgNodeInit(PGNodeInfo *node);
 extern void pgBackupInit(pgBackup *backup);
 extern void pgBackupFree(void *backup);
@@ -980,7 +980,7 @@ extern void makeExternalDirPathByNum(char *ret_path, const char *pattern_path,
 									 const int dir_num);
 extern bool backup_contains_external(const char *dir, parray *dirs_list);
 
-extern int dir_create_dir(const char *path, mode_t mode);
+extern int dir_create_dir(const char *path, mode_t mode, bool strict);
 extern bool dir_is_empty(const char *path, fio_location location);
 
 extern bool fileExists(const char *path, fio_location location);

src/utils/file.c

@@ -1048,7 +1048,7 @@ int fio_mkdir(char const* path, int mode, fio_location location)
 	}
 	else
 	{
-		return dir_create_dir(path, mode);
+		return dir_create_dir(path, mode, false);
 	}
 }
 
@@ -2648,7 +2648,7 @@ void fio_communicate(int in, int out)
 			break;
 		  case FIO_MKDIR:  /* Create directory */
 			hdr.size = 0;
-			hdr.arg = dir_create_dir(buf, hdr.arg);
+			hdr.arg = dir_create_dir(buf, hdr.arg, false);
 			IO_CHECK(fio_write_all(out, &hdr, sizeof(hdr)), sizeof(hdr));
 			break;
 		  case FIO_CHMOD:  /* Change file mode */

tests/backup.py

@@ -2921,12 +2921,14 @@ def test_issue_231(self):
 
         try:
             self.backup_node(
-                backup_dir, 'node', node,
-                data_dir='{0}'.format(datadir), return_id=False)
+                backup_dir, 'node', node, data_dir='{0}'.format(datadir))
         except:
             pass
 
-        self.backup_node(backup_dir, 'node', node, options=['--stream'])
+        out = self.backup_node(backup_dir, 'node', node, options=['--stream'], return_id=False)
+
+        # it is a bit racy
+        self.assertIn("WARNING: Cannot create directory", out)
 
         # Clean after yourself
         self.del_test_dir(module_name, fname)