[Issue #92] Improvements pointed out by review

Author: gsmolk

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);

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))

src/delete.c

@@ -225,14 +225,15 @@ 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 ||
 					backup->status == BACKUP_STATUS_DONE))
 			{
 				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