From 419008d9de92da06b5e0cdf12c3864fd80c4eca9 Mon Sep 17 00:00:00 2001
From: Anastasia <a.lubennikova@postgrespro.ru>
Date: Tue, 2 May 2017 15:13:32 +0300
Subject: [PATCH] code cleanup

---
 backup.c   |  2 +-
 restore.c  | 24 ++++++++++++++----------
 validate.c | 28 ++++++++--------------------
 3 files changed, 23 insertions(+), 31 deletions(-)

diff --git a/backup.c b/backup.c
index 124f5687..b181d607 100644
--- a/backup.c
+++ b/backup.c
@@ -1312,7 +1312,7 @@ add_pgdata_files(parray *files, const char *root)
 			parray_remove(files, i);
 			i--;
 		}
-		/* compress map file it is not data file */
+		/* compress map file is not data file */
 		else if (path_len > 4 &&
 				 strncmp(file->path + (path_len - 4), ".cfm", 4) == 0)
 		{
diff --git a/restore.c b/restore.c
index 0a1d1e80..32031695 100644
--- a/restore.c
+++ b/restore.c
@@ -209,19 +209,24 @@ do_restore_or_validate(time_t target_backup_id,
 
 	/*
 	 * Validate backups from base_full_backup to dest_backup.
-	 * And restore if subcommand is RESTORE.
-	 * TODO what if we found out that backup is not valid?
 	 */
 	for (i = base_full_backup_index; i >= dest_backup_index; i--)
 	{
 		pgBackup   *backup = (pgBackup *) parray_get(backups, i);
+		pgBackupValidate(backup);
+	}
 
-		if (backup->status == BACKUP_STATUS_OK)
+	/* We ensured that all backups are valid, now restore if required */
+	if (is_restore)
+	{
+		for (i = base_full_backup_index; i >= dest_backup_index; i--)
 		{
-			pgBackupValidate(backup);
-
-			if (is_restore)
+			pgBackup   *backup = (pgBackup *) parray_get(backups, i);
+			if (backup->status == BACKUP_STATUS_OK)
 				restore_backup(backup);
+			else
+				elog(ERROR, "backup %s is not valid",
+					 base36enc(backup->start_time));
 		}
 	}
 
@@ -548,9 +553,8 @@ create_directory:
  * Check that all tablespace mapping entries have correct linked directory
  * paths. Linked directories must be empty or do not exist.
  *
- * If tablespace-mapping option is supplied all OLDDIR entries should have
+ * If tablespace-mapping option is supplied, all OLDDIR entries must have
  * entries in tablespace_map file.
- * TODO review
  */
 static void
 check_tablespace_mapping(pgBackup *backup)
@@ -568,7 +572,7 @@ check_tablespace_mapping(pgBackup *backup)
 
 	elog(LOG, "check tablespace directories of backup %s", base36enc(backup->start_time));
 
-	/* 1 - OLDDIR should has an entry in links */
+	/* 1 - each OLDDIR must have an entry in tablespace_map file (links) */
 	for (cell = tablespace_dirs.head; cell; cell = cell->next)
 	{
 		tmp_file->linked = cell->old_dir;
@@ -579,7 +583,7 @@ check_tablespace_mapping(pgBackup *backup)
 				 cell->old_dir);
 	}
 
-	/* 2 - all linked directories should be empty */
+	/* 2 - all linked directories must be empty */
 	for (i = 0; i < parray_num(links); i++)
 	{
 		pgFile	   *link = (pgFile *) parray_get(links, i);
diff --git a/validate.c b/validate.c
index f93a852f..0ae86228 100644
--- a/validate.c
+++ b/validate.c
@@ -18,13 +18,11 @@ static void pgBackupValidateFiles(void *arg);
 typedef struct
 {
 	parray *files;
-	bool validate_crc;
 	bool corrupted;
 } validate_files_args;
 
 /*
  * Validate backup files.
- * TODO review
  */
 void
 pgBackupValidate(pgBackup *backup)
@@ -63,12 +61,6 @@ pgBackupValidate(pgBackup *backup)
 	{
 		validate_files_args *arg = pg_malloc(sizeof(validate_files_args));
 		arg->files = files;
-		arg->validate_crc = true;
-
-		/* TODO Why didn't we validate checksums on restore before? */
-// 		if (backup_subcmd == RESTORE)
-// 			arg->validate_crc = false;
-
 		arg->corrupted = false;
 		validate_threads_args[i] = arg;
 		pthread_create(&validate_threads[i], NULL, (void *(*)(void *)) pgBackupValidateFiles, arg);
@@ -99,7 +91,7 @@ pgBackupValidate(pgBackup *backup)
 }
 
 /*
- * Validate files in the backup with size or CRC.
+ * Validate files in the backup.
  * NOTE: If file is not valid, do not use ERROR log message,
  * rather throw a WARNING and set arguments->corrupted = true.
  * This is necessary to update backup status.
@@ -109,6 +101,7 @@ pgBackupValidateFiles(void *arg)
 {
 	int		i;
 	validate_files_args *arguments = (validate_files_args *)arg;
+	pg_crc32	crc;
 
 	for (i = 0; i < parray_num(arguments->files); i++)
 	{
@@ -158,18 +151,13 @@ pgBackupValidateFiles(void *arg)
 			return;
 		}
 
-		if (arguments->validate_crc)
+		crc = pgFileGetCRC(file);
+		if (crc != file->crc)
 		{
-			pg_crc32	crc;
-
-			crc = pgFileGetCRC(file);
-			if (crc != file->crc)
-			{
-				elog(WARNING, "Invalid CRC of backup file \"%s\" : %X. Expected %X",
-					 file->path, file->crc, crc);
-				arguments->corrupted = true;
-				return;
-			}
+			elog(WARNING, "Invalid CRC of backup file \"%s\" : %X. Expected %X",
+					file->path, file->crc, crc);
+			arguments->corrupted = true;
+			return;
 		}
 	}
 }