From 9864f5816edeefeaa171eb02ed0840331c58a7b9 Mon Sep 17 00:00:00 2001 From: Grigory Smolkin Date: Tue, 24 Mar 2020 21:37:35 +0300 Subject: [PATCH] [Issue #120] Review --- doc/pgprobackup.xml | 2 +- src/delete.c | 61 +++++++++++++++++++++++++++------------------ src/pg_probackup.c | 10 +++++--- src/pg_probackup.h | 4 +-- src/util.c | 36 +++++++++++++------------- tests/delete.py | 5 ++-- 6 files changed, 66 insertions(+), 52 deletions(-) diff --git a/doc/pgprobackup.xml b/doc/pgprobackup.xml index ee05f974..105fda4d 100644 --- a/doc/pgprobackup.xml +++ b/doc/pgprobackup.xml @@ -3239,7 +3239,7 @@ pg_probackup delete -B backup_dir --instance - To delete all backups with some status, use the : + To delete all backups with specific status, use the : pg_probackup delete -B backup_dir --instance instance_name --status=ERROR diff --git a/src/delete.c b/src/delete.c index 69630180..f92b9e6d 100644 --- a/src/delete.c +++ b/src/delete.c @@ -123,7 +123,7 @@ do_delete(time_t backup_id) * which FULL backup should be keeped for redundancy obligation(only valid do), * but if invalid backup is not guarded by retention - it is removed */ -int do_retention(void) +void do_retention(void) { parray *backup_list = NULL; parray *to_keep_list = parray_new(); @@ -154,7 +154,7 @@ int do_retention(void) /* Retention is disabled but we still can cleanup wal */ elog(WARNING, "Retention policy is not set"); if (!delete_wal) - return 0; + return; } else /* At least one retention policy is active */ @@ -196,9 +196,6 @@ int do_retention(void) parray_free(backup_list); parray_free(to_keep_list); parray_free(to_purge_list); - - return 0; - } /* Evaluate every backup by retention policies and populate purge and keep lists. @@ -1024,42 +1021,58 @@ do_delete_instance(void) return 0; } -/* Delete all error backup files of given instance. */ -int -do_delete_status(char* status) +/* Delete all backups of given status in instance */ +void +do_delete_status(InstanceConfig *instance_config, const char *status) { parray *backup_list; - parray *xlog_files_list; int i; - int rc; - char instance_config_path[MAXPGPATH]; - - BackupStatus status_for_delete; + const char *pretty_status; + int n_deleted = 0; - status_for_delete = str2status(status); + BackupStatus status_for_delete = str2status(status); if (status_for_delete == BACKUP_STATUS_INVALID) - elog(ERROR, "Unknown '%s' value for --status option", status); + elog(ERROR, "Unknown value for '--status' option: '%s'", status); + /* + * User may have provided status string in lower case, but + * we should print backup statuses consistently with show command, + * so convert it. + */ + pretty_status = status2str(status_for_delete); - /* Delete all error backups. */ - backup_list = catalog_get_backup_list(instance_name, INVALID_BACKUP_ID); + backup_list = catalog_get_backup_list(instance_config->name, INVALID_BACKUP_ID); + if (parray_num(backup_list) == 0) + { + elog(WARNING, "Instance '%s' has no backups", instance_config->name); + return; + } + + elog(INFO, "Deleting all backups with status '%s'", pretty_status); + + /* Delete all backups with specified status */ for (i = 0; i < parray_num(backup_list); i++) { pgBackup *backup = (pgBackup *) parray_get(backup_list, i); - if (backup->status == status_for_delete){ - /* elog(INFO, "Delete error backup '%s' ", base36enc(backup->backup_id)); */ - catalog_lock_backup_list(backup_list, i, i); + + if (backup->status == status_for_delete) + { + lock_backup(backup); delete_backup_files(backup); + n_deleted++; } } + if (n_deleted > 0) + elog(INFO, "Successfully deleted all backups with status '%s' from instance '%s'", + pretty_status, instance_config->name); + else + elog(WARNING, "Instance '%s' has no backups with status '%s'", + instance_config->name, pretty_status); + /* Cleanup */ parray_walk(backup_list, pgBackupFree); parray_free(backup_list); - - - elog(INFO, "Backups with status '%s' from instance '%s' successfully deleted", status, instance_name); - return 0; } diff --git a/src/pg_probackup.c b/src/pg_probackup.c index f86819a3..814c8ab9 100644 --- a/src/pg_probackup.c +++ b/src/pg_probackup.c @@ -806,14 +806,16 @@ main(int argc, char *argv[]) elog(ERROR, "You cannot specify --merge-expired and (-i, --backup-id) options together"); if (delete_status && backup_id_string) elog(ERROR, "You cannot specify --status and (-i, --backup-id) options together"); - if (!delete_expired && !merge_expired && !delete_wal && delete_status==NULL && !backup_id_string) + if (!delete_expired && !merge_expired && !delete_wal && delete_status == NULL && !backup_id_string) elog(ERROR, "You must specify at least one of the delete options: " "--delete-expired |--delete-wal |--merge-expired |--status |(-i, --backup-id)"); if (!backup_id_string) - if (delete_status) - do_delete_status(delete_status); + { + if (delete_status) + do_delete_status(&instance_config, delete_status); else - return do_retention(); + do_retention(); + } else do_delete(current.backup_id); break; diff --git a/src/pg_probackup.h b/src/pg_probackup.h index 3d70d81e..cdc1bad1 100644 --- a/src/pg_probackup.h +++ b/src/pg_probackup.h @@ -709,9 +709,9 @@ extern int do_show(const char *instance_name, time_t requested_backup_id, bool s /* in delete.c */ extern void do_delete(time_t backup_id); extern void delete_backup_files(pgBackup *backup); -extern int do_retention(void); +extern void do_retention(void); extern int do_delete_instance(void); -extern int do_delete_status(char *status); +extern void do_delete_status(InstanceConfig *instance_config, const char *status); /* in fetch.c */ extern char *slurpFile(const char *datadir, diff --git a/src/util.c b/src/util.c index bf372a3b..776c2ec7 100644 --- a/src/util.c +++ b/src/util.c @@ -18,6 +18,21 @@ #include +static const char *statusName[] = +{ + "UNKNOWN", + "OK", + "ERROR", + "RUNNING", + "MERGING", + "MERGED", + "DELETING", + "DELETED", + "DONE", + "ORPHAN", + "CORRUPT" +}; + const char * base36enc(long unsigned int value) { @@ -458,20 +473,6 @@ parse_program_version(const char *program_version) return result; } -static const char *statusName[] = -{ - "UNKNOWN", - "OK", - "ERROR", - "RUNNING", - "MERGING", - "MERGED", - "DELETING", - "DELETED", - "DONE", - "ORPHAN", - "CORRUPT" -}; const char * status2str(BackupStatus status) @@ -482,16 +483,15 @@ status2str(BackupStatus status) return statusName[status]; } - - BackupStatus str2status(const char *status) { + BackupStatus i; - for (int i = 0; i <= BACKUP_STATUS_CORRUPT; i++) + for (i = BACKUP_STATUS_INVALID; i <= BACKUP_STATUS_CORRUPT; i++) { if (pg_strcasecmp(status, statusName[i]) == 0) return i; } - return 0; + return BACKUP_STATUS_INVALID; } diff --git a/tests/delete.py b/tests/delete.py index 79450280..0f5be6be 100644 --- a/tests/delete.py +++ b/tests/delete.py @@ -845,6 +845,5 @@ class DeleteTest(ProbackupTest, unittest.TestCase): self.assertEqual(show_backups[0]['status'], "OK") self.assertEqual(show_backups[1]['status'], "OK") - # Clean after yourself - self.del_test_dir(module_name, fname) - + # Clean after yourself + self.del_test_dir(module_name, fname)