From fd9165257647440ac50e99648822973c3a6fccd7 Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Sat, 6 Jul 2019 04:00:45 +0300 Subject: [PATCH] [Issue #92] Improvements pointed out by review --- src/backup.c | 5 +++-- src/catalog.c | 11 ++++------- src/delete.c | 6 +++--- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/backup.c b/src/backup.c index c4c1d738..96723f33 100644 --- a/src/backup.c +++ b/src/backup.c @@ -200,8 +200,9 @@ do_backup_instance(PGconn *backup_conn) prev_backup = catalog_get_last_data_backup(backup_list, current.tli, current.start_time); if (prev_backup == NULL) - elog(ERROR, "Valid backup on current timeline is not found. " - "Create new FULL backup before an incremental one."); + elog(ERROR, "Valid backup on current timeline %X is not found. " + "Create new FULL backup before an incremental one.", + current.tli); pgBackupGetPath(prev_backup, prev_backup_filelist_path, lengthof(prev_backup_filelist_path), DATABASE_FILE_LIST); diff --git a/src/catalog.c b/src/catalog.c index aac3527c..4259a4ae 100644 --- a/src/catalog.c +++ b/src/catalog.c @@ -467,10 +467,7 @@ catalog_get_last_data_backup(parray *backup_list, TimeLineID tli, time_t current /* Failed to find valid FULL backup to fulfill ancestor role */ if (!full_backup) - { - elog(WARNING, "Failed to find a valid backup chain"); return NULL; - } elog(INFO, "Latest valid FULL backup: %s", base36enc(full_backup->start_time)); @@ -480,7 +477,7 @@ catalog_get_last_data_backup(parray *backup_list, TimeLineID tli, time_t current { pgBackup *backup = (pgBackup *) parray_get(backup_list, i); - /* only valid descendants are acceptable */ + /* only valid descendants are acceptable for evaluation */ if ((backup->status == BACKUP_STATUS_OK || backup->status == BACKUP_STATUS_DONE)) { @@ -505,9 +502,9 @@ catalog_get_last_data_backup(parray *backup_list, TimeLineID tli, time_t current continue; /* chain is ok */ - case 2 : - /* Yes, we could call is_parent() earlier, after choosing the ancestor, - * but this way we have an opportunity to report about all possible + case 2: + /* Yes, we could call is_parent() earlier - after choosing the ancestor, + * but this way we have an opportunity to detect and report all possible * anomalies. */ if (is_parent(full_backup->start_time, backup, true)) diff --git a/src/delete.c b/src/delete.c index 15a62bfe..1bce734c 100644 --- a/src/delete.c +++ b/src/delete.c @@ -225,7 +225,7 @@ do_retention_internal(parray *backup_list, parray *to_keep_list, parray *to_purg { pgBackup *backup = (pgBackup *) parray_get(backup_list, i); - /* Consider only valid backups for Redundancy */ + /* Consider only valid FULL backups for Redundancy */ if (instance_config.retention_redundancy > 0 && backup->backup_mode == BACKUP_MODE_FULL && (backup->status == BACKUP_STATUS_OK || @@ -233,6 +233,7 @@ do_retention_internal(parray *backup_list, parray *to_keep_list, parray *to_purg { n_full_backups++; + /* Add every FULL backup that satisfy Redundancy policy to separate list */ if (n_full_backups <= instance_config.retention_redundancy) { if (!redundancy_full_backup_list) @@ -260,7 +261,7 @@ do_retention_internal(parray *backup_list, parray *to_keep_list, parray *to_purg bool redundancy_keep = false; pgBackup *backup = (pgBackup *) parray_get(backup_list, (size_t) i); - /* check if backups FULL parent is in redundancy list */ + /* check if backup`s FULL ancestor is in redundancy list */ if (redundancy_full_backup_list) { pgBackup *full_backup = find_parent_full_backup(backup); @@ -283,7 +284,6 @@ do_retention_internal(parray *backup_list, parray *to_keep_list, parray *to_purg * TODO: consider that ERROR backup most likely to have recovery_time == 0 */ if ((days_threshold == 0 || (days_threshold > backup->recovery_time)) && -// (instance_config.retention_redundancy <= (n_full_backups - cur_full_backup_num))) (instance_config.retention_redundancy == 0 || !redundancy_keep)) { /* This backup is not guarded by retention