Compare system_identifier of PGDATA with system_identifier of backup_conn

Artur Zakirov · Artur Zakirov · commit 5c7669757dfc · 2017-04-05T19:48:55.000+03:00
diff --git a/backup.c b/backup.c
@@ -44,6 +44,8 @@ static volatile uint32	total_copy_files_increment;
 static uint32 total_files_num;
 static pthread_mutex_t check_stream_mut = PTHREAD_MUTEX_INITIALIZER;
 
+static int is_ptrack_enable = false;
+
 /* Backup connection */
 static PGconn *backup_conn = NULL;
 
@@ -87,6 +89,7 @@ static char *pg_ptrack_get_and_clear(Oid tablespace_oid,
 
 /* Check functions */
 static void check_server_version(void);
+static void check_system_identifier(void);
 static void confirm_block_size(const char *name, int blcksz);
 
 
@@ -114,7 +117,6 @@ do_backup_database(parray *backup_list, bool smooth_checkpoint)
 	pthread_t	backup_threads[num_threads];
 	pthread_t	stream_thread;
 	backup_files_args *backup_threads_args[num_threads];
-	bool		is_ptrack_support;
 
 	/* repack the options */
 	pgBackup   *prev_backup = NULL;
@@ -131,15 +133,6 @@ do_backup_database(parray *backup_list, bool smooth_checkpoint)
 	 */
 	current.tli = get_current_timeline(false);
 
-	is_ptrack_support = pg_ptrack_support();
-	if (current.backup_mode == BACKUP_MODE_DIFF_PTRACK && !is_ptrack_support)
-		elog(ERROR, "Current Postgres instance does not support ptrack");
-
-	if(current.backup_mode == BACKUP_MODE_DIFF_PTRACK && !pg_ptrack_enable())
-		elog(ERROR, "ptrack is disabled");
-
-	if (is_ptrack_support)
-		is_ptrack_support = pg_ptrack_enable();
 	/*
 	 * In differential backup mode, check if there is an already-validated
 	 * full backup on current timeline.
@@ -153,8 +146,8 @@ do_backup_database(parray *backup_list, bool smooth_checkpoint)
 						"Create new full backup before an incremental one.");
 	}
 
-	/* clear ptrack files for FULL and DIFF backup */
-	if (current.backup_mode != BACKUP_MODE_DIFF_PTRACK && is_ptrack_support)
+	/* Clear ptrack files for FULL and DIFF backup */
+	if (current.backup_mode != BACKUP_MODE_DIFF_PTRACK && is_ptrack_enable)
 		pg_ptrack_clear();
 
 	/* notify start of backup to PostgreSQL server */
@@ -410,12 +403,12 @@ do_backup(bool smooth_checkpoint)
 
 	/* PGDATA and BACKUP_MODE are always required */
 	if (pgdata == NULL)
-		elog(ERROR, "Required parameter not specified: PGDATA "
+		elog(ERROR, "required parameter not specified: PGDATA "
 						 "(-D, --pgdata)");
 
 	/* A backup mode is needed */
 	if (current.backup_mode == BACKUP_MODE_INVALID)
-		elog(ERROR, "Required parameter not specified: BACKUP_MODE "
+		elog(ERROR, "required parameter not specified: BACKUP_MODE "
 						 "(-b, --backup-mode)");
 
 	/* Create connection for PostgreSQL */
@@ -433,18 +426,28 @@ do_backup(bool smooth_checkpoint)
 	if (pg_is_standby() && !from_replica)
 		elog(ERROR, "backup is not allowed for standby");
 
-	/* show configuration actually used */
+	/* ptrack backup checks */
+	if (current.backup_mode == BACKUP_MODE_DIFF_PTRACK && !pg_ptrack_support())
+		elog(ERROR, "current Postgres instance does not support ptrack");
+
+	is_ptrack_enable = pg_ptrack_enable();
+	if(current.backup_mode == BACKUP_MODE_DIFF_PTRACK && !is_ptrack_enable)
+		elog(ERROR, "ptrack is disabled");
+
+	/* Get exclusive lock of backup catalog */
+	catalog_lock(true);
+
+	check_system_identifier();
+
+	/* Show configuration actually used */
 	elog(LOG, "========================================");
 	elog(LOG, "backup start");
 	elog(LOG, "----------------------------------------");
 	if (verbose)
 		pgBackupWriteConfigSection(stderr, &current);
 	elog(LOG, "----------------------------------------");
 
-	/* get exclusive lock of backup catalog */
-	catalog_lock(true);
-
-	/* initialize backup result */
+	/* Initialize backup result */
 	current.status = BACKUP_STATUS_RUNNING;
 	current.tli = 0;		/* get from result of pg_start_backup() */
 	current.start_lsn = 0;
@@ -459,7 +462,7 @@ do_backup(bool smooth_checkpoint)
 	current.checksum_version = get_data_checksum_version(true);
 	current.stream = stream_wal;
 
-	/* create backup directory and backup.ini */
+	/* Create backup directory and backup.ini */
 	if (!check)
 	{
 		if (pgBackupCreateDir(&current))
@@ -544,6 +547,30 @@ check_server_version(void)
 	confirm_block_size("wal_block_size", XLOG_BLCKSZ);
 }
 
+/*
+ * Compare system_identifier of PGDATA with system_identifier of backup_conn.
+ */
+static void
+check_system_identifier(void)
+{
+	PGresult   *res;
+	uint64		id;
+	char	   *val;
+
+	res = pgut_execute(backup_conn,
+					   "SELECT system_identifier FROM pg_control_system()",
+					   0, NULL);
+	val = PQgetvalue(res, 0, 0);
+	PQclear(res);
+
+	if (!parse_uint64(val, &id))
+		elog(ERROR, "%s is not system_identifier", val);
+
+	if (id != system_identifier)
+		elog(ERROR, "target data directory was initialized for system id %ld, but connected instance system id is %ld",
+			 system_identifier, id);
+}
+
 static void
 confirm_block_size(const char *name, int blcksz)
 {
diff --git a/catalog.c b/catalog.c
@@ -215,9 +215,9 @@ catalog_lock(bool check_catalog)
 		Assert(pgdata);
 
 		/* Check system-identifier */
-		id = get_system_identifier(true);
+		id = get_system_identifier(false);
 		if (id != system_identifier)
-			elog(ERROR, "Backup directory was initialized for system id = %ld, but target system id = %ld",
+			elog(ERROR, "backup directory was initialized for system id %ld, but target system id is %ld",
 				 system_identifier, id);
 	}
 }
diff --git a/init.c b/init.c
@@ -18,7 +18,7 @@
  */
 static int selects(const struct dirent *dir)
 {
-  return dir->d_name[0] != '.';
+	return dir->d_name[0] != '.';
 }
 
 /*
@@ -27,10 +27,10 @@ static int selects(const struct dirent *dir)
 int
 do_init(void)
 {
-	char	path[MAXPGPATH];
-	char	arclog_path_dir[MAXPGPATH];
-	FILE   *fp;
-	uint64 _system_identifier;
+	char		path[MAXPGPATH];
+	char		arclog_path_dir[MAXPGPATH];
+	FILE	   *fp;
+	uint64		id;
 
 	struct dirent **dp;
 	int results;
@@ -47,6 +47,9 @@ do_init(void)
 			elog(ERROR, "backup catalog already exist and it's not empty");
 	}
 
+	/* Read system_identifier from PGDATA */
+	id = get_system_identifier(false);
+
 	/* create backup catalog root directory */
 	dir_create_dir(backup_path, DIR_PERMISSION);
 
@@ -58,15 +61,14 @@ do_init(void)
 	join_path_components(arclog_path_dir, backup_path, "wal");
 	dir_create_dir(arclog_path_dir, DIR_PERMISSION);
 
-	_system_identifier = get_system_identifier(false);
 	/* create pg_probackup.conf */
 	join_path_components(path, backup_path, BACKUP_CATALOG_CONF_FILE);
 	fp = fopen(path, "wt");
 	if (fp == NULL)
 		elog(ERROR, "cannot create %s: %s",
 			 BACKUP_CATALOG_CONF_FILE, strerror(errno));
 
-	fprintf(fp, "system-identifier = %li\n", _system_identifier);
+	fprintf(fp, "system-identifier = %li\n", id);
 	fprintf(fp, "\n");
 	fclose(fp);
 
diff --git a/restore.c b/restore.c
@@ -457,7 +457,7 @@ restore_directories(const char *pg_data_dir, const char *backup_dir)
 				 * it again.
 				 */
 				if (!dir_is_empty(linked_path))
-					elog(ERROR, "restore destination is not empty: \"%s\"",
+					elog(ERROR, "restore tablespace destination is not empty: \"%s\"",
 						 linked_path);
 
 				if (link_sep)
@@ -560,7 +560,7 @@ check_tablespace_mapping(pgBackup *backup)
 				 linked_path);
 
 		if (!dir_is_empty(linked_path))
-			elog(ERROR, "restore destination is not empty: \"%s\"",
+			elog(ERROR, "restore tablespace destination is not empty: \"%s\"",
 				 linked_path);
 	}
 
@@ -1071,7 +1071,7 @@ get_tablespace_mapping(const char *dir)
 
 /*
  * Save create directory path into memory. We can use it in next page restore to
- * not raise the error "restore destination is not empty" in
+ * not raise the error "restore tablespace destination is not empty" in
  * restore_directories().
  */
 static void
diff --git a/tests/option_test.py b/tests/option_test.py
@@ -48,7 +48,7 @@ def test_options_4(self):
 		# backup command failure without backup mode option
 		self.assertEqual(
 			self.run_pb(["backup", "-B", self.backup_dir(node), "-D", node.data_dir]),
-			six.b("ERROR: Required parameter not specified: BACKUP_MODE (-b, --backup-mode)\n")
+			six.b("ERROR: required parameter not specified: BACKUP_MODE (-b, --backup-mode)\n")
 		)
 
 		# backup command failure with invalid backup mode option
diff --git a/tests/restore_test.py b/tests/restore_test.py
@@ -524,7 +524,7 @@ def test_restore_with_tablespace_mapping_12(self):
 
 		# 2 - Try to restore to existing tablespace directory
 		node.cleanup()
-		self.assertIn(six.b("ERROR: restore destination is not empty"),
+		self.assertIn(six.b("ERROR: restore tablespace destination is not empty"),
 			self.restore_pb(node))
 
 		# 3 - Restore using tablespace-mapping
diff --git a/util.c b/util.c
@@ -114,8 +114,8 @@ uint64
 get_system_identifier(bool safe)
 {
 	ControlFileData ControlFile;
-	char       *buffer;
-	size_t      size;
+	char	   *buffer;
+	size_t		size;
 
 	/* First fetch file... */
 	buffer = slurpFile(pgdata, "global/pg_control", &size, safe);

Original file line number	Diff line number	Diff line change
`@@ -215,9 +215,9 @@ catalog_lock(bool check_catalog)`
`215`	`215`	`Assert(pgdata);`
`216`	`216`
`217`	`217`	`/* Check system-identifier */`
`218`		`- id = get_system_identifier(true);`
	`218`	`+ id = get_system_identifier(false);`
`219`	`219`	`if (id != system_identifier)`
`220`		`- elog(ERROR, "Backup directory was initialized for system id = %ld, but target system id = %ld",`
	`220`	`+ elog(ERROR, "backup directory was initialized for system id %ld, but target system id is %ld",`
`221`	`221`	`system_identifier, id);`
`222`	`222`	`}`
`223`	`223`	`}`
Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ def test_options_4(self):`
`48`	`48`	`# backup command failure without backup mode option`
`49`	`49`	`self.assertEqual(`
`50`	`50`	`self.run_pb(["backup", "-B", self.backup_dir(node), "-D", node.data_dir]),`
`51`		`- six.b("ERROR: Required parameter not specified: BACKUP_MODE (-b, --backup-mode)\n")`
	`51`	`+ six.b("ERROR: required parameter not specified: BACKUP_MODE (-b, --backup-mode)\n")`
`52`	`52`	`)`
`53`	`53`
`54`	`54`	`# backup command failure with invalid backup mode option`