Skip to content

Commit d57bf8e

Browse files
gsmolkgsmolk
committed
[Issue #169] Addressing some issues, pointed out by Anastasia Lubennikova review: setbuf, comments, tests
1 parent 0762083 commit d57bf8e

File tree

7 files changed

+42
-23
lines changed

7 files changed

+42
-23
lines changed

‎src/data.c

+8-4
Original file line numberDiff line numberDiff line change
@@ -613,15 +613,15 @@ backup_data_file(ConnectionArgs* conn_arg, pgFile *file,
613613
}
614614

615615
if (!fio_is_remote_file(in))
616-
setbuf(in, in_buffer);
616+
setbuffer(in, in_buffer, STDIO_BUFSIZE);
617617

618618
/* open backup file for write */
619619
out = fopen(to_fullpath, PG_BINARY_W);
620620
if (out == NULL)
621621
elog(ERROR, "Cannot open backup file \"%s\": %s",
622622
to_fullpath, strerror(errno));
623623

624-
setbuf(out, out_buffer);
624+
setbuffer(out, out_buffer, STDIO_BUFSIZE);
625625

626626
/* update file permission */
627627
if (chmod(to_fullpath, FILE_PERMISSION) == -1)
@@ -844,6 +844,10 @@ restore_data_file(parray *parent_chain, pgFile *dest_file, FILE *out, const char
844844
if (tmp_file->write_size == BYTES_INVALID)
845845
continue;
846846

847+
/* If file was truncated in intermediate backup,
848+
* it is ok not to truncate it now, because old blocks will be
849+
* overwritten by new blocks from next backup.
850+
*/
847851
if (tmp_file->write_size == 0)
848852
continue;
849853

@@ -859,7 +863,7 @@ restore_data_file(parray *parent_chain, pgFile *dest_file, FILE *out, const char
859863
elog(ERROR, "Cannot open backup file \"%s\": %s", from_fullpath,
860864
strerror(errno));
861865

862-
setbuf(in, buffer);
866+
setbuffer(in, buffer, STDIO_BUFSIZE);
863867

864868
/*
865869
* Restore the file.
@@ -1149,7 +1153,7 @@ restore_non_data_file(parray *parent_chain, pgBackup *dest_backup,
11491153
elog(ERROR, "Cannot open backup file \"%s\": %s", from_fullpath,
11501154
strerror(errno));
11511155

1152-
setbuf(in, buffer);
1156+
setbuffer(in, buffer, STDIO_BUFSIZE);
11531157

11541158
/* do actual work */
11551159
restore_non_data_file_internal(in, out, tmp_file, from_fullpath, to_fullpath);

‎src/help.c

+6
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ help_pg_probackup(void)
119119
printf(_(" [--backup-pg-log] [-j num-threads] [--progress]\n"));
120120
printf(_(" [--no-validate] [--skip-block-validation]\n"));
121121
printf(_(" [--external-dirs=external-directories-paths]\n"));
122+
printf(_(" [--no-sync]\n"));
122123
printf(_(" [--log-level-console=log-level-console]\n"));
123124
printf(_(" [--log-level-file=log-level-file]\n"));
124125
printf(_(" [--log-filename=log-filename]\n"));
@@ -156,6 +157,7 @@ help_pg_probackup(void)
156157
printf(_(" [-T OLDDIR=NEWDIR] [--progress]\n"));
157158
printf(_(" [--external-mapping=OLDDIR=NEWDIR]\n"));
158159
printf(_(" [--skip-external-dirs] [--restore-command=cmdline]\n"));
160+
printf(_(" [--no-sync]\n"));
159161
printf(_(" [--db-include | --db-exclude]\n"));
160162
printf(_(" [--remote-proto] [--remote-host]\n"));
161163
printf(_(" [--remote-port] [--remote-path] [--remote-user]\n"));
@@ -256,6 +258,7 @@ help_backup(void)
256258
printf(_(" [--backup-pg-log] [-j num-threads] [--progress]\n"));
257259
printf(_(" [--no-validate] [--skip-block-validation]\n"));
258260
printf(_(" [-E external-directories-paths]\n"));
261+
printf(_(" [--no-sync]\n"));
259262
printf(_(" [--log-level-console=log-level-console]\n"));
260263
printf(_(" [--log-level-file=log-level-file]\n"));
261264
printf(_(" [--log-filename=log-filename]\n"));
@@ -294,6 +297,7 @@ help_backup(void)
294297
printf(_(" -E --external-dirs=external-directories-paths\n"));
295298
printf(_(" backup some directories not from pgdata \n"));
296299
printf(_(" (example: --external-dirs=/tmp/dir1:/tmp/dir2)\n"));
300+
printf(_(" --no-sync do not sync backed up files to disk\n"));
297301

298302
printf(_("\n Logging options:\n"));
299303
printf(_(" --log-level-console=log-level-console\n"));
@@ -390,6 +394,7 @@ help_restore(void)
390394
printf(_(" [--external-mapping=OLDDIR=NEWDIR]\n"));
391395
printf(_(" [--skip-external-dirs]\n"));
392396
printf(_(" [--restore-command=cmdline]\n"));
397+
printf(_(" [--no-sync]\n"));
393398
printf(_(" [--db-include dbname | --db-exclude dbname]\n"));
394399
printf(_(" [--remote-proto] [--remote-host]\n"));
395400
printf(_(" [--remote-port] [--remote-path] [--remote-user]\n"));
@@ -432,6 +437,7 @@ help_restore(void)
432437
printf(_(" relocate the external directory from OLDDIR to NEWDIR\n"));
433438
printf(_(" --skip-external-dirs do not restore all external directories\n"));
434439
printf(_(" --restore-command=cmdline command to use as 'restore_command' in recovery.conf; 'none' disables\n"));
440+
printf(_(" --no-sync do not sync restored files to disk\n"));
435441

436442
printf(_("\n Partial restore options:\n"));
437443
printf(_(" --db-include dbname restore only specified databases\n"));

‎src/merge.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -1107,7 +1107,7 @@ merge_data_file(parray *parent_chain, pgBackup *full_backup,
11071107
if (out == NULL)
11081108
elog(ERROR, "Cannot open merge target file \"%s\": %s",
11091109
to_fullpath_tmp1, strerror(errno));
1110-
setbuf(out, buffer);
1110+
setbuffer(out, buffer, STDIO_BUFSIZE);
11111111

11121112
/* restore file into temp file */
11131113
tmp_file->size = restore_data_file(parent_chain, dest_file, out, to_fullpath_tmp1);

‎src/pg_probackup.c

+14-11
Original file line numberDiff line numberDiff line change
@@ -152,28 +152,29 @@ static void opt_datname_include_list(ConfigOption *opt, const char *arg);
152152

153153
/*
154154
* Short name should be non-printable ASCII character.
155+
* Use values between 128 and 255.
155156
*/
156157
static ConfigOption cmd_options[] =
157158
{
158159
/* directory options */
159-
{ 'b', 120, "help", &help_opt, SOURCE_CMD_STRICT },
160+
{ 'b', 130, "help", &help_opt, SOURCE_CMD_STRICT },
160161
{ 's', 'B', "backup-path", &backup_path, SOURCE_CMD_STRICT },
161162
/* common options */
162163
{ 'u', 'j', "threads", &num_threads, SOURCE_CMD_STRICT },
163-
{ 'b', 121, "stream", &stream_wal, SOURCE_CMD_STRICT },
164-
{ 'b', 122, "progress", &progress, SOURCE_CMD_STRICT },
164+
{ 'b', 131, "stream", &stream_wal, SOURCE_CMD_STRICT },
165+
{ 'b', 132, "progress", &progress, SOURCE_CMD_STRICT },
165166
{ 's', 'i', "backup-id", &backup_id_string, SOURCE_CMD_STRICT },
166-
{ 'b', 123, "no-sync", &no_sync, SOURCE_CMD_STRICT },
167+
{ 'b', 133, "no-sync", &no_sync, SOURCE_CMD_STRICT },
167168
/* backup options */
168-
{ 'b', 133, "backup-pg-log", &backup_logs, SOURCE_CMD_STRICT },
169+
{ 'b', 180, "backup-pg-log", &backup_logs, SOURCE_CMD_STRICT },
169170
{ 'f', 'b', "backup-mode", opt_backup_mode, SOURCE_CMD_STRICT },
170171
{ 'b', 'C', "smooth-checkpoint", &smooth_checkpoint, SOURCE_CMD_STRICT },
171172
{ 's', 'S', "slot", &replication_slot, SOURCE_CMD_STRICT },
172-
{ 'b', 234, "temp-slot", &temp_slot, SOURCE_CMD_STRICT },
173-
{ 'b', 134, "delete-wal", &delete_wal, SOURCE_CMD_STRICT },
174-
{ 'b', 135, "delete-expired", &delete_expired, SOURCE_CMD_STRICT },
175-
{ 'b', 235, "merge-expired", &merge_expired, SOURCE_CMD_STRICT },
176-
{ 'b', 237, "dry-run", &dry_run, SOURCE_CMD_STRICT },
173+
{ 'b', 181, "temp-slot", &temp_slot, SOURCE_CMD_STRICT },
174+
{ 'b', 182, "delete-wal", &delete_wal, SOURCE_CMD_STRICT },
175+
{ 'b', 183, "delete-expired", &delete_expired, SOURCE_CMD_STRICT },
176+
{ 'b', 184, "merge-expired", &merge_expired, SOURCE_CMD_STRICT },
177+
{ 'b', 185, "dry-run", &dry_run, SOURCE_CMD_STRICT },
177178
/* restore options */
178179
{ 's', 136, "recovery-target-time", &target_time, SOURCE_CMD_STRICT },
179180
{ 's', 137, "recovery-target-xid", &target_xid, SOURCE_CMD_STRICT },
@@ -218,7 +219,9 @@ static ConfigOption cmd_options[] =
218219
{ 'I', 170, "ttl", &ttl, SOURCE_CMD_STRICT, SOURCE_DEFAULT, 0, OPTION_UNIT_S, option_get_value},
219220
{ 's', 171, "expire-time", &expire_time_string, SOURCE_CMD_STRICT },
220221

221-
/* options for backward compatibility */
222+
/* options for backward compatibility
223+
* TODO: remove in 3.0.0
224+
*/
222225
{ 's', 136, "time", &target_time, SOURCE_CMD_STRICT },
223226
{ 's', 137, "xid", &target_xid, SOURCE_CMD_STRICT },
224227
{ 's', 138, "inclusive", &target_inclusive, SOURCE_CMD_STRICT },

‎src/restore.c

+8-5
Original file line numberDiff line numberDiff line change
@@ -441,7 +441,6 @@ do_restore_or_validate(time_t target_backup_id, pgRecoveryTarget *rt,
441441
base36enc(dest_backup->start_time), status2str(dest_backup->status));
442442

443443
/* We ensured that all backups are valid, now restore if required
444-
* TODO: before restore - lock entire parent chain
445444
*/
446445
if (params->is_restore)
447446
{
@@ -555,7 +554,11 @@ restore_chain(pgBackup *dest_backup, parray *parent_chain,
555554
else
556555
backup->files = dest_files;
557556

558-
/* this sorting is important */
557+
/*
558+
* this sorting is important, because we rely on it to find
559+
* destination file in intermediate backups file lists
560+
* using bsearch.
561+
*/
559562
parray_qsort(backup->files, pgFileCompareRelPathWithExternal);
560563
}
561564

@@ -622,8 +625,6 @@ restore_chain(pgBackup *dest_backup, parray *parent_chain,
622625
threads = (pthread_t *) palloc(sizeof(pthread_t) * num_threads);
623626
threads_args = (restore_files_arg *) palloc(sizeof(restore_files_arg) *
624627
num_threads);
625-
626-
/* Restore files into target directory */
627628
if (dest_backup->stream)
628629
dest_bytes = dest_backup->pgdata_bytes + dest_backup->wal_bytes;
629630
else
@@ -633,6 +634,8 @@ restore_chain(pgBackup *dest_backup, parray *parent_chain,
633634
elog(INFO, "Start restoring backup files. PGDATA size: %s", pretty_dest_bytes);
634635
time(&start_time);
635636
thread_interrupted = false;
637+
638+
/* Restore files into target directory */
636639
for (i = 0; i < num_threads; i++)
637640
{
638641
restore_files_arg *arg = &(threads_args[i]);
@@ -851,7 +854,7 @@ restore_files(void *arg)
851854
goto done;
852855

853856
if (!fio_is_remote_file(out))
854-
setbuf(out, buffer);
857+
setbuffer(out, buffer, STDIO_BUFSIZE);
855858

856859
/* Restore destination file */
857860
if (dest_file->is_datafile && !dest_file->is_cfs)

‎src/utils/file.c

+1
Original file line numberDiff line numberDiff line change
@@ -549,6 +549,7 @@ int fio_pread(FILE* f, void* buf, off_t offs)
549549
}
550550
else
551551
{
552+
/* For local file, opened by fopen, we should use stdio operations */
552553
int rc;
553554
rc = fseek(f, offs, SEEK_SET);
554555

‎src/validate.c

+4-2
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,7 @@ pgBackupValidateFiles(void *arg)
264264
*/
265265
if (file->write_size == BYTES_INVALID)
266266
{
267+
/* TODO: lookup corresponding merge bug */
267268
if (arguments->backup_mode == BACKUP_MODE_FULL)
268269
{
269270
/* It is illegal for file in FULL backup to have BYTES_INVALID */
@@ -276,10 +277,11 @@ pgBackupValidateFiles(void *arg)
276277
continue;
277278
}
278279

279-
/* no point in trying to open empty or non-changed files */
280-
if (file->write_size <= 0)
280+
/* no point in trying to open empty file */
281+
if (file->write_size == 0)
281282
continue;
282283

284+
/* TODO: it is redundant to check file existence using stat */
283285
if (stat(file->path, &st) == -1)
284286
{
285287
if (errno == ENOENT)

0 commit comments

Comments
 (0)