minor improvement: be more paranoid about illegal values in file->size during merge and validation

Author: gsmolk

src/merge.c

@@ -468,9 +468,6 @@ merge_files(void *arg)
 		char		from_file_path[MAXPGPATH];
 		char	   *prev_file_path;
 
-		if (!pg_atomic_test_set_flag(&file->lock))
-			continue;
-
 		/* check for interrupt */
 		if (interrupted || thread_interrupted)
 			elog(ERROR, "Interrupted during merging backups");
@@ -479,6 +476,9 @@ merge_files(void *arg)
 		if (S_ISDIR(file->mode))
 			continue;
 
+		if (!pg_atomic_test_set_flag(&file->lock))
+			continue;
+
 		if (progress)
 			elog(INFO, "Progress: (%d/%d). Process file \"%s\"",
 				 i + 1, num_files, file->path);
@@ -491,20 +491,28 @@ merge_files(void *arg)
 
 		/*
 		 * Skip files which haven't changed since previous backup. But in case
-		 * of DELTA backup we should consider n_blocks to truncate the target
-		 * backup.
+		 * of DELTA backup we must truncate the target file to n_blocks.
+		 * Unless it is a non data file, in this case truncation is not needed.
 		 */
-		if (file->write_size == BYTES_INVALID && file->n_blocks == BLOCKNUM_INVALID)
+		if (file->write_size == BYTES_INVALID)
 		{
-			elog(VERBOSE, "Skip merging file \"%s\", the file didn't change",
-				 file->path);
-
-			/*
-			 * If the file wasn't changed in PAGE backup, retreive its
-			 * write_size from previous FULL backup.
-			 */
-			if (to_file)
+			/* sanity */
+			if (!to_file)
+				elog(ERROR, "The file \"%s\" is missing in FULL backup %s",
+						file->rel_path, base36enc(to_backup->start_time));
+
+			/* for not changed files of all types in PAGE and PTRACK */
+			if (from_backup->backup_mode != BACKUP_MODE_DIFF_DELTA ||
+			/* and not changed non-data files in DELTA */
+				(!file->is_datafile || file->is_cfs))
 			{
+				elog(VERBOSE, "Skip merging file \"%s\", the file didn't change",
+					 file->path);
+
+				/*
+				 * If the file wasn't changed, retreive its
+				 * write_size and compression algorihtm from previous FULL backup.
+				 */
 				file->compress_alg = to_file->compress_alg;
 				file->write_size = to_file->write_size;
 
@@ -516,9 +524,9 @@ merge_files(void *arg)
 				/* Otherwise just get it from the previous file */
 				else
 					file->crc = to_file->crc;
-			}
 
-			continue;
+				continue;
+			}
 		}
 
 		/* We need to make full path, file object has relative path */
@@ -667,6 +675,8 @@ merge_files(void *arg)
 		if (file->write_size != BYTES_INVALID)
 			elog(VERBOSE, "Merged file \"%s\": " INT64_FORMAT " bytes",
 				 file->path, file->write_size);
+		else
+			elog(ERROR, "Merge of file \"%s\" failed. Invalid size: %i", BYTES_INVALID);
 
 		/* Restore relative path */
 		file->path = prev_file_path;

src/validate.c

@@ -24,11 +24,12 @@ static bool skipped_due_to_lock = false;
 typedef struct
 {
 	const char *base_path;
-	parray	   *files;
+	parray		*files;
 	bool		corrupted;
-	XLogRecPtr	stop_lsn;
+	XLogRecPtr 	stop_lsn;
 	uint32		checksum_version;
 	uint32		backup_version;
+	BackupMode	backup_mode;
 
 	/*
 	 * Return value from the thread.
@@ -125,6 +126,7 @@ pgBackupValidate(pgBackup *backup)
 		arg->base_path = base_path;
 		arg->files = files;
 		arg->corrupted = false;
+		arg->backup_mode = backup->backup_mode;
 		arg->stop_lsn = backup->stop_lsn;
 		arg->checksum_version = backup->checksum_version;
 		arg->backup_version = parse_program_version(backup->program_version);
@@ -184,33 +186,46 @@ pgBackupValidateFiles(void *arg)
 		struct stat st;
 		pgFile	   *file = (pgFile *) parray_get(arguments->files, i);
 
-		if (!pg_atomic_test_set_flag(&file->lock))
-			continue;
-
 		if (interrupted || thread_interrupted)
 			elog(ERROR, "Interrupted during validate");
 
 		/* Validate only regular files */
 		if (!S_ISREG(file->mode))
 			continue;
-		/*
-		 * Skip files which has no data, because they
-		 * haven't changed between backups.
-		 */
-		if (file->write_size == BYTES_INVALID)
-			continue;
 
 		/*
 		 * Currently we don't compute checksums for
 		 * cfs_compressed data files, so skip them.
+		 * TODO: investigate
 		 */
 		if (file->is_cfs)
 			continue;
 
+		if (!pg_atomic_test_set_flag(&file->lock))
+			continue;
+
 		if (progress)
 			elog(INFO, "Progress: (%d/%d). Process file \"%s\"",
 				 i + 1, num_files, file->path);
 
+		/*
+		 * Skip files which has no data, because they
+		 * haven't changed between backups.
+		 */
+		if (file->write_size == BYTES_INVALID)
+		{
+			if (arguments->backup_mode == BACKUP_MODE_FULL)
+			{
+				/* It is illegal for file in FULL backup to have BYTES_INVALID */
+				elog(WARNING, "Backup file \"%s\" has invalid size. Possible metadata corruption.",
+					file->path);
+				arguments->corrupted = true;
+				break;
+			}
+			else
+				continue;
+		}
+
 		if (stat(file->path, &st) == -1)
 		{
 			if (errno == ENOENT)