backup: ValidateEndTimeAndTruncate now supports compacted backups with rev history

Previously, the logic in `ValidateEndTimeAndTruncate` assumed that for a
revision history backup tree, there would be no compacted backups. This
patch teaches the function to now work with trees that contain both
revision history backups and compacted backups. The logic works as such:

1. Compacted backups that cover the requested AOST (but do not have a
matching end time) are elided.
2. Any preceding compacted backups will still be included and its
component backups elided.

This patch is part of a larger effort to fully support compactions in
revision history trees.

Informs: #158696

Release note: None

Author: kev-cao

pkg/backup/backupinfo/manifest_handling.go

@@ -967,34 +967,54 @@ func ValidateEndTimeAndTruncate[E ManifestLike](
 		if includeSkipped {
 			return manifests, nil
 		}
-		manifests = elideSkippedLayers(manifests)
+		manifests = elideSkippedLayers(manifests, endTime)
 		if err := validateContinuity(manifests); err != nil {
 			return nil, err
 		}
 		return manifests, nil
 	}
+
+	var closestEndTime hlc.Timestamp
+	var closestTimeDiff time.Duration
+	arbitraryTimeErr := func() error {
+		return errors.Errorf(
+			"invalid RESTORE timestamp: restoring to arbitrary time requires that "+
+				"BACKUP for requested time be created with `revision_history` option."+
+				" nearest backup time is %s",
+			closestEndTime.GoTime().UTC(),
+		)
+	}
 	for i, b := range manifests {
-		// Find the backup that covers the requested time.
-		if !(b.Start().Less(endTime) && endTime.LessEq(b.End())) {
+		timeDiff := b.End().GoTime().Sub(endTime.GoTime()).Abs()
+		if closestEndTime.IsEmpty() || timeDiff < closestTimeDiff {
+			closestEndTime = b.End()
+			closestTimeDiff = timeDiff
+		}
+
+		if b.End().Less(endTime) {
+			// This backup precedes the requested time, so continue.
 			continue
+		} else if endTime.LessEq(b.Start()) {
+			// We've overshot the requested time, so no subsequent backups will cover
+			// it. This implies the user has requested an arbitrary time without a
+			// covering revision history backup.
+			return nil, arbitraryTimeErr()
 		}
 
-		// Ensure that the backup actually has revision history.
+		// At this point, we know that b.Start() < endTime <= b.End(). If endTime !=
+		// b.End(), then this backup only covers the time if it is a revision
+		// history backup.
 		if !endTime.Equal(b.End()) {
 			if b.MVCC() != backuppb.MVCCFilter_All {
-				const errPrefix = "invalid RESTORE timestamp: restoring to arbitrary time requires that BACKUP for requested time be created with 'revision_history' option."
-				if i == 0 {
-					return nil, errors.Errorf(
-						errPrefix+" nearest backup time is %s",
-						timeutil.Unix(0, b.End().WallTime).UTC(),
-					)
+				if i < len(manifests)-1 {
+					// This backup is not a revision history backup, but subsequent
+					// backups might be (e.g. revision history backup chain that contains
+					// compacted backups). We continue to search.
+					continue
 				}
-				return nil, errors.Errorf(
-					errPrefix+" nearest BACKUP times are %s or %s",
-					timeutil.Unix(0, manifests[i-1].End().WallTime).UTC(),
-					timeutil.Unix(0, b.End().WallTime).UTC(),
-				)
+				return nil, arbitraryTimeErr()
 			}
+
 			// Ensure that the revision history actually covers the requested time -
 			// while the BACKUP's start and end might contain the requested time for
 			// example if start time is 0 (full backup), the revision history was
@@ -1007,16 +1027,18 @@ func ValidateEndTimeAndTruncate[E ManifestLike](
 				)
 			}
 		}
+
 		if includeSkipped {
 			return manifests[:i+1], nil
 		}
-		manifests = elideSkippedLayers(manifests[:i+1])
+		manifests = elideSkippedLayers(manifests[:i+1], endTime)
 		if err := validateContinuity(manifests); err != nil {
 			return nil, err
 		}
 		return manifests, nil
 	}
 
+	// If we reach here, the requested time is beyond the end time of all backups.
 	return nil, errors.Errorf(
 		"invalid RESTORE timestamp: supplied backups do not cover requested time",
 	)
@@ -1037,7 +1059,7 @@ func skipCompactedBackups[E ManifestLike](manifests []E) []E {
 
 // validateContinuity checks that the backups are continuous.
 //
-// Note: This fucntion assumes the backups are sorted by end time in ascending
+// Note: This function assumes the backups are sorted by end time in ascending
 // order.
 func validateContinuity[E ManifestLike](manifests []E) error {
 	if len(manifests) == 0 {
@@ -1056,12 +1078,48 @@ func validateContinuity[E ManifestLike](manifests []E) error {
 	return nil
 }
 
+// elideCompactedBackupsForRevisionHistory removes compacted backups that will
+// not be used in a revision history restore. Since compacted backups do not
+// contain revision histories, any compacted backup that contains the requested
+// time without matching its end time must be elided. However, all prior
+// compacted backups can be retained. The requestedEnd may be empty to indicate
+// no specified end time.
+//
+// Note: This function assumes that the backups are sorted by end time in
+// increasing order.
+func elideCompactedBackupsForRevisionHistory[E ManifestLike](
+	manifests []E, requestedEnd hlc.Timestamp,
+) []E {
+	if requestedEnd.IsEmpty() {
+		return manifests
+	}
+	for i := len(manifests) - 1; i >= 0; i-- {
+		// Since manifests are sorted by end time ascending, once we reach a backup
+		// whose end time is less than the requested end time, we can stop. No
+		// subsequent backups will cover.
+		if manifests[i].End().Less(requestedEnd) {
+			break
+		}
+		if !manifests[i].Compacted() {
+			continue
+		}
+		// If the compacted backup covers the end time but does not end at the end
+		// time specifically, it cannot be used in the restore.
+		if manifests[i].Start().Less(requestedEnd) && !requestedEnd.Equal(manifests[i].End()) {
+			manifests = slices.Delete(manifests, i, i+1)
+		}
+	}
+	return manifests
+}
+
 // elideSkippedLayers removes backups that are skipped in the backup chain and
-// ensures only backups that will be used in the restore are returned.
+// ensures only backups that will be used in the restore are returned. The
+// provided requestedEnd may be empty to indicate no specified end time.
 //
 // Note: This function assumes that the manifests in the chain are sorted by end
 // time in ascending order and ties are broken by start time in ascending order.
-func elideSkippedLayers[E ManifestLike](manifests []E) []E {
+func elideSkippedLayers[E ManifestLike](manifests []E, requestedEnd hlc.Timestamp) []E {
+	manifests = elideCompactedBackupsForRevisionHistory(manifests, requestedEnd)
 	manifests = elideDuplicateEndTimes(manifests)
 	i := len(manifests) - 1
 	for i > 0 {

pkg/backup/backupinfo/manifest_handling_test.go

@@ -441,21 +441,76 @@ func TestValidateEndTimeAndTruncate(t *testing.T) {
 			err:     "supplied backups do not cover requested time",
 		},
 		{
-			name: "revision history restore should fail on non-revision history backups",
+			name: "rev history restore should fail on non-rev history backups",
 			manifests: []backuppb.BackupManifest{
 				mNorm(0, 2), mNorm(2, 4),
 			},
 			endTime: 3,
 			err:     "restoring to arbitrary time",
 		},
 		{
-			name: "revision history restore should succeed on revision history backups",
+			name: "rev history restore should succeed on rev history backups",
 			manifests: []backuppb.BackupManifest{
 				mRev(0, 2), mRev(2, 4),
 			},
 			endTime:  3,
 			expected: [][]int{{0, 2}, {2, 4}},
 		},
+		{
+			name: "rev history restore to aost after compacted backup should succeed",
+			manifests: []backuppb.BackupManifest{
+				mRev(0, 2), mRev(2, 4), mRev(4, 6), mComp(2, 8), mRev(6, 8), mRev(8, 10),
+			},
+			endTime:          9,
+			includeCompacted: true,
+			expected: [][]int{
+				{0, 2}, {2, 8}, {8, 10},
+			},
+		},
+		{
+			name: "rev history restore to aost of compacted backup should include compacted backup",
+			manifests: []backuppb.BackupManifest{
+				mRev(0, 2), mRev(2, 4), mRev(4, 6), mComp(2, 8), mRev(6, 8), mRev(8, 10),
+			},
+			endTime:          8,
+			includeCompacted: true,
+			expected: [][]int{
+				{0, 2}, {2, 8},
+			},
+		},
+		{
+			name: "rev history restore to aost in middle of compacted backup should succeed",
+			manifests: []backuppb.BackupManifest{
+				mRev(0, 2), mRev(2, 4), mRev(4, 6), mComp(2, 8), mRev(6, 8), mRev(8, 10),
+			},
+			endTime:          7,
+			includeCompacted: true,
+			expected: [][]int{
+				{0, 2}, {2, 4}, {4, 6}, {6, 8},
+			},
+		},
+		{
+			name: "rev history restore to aost in middle of compacted backup with two compactions",
+			manifests: []backuppb.BackupManifest{
+				mRev(0, 2), mRev(2, 4), mRev(4, 6), mComp(2, 8), mComp(4, 8), mRev(6, 8), mRev(8, 10),
+			},
+			endTime:          7,
+			includeCompacted: true,
+			expected: [][]int{
+				{0, 2}, {2, 4}, {4, 6}, {6, 8},
+			},
+		},
+		{
+			name: "rev history restore should still use preceding compacted backups",
+			manifests: []backuppb.BackupManifest{
+				mRev(0, 2), mRev(2, 4), mComp(2, 6), mRev(4, 6), mRev(6, 8), mComp(6, 10), mRev(8, 10),
+			},
+			endTime:          9,
+			includeCompacted: true,
+			expected: [][]int{
+				{0, 2}, {2, 6}, {6, 8}, {8, 10},
+			},
+		},
 		{
 			name: "end time in middle of chain should truncate",
 			manifests: []backuppb.BackupManifest{