Skip to content

Commit 93e8a99

Browse files
gsmolkgsmolk
committed
minor improvement: be more paranoid about illegal values in file->size during merge and validation
1 parent ad582b4 commit 93e8a99

File tree

2 files changed

+52
-27
lines changed

2 files changed

+52
-27
lines changed

‎src/merge.c

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -468,9 +468,6 @@ merge_files(void *arg)
468468
char from_file_path[MAXPGPATH];
469469
char *prev_file_path;
470470

471-
if (!pg_atomic_test_set_flag(&file->lock))
472-
continue;
473-
474471
/* check for interrupt */
475472
if (interrupted || thread_interrupted)
476473
elog(ERROR, "Interrupted during merging backups");
@@ -479,6 +476,9 @@ merge_files(void *arg)
479476
if (S_ISDIR(file->mode))
480477
continue;
481478

479+
if (!pg_atomic_test_set_flag(&file->lock))
480+
continue;
481+
482482
if (progress)
483483
elog(INFO, "Progress: (%d/%d). Process file \"%s\"",
484484
i + 1, num_files, file->path);
@@ -491,20 +491,28 @@ merge_files(void *arg)
491491

492492
/*
493493
* Skip files which haven't changed since previous backup. But in case
494-
* of DELTA backup we should consider n_blocks to truncate the target
495-
* backup.
494+
* of DELTA backup we must truncate the target file to n_blocks.
495+
* Unless it is a non data file, in this case truncation is not needed.
496496
*/
497-
if (file->write_size == BYTES_INVALID && file->n_blocks == BLOCKNUM_INVALID)
497+
if (file->write_size == BYTES_INVALID)
498498
{
499-
elog(VERBOSE, "Skip merging file \"%s\", the file didn't change",
500-
file->path);
501-
502-
/*
503-
* If the file wasn't changed in PAGE backup, retreive its
504-
* write_size from previous FULL backup.
505-
*/
506-
if (to_file)
499+
/* sanity */
500+
if (!to_file)
501+
elog(ERROR, "The file \"%s\" is missing in FULL backup %s",
502+
file->rel_path, base36enc(to_backup->start_time));
503+
504+
/* for not changed files of all types in PAGE and PTRACK */
505+
if (from_backup->backup_mode != BACKUP_MODE_DIFF_DELTA ||
506+
/* and not changed non-data files in DELTA */
507+
(!file->is_datafile || file->is_cfs))
507508
{
509+
elog(VERBOSE, "Skip merging file \"%s\", the file didn't change",
510+
file->path);
511+
512+
/*
513+
* If the file wasn't changed, retreive its
514+
* write_size and compression algorihtm from previous FULL backup.
515+
*/
508516
file->compress_alg = to_file->compress_alg;
509517
file->write_size = to_file->write_size;
510518

@@ -516,9 +524,9 @@ merge_files(void *arg)
516524
/* Otherwise just get it from the previous file */
517525
else
518526
file->crc = to_file->crc;
519-
}
520527

521-
continue;
528+
continue;
529+
}
522530
}
523531

524532
/* We need to make full path, file object has relative path */
@@ -667,6 +675,8 @@ merge_files(void *arg)
667675
if (file->write_size != BYTES_INVALID)
668676
elog(VERBOSE, "Merged file \"%s\": " INT64_FORMAT " bytes",
669677
file->path, file->write_size);
678+
else
679+
elog(ERROR, "Merge of file \"%s\" failed. Invalid size: %i", BYTES_INVALID);
670680

671681
/* Restore relative path */
672682
file->path = prev_file_path;

‎src/validate.c

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,12 @@ static bool skipped_due_to_lock = false;
2424
typedef struct
2525
{
2626
const char *base_path;
27-
parray *files;
27+
parray *files;
2828
bool corrupted;
29-
XLogRecPtr stop_lsn;
29+
XLogRecPtr stop_lsn;
3030
uint32 checksum_version;
3131
uint32 backup_version;
32+
BackupMode backup_mode;
3233

3334
/*
3435
* Return value from the thread.
@@ -125,6 +126,7 @@ pgBackupValidate(pgBackup *backup)
125126
arg->base_path = base_path;
126127
arg->files = files;
127128
arg->corrupted = false;
129+
arg->backup_mode = backup->backup_mode;
128130
arg->stop_lsn = backup->stop_lsn;
129131
arg->checksum_version = backup->checksum_version;
130132
arg->backup_version = parse_program_version(backup->program_version);
@@ -184,33 +186,46 @@ pgBackupValidateFiles(void *arg)
184186
struct stat st;
185187
pgFile *file = (pgFile *) parray_get(arguments->files, i);
186188

187-
if (!pg_atomic_test_set_flag(&file->lock))
188-
continue;
189-
190189
if (interrupted || thread_interrupted)
191190
elog(ERROR, "Interrupted during validate");
192191

193192
/* Validate only regular files */
194193
if (!S_ISREG(file->mode))
195194
continue;
196-
/*
197-
* Skip files which has no data, because they
198-
* haven't changed between backups.
199-
*/
200-
if (file->write_size == BYTES_INVALID)
201-
continue;
202195

203196
/*
204197
* Currently we don't compute checksums for
205198
* cfs_compressed data files, so skip them.
199+
* TODO: investigate
206200
*/
207201
if (file->is_cfs)
208202
continue;
209203

204+
if (!pg_atomic_test_set_flag(&file->lock))
205+
continue;
206+
210207
if (progress)
211208
elog(INFO, "Progress: (%d/%d). Process file \"%s\"",
212209
i + 1, num_files, file->path);
213210

211+
/*
212+
* Skip files which has no data, because they
213+
* haven't changed between backups.
214+
*/
215+
if (file->write_size == BYTES_INVALID)
216+
{
217+
if (arguments->backup_mode == BACKUP_MODE_FULL)
218+
{
219+
/* It is illegal for file in FULL backup to have BYTES_INVALID */
220+
elog(WARNING, "Backup file \"%s\" has invalid size. Possible metadata corruption.",
221+
file->path);
222+
arguments->corrupted = true;
223+
break;
224+
}
225+
else
226+
continue;
227+
}
228+
214229
if (stat(file->path, &st) == -1)
215230
{
216231
if (errno == ENOENT)

0 commit comments

Comments
 (0)