Fix bug: get_control_value() should use signed long int; refactor restore_data_file(), so the loop will break in first truncated block

Author: za-arthur

src/data.c

@@ -513,6 +513,8 @@ backup_data_file(backup_files_args* arguments,
 	 * Read each page, verify checksum and write it to backup.
 	 * If page map is empty or file is not present in previous backup
 	 * backup all pages of the relation.
+	 *
+	 * We will enter here if backup_mode is FULL or DELTA.
 	 */
 	if (file->pagemap.bitmapsize == PageBitmapIsEmpty ||
 		file->pagemap_isabsent || !file->exists_in_prev)
@@ -525,11 +527,17 @@ backup_data_file(backup_files_args* arguments,
 			compress_and_backup_page(file, blknum, in, out, &(file->crc),
 			                          page_state, curr_page);
 			n_blocks_read++;
+			if (page_state == PageIsTruncated)
+				break;
 		}
 		if (backup_mode == BACKUP_MODE_DIFF_DELTA)
 			file->n_blocks = n_blocks_read;
 	}
-	/* If page map is not empty we scan only changed blocks, */
+	/*
+	 * If page map is not empty we scan only changed blocks.
+	 *
+	 * We will enter here if backup_mode is PAGE or PTRACK.
+	 */
 	else
 	{
 		datapagemap_iterator_t *iter;
@@ -542,6 +550,8 @@ backup_data_file(backup_files_args* arguments,
 			compress_and_backup_page(file, blknum, in, out, &(file->crc),
 			                          page_state, curr_page);
 			n_blocks_read++;
+			if (page_state == PageIsTruncated)
+				break;
 		}
 
 		pg_free(file->pagemap.bitmap);
@@ -596,8 +606,9 @@ restore_data_file(const char *from_root,
 	FILE	   *in = NULL;
 	FILE	   *out = NULL;
 	BackupPageHeader header;
-	BlockNumber	blknum;
-	size_t		file_size;
+	BlockNumber	blknum = 0,
+				truncate_from = 0;
+	bool		need_truncate = false;
 
 	/* BYTES_INVALID allowed only in case of restoring file from DELTA backup */
 	if (file->write_size != BYTES_INVALID)
@@ -628,7 +639,7 @@ restore_data_file(const char *from_root,
 			 to_path, strerror(errno_tmp));
 	}
 
-	for (blknum = 0; ; blknum++)
+	while (true)
 	{
 		size_t		read_len;
 		DataPage	compressed_page; /* used as read buffer */
@@ -638,6 +649,21 @@ restore_data_file(const char *from_root,
 		if (file->write_size == BYTES_INVALID)
 			break;
 
+		/*
+		 * We need to truncate result file if data file in a incremental backup
+		 * less than data file in a full backup. We know it thanks to n_blocks.
+		 *
+		 * It may be equal to -1, then we don't want to truncate the result
+		 * file.
+		 */
+		if (file->n_blocks != BLOCKNUM_INVALID &&
+			(blknum + 1) > file->n_blocks)
+		{
+			truncate_from = blknum;
+			need_truncate = true;
+			break;
+		}
+
 		/* read BackupPageHeader */
 		read_len = fread(&header, 1, sizeof(header), in);
 		if (read_len != sizeof(header))
@@ -658,17 +684,16 @@ restore_data_file(const char *from_root,
 			elog(ERROR, "backup is broken at file->path %s block %u",
 			     file->path, blknum);
 
+		blknum = header.block;
+
 		if (header.compressed_size == PageIsTruncated)
 		{
 			/*
 			 * Backup contains information that this block was truncated.
-			 * Truncate file to this length.
+			 * We need to truncate file to this length.
 			 */
-			if (ftruncate(fileno(out), header.block * BLCKSZ) != 0)
-				elog(ERROR, "cannot truncate \"%s\": %s",
-					 file->path, strerror(errno));
-			elog(VERBOSE, "truncate file %s to block %u",
-			     file->path, header.block);
+			truncate_from = blknum;
+			need_truncate = true;
 			break;
 		}
 
@@ -697,7 +722,6 @@ restore_data_file(const char *from_root,
 		/*
 		 * Seek and write the restored page.
 		 */
-		blknum = header.block;
 		if (fseek(out, blknum * BLCKSZ, SEEK_SET) < 0)
 			elog(ERROR, "cannot seek block %u of \"%s\": %s",
 				 blknum, to_path, strerror(errno));
@@ -724,25 +748,34 @@ restore_data_file(const char *from_root,
 	 * So when restoring file from DELTA backup we, knowning it`s size at
 	 * a time of a backup, can truncate file to this size.
 	 */
-
-	if (backup->backup_mode == BACKUP_MODE_DIFF_DELTA)
+	if (backup->backup_mode == BACKUP_MODE_DIFF_DELTA &&
+		file->n_blocks != BLOCKNUM_INVALID && !need_truncate)
 	{
+		size_t		file_size = 0;
+
 		/* get file current size */
 		fseek(out, 0, SEEK_END);
 		file_size = ftell(out);
+
 		if (file_size > file->n_blocks * BLCKSZ)
 		{
-			/*
-			 * Truncate file to this length.
-			 */
-			if (ftruncate(fileno(out), file->n_blocks * BLCKSZ) != 0)
-				elog(ERROR, "cannot truncate \"%s\": %s",
-					 file->path, strerror(errno));
-			elog(INFO, "Delta truncate file %s to block %u",
-			     file->path, file->n_blocks);
+			truncate_from = file->n_blocks;
+			need_truncate = true;
 		}
 	}
 
+	if (need_truncate)
+	{
+		/*
+		 * Truncate file to this length.
+		 */
+		if (ftruncate(fileno(out), truncate_from * BLCKSZ) != 0)
+			elog(ERROR, "cannot truncate \"%s\": %s",
+				 file->path, strerror(errno));
+		elog(INFO, "Delta truncate file %s to block %u",
+			 file->path, truncate_from);
+	}
+
 	/* update file permission */
 	if (chmod(to_path, file->mode) == -1)
 	{

src/dir.c

@@ -186,7 +186,8 @@ pgFileInit(const char *path)
 
 	file->is_cfs = false;
 	file->exists_in_prev = false;	/* can change only in Incremental backup. */
-	file->n_blocks = -1;			/* can change only in DELTA backup. Number of blocks readed during backup */
+	/* Number of blocks readed during backup */
+	file->n_blocks = BLOCKNUM_INVALID;
 	file->compress_alg = NOT_DEFINED_COMPRESS;
 	return file;
 }
@@ -836,7 +837,7 @@ print_file_list(FILE *out, const parray *files, const char *root)
 #endif
 			fprintf(out, ",\"linked\":\"%s\"", file->linked);
 
-		if (file->n_blocks != -1)
+		if (file->n_blocks != BLOCKNUM_INVALID)
 			fprintf(out, ",\"n_blocks\":\"%i\"", file->n_blocks);
 
 		fprintf(out, "}\n");
@@ -858,23 +859,25 @@ print_file_list(FILE *out, const parray *files, const char *root)
  *   {"name1":"value1", "name2":"value2"}
  *
  * The value will be returned to "value_str" as string if it is not NULL. If it
- * is NULL the value will be returned to "value_ulong" as unsigned long.
+ * is NULL the value will be returned to "value_uint64" as int64.
+ *
+ * Returns true if the value was found in the line.
  */
-static void
+static bool
 get_control_value(const char *str, const char *name,
-				  char *value_str, uint64 *value_uint64, bool is_mandatory)
+				  char *value_str, int64 *value_int64, bool is_mandatory)
 {
 	int			state = CONTROL_WAIT_NAME;
 	char	   *name_ptr = (char *) name;
 	char	   *buf = (char *) str;
-	char		buf_uint64[32],	/* Buffer for "value_uint64" */
-			   *buf_uint64_ptr = buf_uint64;
+	char		buf_int64[32],	/* Buffer for "value_int64" */
+			   *buf_int64_ptr = buf_int64;
 
 	/* Set default values */
 	if (value_str)
 		*value_str = '\0';
-	else if (value_uint64)
-		*value_uint64 = 0;
+	else if (value_int64)
+		*value_int64 = 0;
 
 	while (*buf)
 	{
@@ -909,7 +912,7 @@ get_control_value(const char *str, const char *name,
 				if (*buf == '"')
 				{
 					state = CONTROL_INVALUE;
-					buf_uint64_ptr = buf_uint64;
+					buf_int64_ptr = buf_int64;
 				}
 				else if (IsAlpha(*buf))
 					goto bad_format;
@@ -922,19 +925,19 @@ get_control_value(const char *str, const char *name,
 					{
 						*value_str = '\0';
 					}
-					else if (value_uint64)
+					else if (value_int64)
 					{
 						/* Length of buf_uint64 should not be greater than 31 */
-						if (buf_uint64_ptr - buf_uint64 >= 32)
+						if (buf_int64_ptr - buf_int64 >= 32)
 							elog(ERROR, "field \"%s\" is out of range in the line %s of the file %s",
 								 name, str, DATABASE_FILE_LIST);
 
-						*buf_uint64_ptr = '\0';
-						if (!parse_uint64(buf_uint64, value_uint64, 0))
+						*buf_int64_ptr = '\0';
+						if (!parse_int64(buf_int64, value_int64, 0))
 							goto bad_format;
 					}
 
-					return;
+					return true;
 				}
 				else
 				{
@@ -945,8 +948,8 @@ get_control_value(const char *str, const char *name,
 					}
 					else
 					{
-						*buf_uint64_ptr = *buf;
-						buf_uint64_ptr++;
+						*buf_int64_ptr = *buf;
+						buf_int64_ptr++;
 					}
 				}
 				break;
@@ -970,11 +973,12 @@ get_control_value(const char *str, const char *name,
 	if (is_mandatory)
 		elog(ERROR, "field \"%s\" is not found in the line %s of the file %s",
 			 name, str, DATABASE_FILE_LIST);
-	return;
+	return false;
 
 bad_format:
 	elog(ERROR, "%s file has invalid format in line %s",
 		 DATABASE_FILE_LIST, str);
+	return false;	/* Make compiler happy */
 }
 
 /*
@@ -1001,7 +1005,7 @@ dir_read_file_list(const char *root, const char *file_txt)
 		char		filepath[MAXPGPATH];
 		char		linked[MAXPGPATH];
 		char		compress_alg_string[MAXPGPATH];
-		uint64		write_size,
+		int64		write_size,
 					mode,		/* bit length of mode_t depends on platforms */
 					is_datafile,
 					is_cfs,
@@ -1016,12 +1020,7 @@ dir_read_file_list(const char *root, const char *file_txt)
 		get_control_value(buf, "is_datafile", NULL, &is_datafile, true);
 		get_control_value(buf, "is_cfs", NULL, &is_cfs, false);
 		get_control_value(buf, "crc", NULL, &crc, true);
-
-		/* optional fields */
-		get_control_value(buf, "linked", linked, NULL, false);
-		get_control_value(buf, "segno", NULL, &segno, false);
 		get_control_value(buf, "compress_alg", compress_alg_string, NULL, false);
-		get_control_value(buf, "n_blocks", NULL, &n_blocks, false);
 
 		if (root)
 			join_path_components(filepath, root, path);
@@ -1036,10 +1035,19 @@ dir_read_file_list(const char *root, const char *file_txt)
 		file->is_cfs = is_cfs ? true : false;
 		file->crc = (pg_crc32) crc;
 		file->compress_alg = parse_compress_alg(compress_alg_string);
-		if (linked[0])
+
+		/*
+		 * Optional fields
+		 */
+
+		if (get_control_value(buf, "linked", linked, NULL, false) && linked[0])
 			file->linked = pgut_strdup(linked);
-		file->segno = (int) segno;
-		file->n_blocks = (int) n_blocks;
+
+		if (get_control_value(buf, "segno", NULL, &segno, false))
+			file->segno = (int) segno;
+
+		if (get_control_value(buf, "n_blocks", NULL, &n_blocks, false))
+			file->n_blocks = (int) n_blocks;
 
 		parray_append(files, file);
 	}

src/pg_probackup.h

@@ -165,6 +165,7 @@ typedef enum ShowFormat
 /* special values of pgBackup fields */
 #define INVALID_BACKUP_ID	0    /* backup ID is not provided by user */
 #define BYTES_INVALID		(-1)
+#define BLOCKNUM_INVALID	(-1)
 
 typedef struct pgBackupConfig
 {