Incremental backup: fix calculation of binlog files to use#13066
Incremental backup: fix calculation of binlog files to use#13066shlomi-noach merged 5 commits intovitessio:mainfrom
Conversation
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
vitess-bot
bot
added
NeedsDescriptionUpdate
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
If a new flag is being introduced:
If a workflow is added or modified:
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
shlomi-noach
removed
NeedsDescriptionUpdate
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
LGTM! Only had a few very minor nits/suggestions. I'll leave those up to you.
go/vt/mysqlctl/binlogs_gtid.go
Outdated
| if unionPreviousGTIDs { | ||
| prevPos.GTIDSet = prevGTIDsUnion | ||
| if i == 0 { | ||
| return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "the very first binlog file %v has PreviousGTIDs %s that exceed given incremental backup pos. There are GTID entries that are missing and this backup cannot run", binlog, previousGTIDsPos) |
There was a problem hiding this comment.
Might be worth using the GTID_PURGED language/variable specifically to aid in debugging and bridging the MySQL<->Vitess concepts/pieces here.
go/vt/mysqlctl/binlogs_gtid.go
Outdated
| if i == 0 { | ||
| return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "the very first binlog file %v has PreviousGTIDs %s that exceed given incremental backup pos. There are GTID entries that are missing and this backup cannot run", binlog, prevPos) | ||
| if backupFromGTIDSet.Contains(previousGTIDsPos.GTIDSet) { | ||
| // All previous binary logs are fully contained by backupPos. So definitely all binlogs _prior_ to |
There was a problem hiding this comment.
IMO it's easier to reason about when explained as the current binary log being the oldest one available -- so all previous GTIDs have been purged and are no longer available.
go/vt/mysqlctl/binlogs_gtid_test.go
Outdated
| }, | ||
| backupPos: "16b1039f-22b6-11ed-b765-0a43f95f28a3:1-63", | ||
| gtidPurged: "16b1039f-22b6-11ed-b765-0a43f95f28a3:1-2", | ||
| expectError: "There are GTID entries that are missing", |
There was a problem hiding this comment.
Nit, but [purged and] no longer available feels more accurate than missing.
|
@mattlord please see if rewording makes more sense. I find it really difficult to explain the constraints in proper English. Please feel free to edit. |
@shlomi-noach LGTM! ❤️ |
deepthi
left a comment
deepthi
left a comment
There was a problem hiding this comment.
Only nits on wording etc. The logic looks good to me.
go/vt/mysqlctl/binlogs_gtid.go
Outdated
| return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "binary log %v with previous GTIDS %s neither contains requested GTID %s nor contains it. Backup cannot take place", binlog, prevPos.GTIDSet, lookFromGTIDSet) | ||
| if !prevGTIDsUnion.Union(purgedGTIDSet).Contains(backupFromGTIDSet) { | ||
| return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, | ||
| "Mismatching GTID entries. Requested backup pos has entries not found in the binary logs, and binary logs have entries not found in the requested backup pos. Requested pos=%v, binlog pos=%v", |
There was a problem hiding this comment.
Is it and binary logs or or binary logs?
In other words, does the check imply both conditions have occurred, or that at least one of them has occurred?
You could replace and with and/or.
There was a problem hiding this comment.
It's an and, as described. It's like a Venn diagram of two circles which overlap, but neither fully contains the other.
There was a problem hiding this comment.
Added Neither fully contains the other to the message. I hope it clarifies.
Co-authored-by: Deepthi Sigireddi <deepthi.sigireddi@gmail.com> Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
3 participants
Description
Incremental backup logic introduced in #11097 (still undocumented) did not evaluate the correct list of binary logs when
gtid_purgedhappened to be exactly the last full backup position. There are probably more scenarios affected by the bug.This PR treats
gtid_purgedand the last known backup pos correctly. Tests added.Incremental backup and Point in time recovery as introduced in #11097, are still undocumented.
Related Issue(s)
Checklist
Deployment Notes