Skip to content

backup: ValidateEndTimeAndTruncate now supports compacted backups with rev history #158709

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kev-cao wants to merge 1 commit into
base: master
Choose a base branch
from
Open

backup: ValidateEndTimeAndTruncate now supports compacted backups with rev history #158709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 77 additions & 19 deletions pkg/backup/backupinfo/manifest_handling.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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",
)
Expand All @@ -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 {
Expand All @@ -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.
Comment on lines +1097 to +1099
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The comment could be clearer. "Subsequent backups" is ambiguous when iterating backwards. Consider: "Since manifests are sorted by end time ascending and we're iterating backwards, once we reach a backup whose end time is less than the requested end time, we can stop. All remaining backups (with lower indices) will have even earlier end times and cannot cover the requested time."

Suggested change
// 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.
// Since manifests are sorted by end time ascending and we're iterating backwards,
// once we reach a backup whose end time is less than the requested end time, we can stop.
// All remaining backups (with lower indices) will have even earlier end times and cannot cover the requested time.

Copilot uses AI. Check for mistakes.
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 {
Expand Down
59 changes: 57 additions & 2 deletions pkg/backup/backupinfo/manifest_handling_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
Loading