Skip to content

Conversation

@kev-cao
Copy link
Contributor

@kev-cao kev-cao commented Dec 3, 2025

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

@kev-cao kev-cao requested a review from a team as a code owner December 3, 2025 21:48
@kev-cao kev-cao requested review from dt and removed request for a team December 3, 2025 21:48
@cockroach-teamcity
Copy link
Member

This change is Reviewable

…h 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: cockroachdb#158696

Release note: None
@kev-cao kev-cao force-pushed the backup/truncate-rev-history-compactions branch from fb2e490 to 12a500e Compare December 4, 2025 15:48
@kev-cao kev-cao requested a review from Copilot December 5, 2025 16:02
Copilot finished reviewing on behalf of kev-cao December 5, 2025 16:04
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances the ValidateEndTimeAndTruncate function to properly handle revision history backup trees that contain compacted backups. Previously, the function assumed revision history backup trees would not contain compacted backups. The new implementation intelligently handles mixed trees by eliding compacted backups that cover the requested time but don't match its end time exactly, while retaining preceding compacted backups that can be used in the restore operation.

Key changes:

  • Modified ValidateEndTimeAndTruncate to continue searching through the backup chain when encountering non-revision history backups (e.g., compacted backups), rather than failing immediately
  • Added new elideCompactedBackupsForRevisionHistory function to selectively remove compacted backups that cannot be used for the requested restore time
  • Updated elideSkippedLayers signature to accept the requested end time, enabling proper filtering of compacted backups in revision history restores

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
pkg/backup/backupinfo/manifest_handling.go Implements core logic changes to support compacted backups in revision history trees, including new helper function and improved error messages. Also fixes spelling error ("fucntion" → "function").
pkg/backup/backupinfo/manifest_handling_test.go Adds comprehensive test coverage for various scenarios involving compacted backups in revision history trees, including edge cases with multiple compactions and different AOST positions. Test names abbreviated for consistency.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1097 to +1099
// 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.
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.
@kev-cao
Copy link
Contributor Author

kev-cao commented Dec 5, 2025

Holding off on merging this PR until after branch cut.

@kev-cao kev-cao added the do-not-merge bors won't merge a PR with this label. label Dec 5, 2025
@msbutler msbutler self-requested a review December 5, 2025 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge bors won't merge a PR with this label.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants