[Issue #120] Review

Author: gsmolk

doc/pgprobackup.xml

@@ -3239,7 +3239,7 @@ pg_probackup delete -B <replaceable>backup_dir</replaceable> --instance <replace
       policy, without performing any irreversible actions.
     </para>
     <para>
-      To delete all backups with some status, use the <option>--status</option>:
+      To delete all backups with specific status, use the <option>--status</option>:
     </para>
     <programlisting>
 pg_probackup delete -B <replaceable>backup_dir</replaceable> --instance <replaceable>instance_name</replaceable> --status=ERROR

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);
 
+	backup_list = catalog_get_backup_list(instance_config->name, INVALID_BACKUP_ID);
 
-	/* Delete all error backups. */
-	backup_list = catalog_get_backup_list(instance_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;
 }

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;

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,

src/util.c

@@ -18,6 +18,21 @@
 
 #include <sys/stat.h>
 
+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;
 }

tests/delete.py

@@ -845,6 +845,5 @@ def test_delete_error_backups(self):
 	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)