backupccl: simplify index span merging logic for backup #72293
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change simplifies the index span merging logic used by BACKUP when identifying what spans to backup and protect. Previously, we would attempt to "logically" merge spans by checking if any dropped indexes that have been GC'ed can be merged over. This has been the cause of a few subtle bugs as outlined in cockroachdb#72263. This change simplifies the merging logic to only merge adjacent, public index spans into a single span. If there exist any non-public index IDs between the two public index IDs, we no longer attempt to merge. In the presence of dropped indexes this will result in more spans being backed up and protected than before, but we believe that the simplification is worth losing the optimization in this case. This change also fixes an existing bug in the merging logic where two (or more) public indexes followed by an interleaved index would result in certain index keys being missed during backup. A regression test has been added to this effect. Fixes: cockroachdb#72263 Release note (bug fix): This change fixes two bugs in the logic that optimized the number of spans to backup.
adityamaru
requested review from
stevendanna and
a team
and removed request for
a team
|
This change is |
|
This is a forward port of #72270. |
stevendanna
approved these changes
Nov 1, 2021
|
Filed #72321 for the bazel failure. I want to get this baking on master before we merge the backport so I'm going ahead with the merge. TFTR! bors r=stevendanna |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change simplifies the index span merging logic used
by BACKUP when identifying what spans to backup and protect.
Previously, we would attempt to "logically" merge spans by
checking if any dropped indexes that have been GC'ed can be
merged over.
This has been the cause of a few subtle bugs as outlined in
#72263.
This change simplifies the merging logic to only merge
adjacent, public index spans into a single span. If there
exist any non-public index IDs between the two public index
IDs, we no longer attempt to merge. In the presence of
dropped indexes this will result in more spans being backed
up and protected than before, but we believe that the simplification
is worth losing the optimization in this case.
This change also fixes an existing bug in the merging logic
where two (or more) public indexes followed by an interleaved
index would result in certain index keys being missed during
backup. A regression test has been added to this effect.
Fixes: #72263
Release note (bug fix): This change fixes two bugs in the
logic that optimized the number of spans to backup.