Skip to content
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

Jump to bottom

storage: Fix CheckSSTConflicts stats calculations #98426

Merged
merged 1 commit into from
Mar 12, 2023
Merged

storage: Fix CheckSSTConflicts stats calculations #98426

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
50 changes: 50 additions & 0 deletions pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,6 +808,56 @@ func TestEvalAddSSTable(t *testing.T) {
sst: kvs{rangeKV("a", "b", 8, ""), pointKV("a", 7, "a8")},
expect: kvs{rangeKV("a", "b", 8, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d")},
},
"DisallowConflicts allows sst range keys 2": {
noConflict: true,
reqTS: 10,
data: kvs{pointKV("a", 6, "d"), pointKV("bbb", 6, ""), pointKV("c", 6, "d"), pointKV("d", 6, "d"), pointKV("dd", 6, "")},
sst: kvs{pointKV("aa", 9, ""), pointKV("b", 9, "dee"), rangeKV("bb", "e", 9, ""), pointKV("cc", 10, "")},
expect: kvs{pointKV("a", 6, "d"), pointKV("aa", 9, ""), pointKV("b", 9, "dee"), rangeKV("bb", "e", 9, ""), pointKV("bbb", 6, ""), pointKV("c", 6, "d"), pointKV("cc", 10, ""), pointKV("d", 6, "d"), pointKV("dd", 6, "")},
},
"DisallowConflicts does not skip sst range keys ahead of first one": {
noConflict: true,
reqTS: 10,
toReqTS: 8,
data: kvs{pointKV("a", 6, "d"), pointKV("e", 5, "d")},
sst: kvs{rangeKV("b", "c", 8, ""), rangeKV("cc", "f", 8, "")},
expect: kvs{pointKV("a", 6, "d"), rangeKV("b", "c", 10, ""), rangeKV("cc", "f", 10, ""), pointKV("e", 5, "d")},
},
"DisallowConflicts correctly accounts for complex fragment cases": {
noConflict: true,
reqTS: 10,
data: kvs{rangeKV("b", "c", 7, ""), rangeKV("b", "c", 6, ""), rangeKV("c", "f", 6, ""), rangeKV("f", "g", 7, ""), rangeKV("f", "g", 6, "")},
sst: kvs{rangeKV("a", "d", 8, ""), rangeKV("e", "g", 8, "")},
expect: kvs{rangeKV("a", "b", 8, ""), rangeKV("b", "c", 8, ""), rangeKV("b", "c", 7, ""), rangeKV("b", "c", 6, ""), rangeKV("c", "d", 8, ""), rangeKV("c", "d", 6, ""), rangeKV("d", "e", 6, ""), rangeKV("e", "f", 8, ""), rangeKV("e", "f", 6, ""), rangeKV("f", "g", 8, ""), rangeKV("f", "g", 7, ""), rangeKV("f", "g", 6, "")},
},
"DisallowConflicts correctly accounts for complex fragment cases 2": {
noConflict: true,
reqTS: 10,
data: kvs{rangeKV("a", "g", 6, ""), pointKV("bb", 5, "bar")},
sst: kvs{rangeKV("b", "c", 8, ""), pointKV("cc", 8, "foo"), rangeKV("e", "f", 8, "")},
expect: kvs{rangeKV("a", "b", 6, ""), rangeKV("b", "c", 8, ""), rangeKV("b", "c", 6, ""), pointKV("bb", 5, "bar"), rangeKV("c", "e", 6, ""), pointKV("cc", 8, "foo"), rangeKV("e", "f", 8, ""), rangeKV("e", "f", 6, ""), rangeKV("f", "g", 6, "")},
},
"DisallowConflicts correctly accounts for complex fragment cases 3": {
noConflict: true,
reqTS: 10,
data: kvs{rangeKV("c", "d", 6, ""), pointKV("e", 6, ""), pointKV("e", 5, "foo"), pointKV("f", 6, ""), pointKV("f", 5, ""), pointKV("g", 6, ""), rangeKV("j", "k", 5, "")},
sst: kvs{rangeKV("a", "l", 8, "")},
expect: kvs{rangeKV("a", "c", 8, ""), rangeKV("c", "d", 8, ""), rangeKV("c", "d", 6, ""), rangeKV("d", "j", 8, ""), pointKV("e", 6, ""), pointKV("e", 5, "foo"), pointKV("f", 6, ""), pointKV("f", 5, ""), pointKV("g", 6, ""), rangeKV("j", "k", 8, ""), rangeKV("j", "k", 5, ""), rangeKV("k", "l", 8, "")},
},
"DisallowConflicts correctly accounts for complex fragment cases 4": {
noConflict: true,
reqTS: 10,
data: kvs{rangeKV("c", "d", 6, ""), rangeKV("j", "k", 5, "")},
sst: kvs{rangeKV("a", "l", 8, "")},
expect: kvs{rangeKV("a", "c", 8, ""), rangeKV("c", "d", 8, ""), rangeKV("c", "d", 6, ""), rangeKV("d", "j", 8, ""), rangeKV("j", "k", 8, ""), rangeKV("j", "k", 5, ""), rangeKV("k", "l", 8, "")},
},
"DisallowConflicts accounts for point key already deleted in engine": {
noConflict: true,
reqTS: 10,
data: kvs{rangeKV("b", "d", 6, ""), pointKV("c", 5, "foo")},
sst: kvs{rangeKV("a", "d", 8, "")},
expect: kvs{rangeKV("a", "b", 8, ""), rangeKV("b", "d", 8, ""), rangeKV("b", "d", 6, ""), pointKV("c", 5, "foo")},
},
"DisallowConflicts allows fragmented sst range keys": {
noConflict: true,
data: kvs{pointKV("a", 6, "d")},
Expand Down
72 changes: 34 additions & 38 deletions pkg/storage/sst.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ func CheckSSTConflicts(
//
// TODO(bilal): Close this gap in GCBytesAge calculation, see:
// https://github.com/cockroachdb/cockroach/issues/92254
statsDiff.ContainsEstimates++
statsDiff.ContainsEstimates += 2
}
extIter := reader.NewMVCCIterator(MVCCKeyAndIntentsIterKind, IterOptions{
KeyTypes: IterKeyTypePointsAndRanges,
Expand Down Expand Up @@ -704,24 +704,6 @@ func CheckSSTConflicts(
sstPrevRangeKeys = sstRangeKeys.Clone()
}
if extRangeKeysChanged {
if !extPrevRangeKeys.IsEmpty() && extPrevRangeKeys.Bounds.EndKey.Compare(extRangeKeys.Bounds.Key) < 0 &&
sstHasRange && sstRangeKeys.Bounds.Key.Compare(extRangeKeys.Bounds.Key) < 0 &&
sstRangeKeys.Bounds.Overlaps(extRangeKeys.Bounds) {
// We're adding a fragment between two non-abutting engine range keys.
startKey := extPrevRangeKeys.Bounds.EndKey
if sstRangeKeys.Bounds.Key.Compare(startKey) > 0 {
startKey = sstRangeKeys.Bounds.Key
}
endKey := extRangeKeys.Bounds.Key
newRangeKeyStack := MVCCRangeKeyStack{
Versions: sstRangeKeys.Versions,
Bounds: roachpb.Span{Key: startKey, EndKey: endKey},
}
statsDiff.Add(updateStatsOnRangeKeyPut(newRangeKeyStack))
if extPrevRangeKeys.CanMergeRight(newRangeKeyStack) {
statsDiff.Add(updateStatsOnRangeKeyMerge(startKey, newRangeKeyStack.Versions))
}
}
// Note that we exclude sstRangeKeysChanged below, as this case only
// accounts for additional ext range keys that this SST range key stack
// could be adding versions to. The very first ext range key stack that
Expand Down Expand Up @@ -877,7 +859,7 @@ func CheckSSTConflicts(
if err := compareForCollision(sstKey, extKey, sstValueRaw, extValueRaw); err != nil {
return enginepb.MVCCStats{}, err
}
} else if extValueDeletedByRange {
} else if sstHasPoint && extValueDeletedByRange {
// Don't double-count the current key.
var deletedAt hlc.Timestamp
if _, isTombstone, err := extIter.MVCCValueLenAndIsTombstone(); err != nil {
Expand Down Expand Up @@ -1053,10 +1035,19 @@ func CheckSSTConflicts(
if sstErr != nil {
return enginepb.MVCCStats{}, sstErr
}
if sstOK && !sstIter.UnsafeKey().Key.Equal(oldKey.Key) {
// sstIter stepped one key ahead. Re-seek extIter at this key.
extIter.SeekGE(MVCCKey{Key: sstIter.UnsafeKey().Key})
if sstOK && !sstIter.UnsafeKey().Key.Equal(oldKey.Key) && extIter.UnsafeKey().Key.Compare(sstIter.UnsafeKey().Key) < 0 {
// sstIter stepped one key ahead. Re-seek both iterators at the next
// ext key.
extIter.NextKey()
extOK, extErr = extIter.Valid()
if extOK {
sstIter.SeekGE(MVCCKey{Key: extIter.UnsafeKey().Key})
}
sstOK, sstErr = sstIter.Valid()
if sstOK {
extIter.SeekGE(MVCCKey{Key: sstIter.UnsafeKey().Key})
extOK, extErr = extIter.Valid()
}
}
// If sstIter found a point key at the same MVCC Key, we still need
// to check for conflicts against it.
Expand All @@ -1068,24 +1059,28 @@ func CheckSSTConflicts(
// this logic does not guarantee forward progress in those cases.
sstIter.Next()
sstOK, sstErr = sstIter.Valid()
extIter.Next()
steppedExtIter = true
extOK, extErr = extIter.Valid()
sstChangedKeys := !sstOK || !sstIter.UnsafeKey().Key.Equal(oldKey.Key)
extChangedKeys := !extOK || !extIter.UnsafeKey().Key.Equal(oldKey.Key)
if sstChangedKeys && !extChangedKeys {
sstIter.SeekGE(MVCCKey{Key: extIter.UnsafeKey().Key})
sstOK, sstErr = sstIter.Valid()
}
// Re-seek the ext iterator if the ext iterator changed keys and:
// 1) the SST iterator did not change keys, and we need to bring the ext
// iterator back.
// 2) the ext iterator became invalid
// 3) both iterators changed keys and the sst iterator's key is further
// ahead.
if sstOK && extChangedKeys && (!sstChangedKeys || !extOK || extIter.UnsafeKey().Key.Compare(sstIter.UnsafeKey().Key) < 0) {
if sstOK && sstChangedKeys {
extIter.SeekGE(MVCCKey{Key: sstIter.UnsafeKey().Key})
extOK, extErr = extIter.Valid()
} else {
extIter.Next()
steppedExtIter = true
extOK, extErr = extIter.Valid()
extChangedKeys := !extOK || !extIter.UnsafeKey().Key.Equal(oldKey.Key)
if sstChangedKeys && !extChangedKeys {
sstIter.SeekGE(MVCCKey{Key: extIter.UnsafeKey().Key})
sstOK, sstErr = sstIter.Valid()
}
// Re-seek the ext iterator if the ext iterator changed keys and:
// 1) the SST iterator did not change keys, and we need to bring the ext
// iterator back.
// 2) the ext iterator became invalid
// 3) both iterators changed keys.
if sstOK && extChangedKeys {
extIter.SeekGE(MVCCKey{Key: sstIter.UnsafeKey().Key})
extOK, extErr = extIter.Valid()
}
}
// If both iterators are invalid, we are now done. If one both iterators
// are at point keys under the same MVCC key, then we can check for
Expand All @@ -1107,6 +1102,7 @@ func CheckSSTConflicts(
// This SeekGE is purely to maintain the extIter > sstIter invariant
// as in most cases it'll be a no-op.
extIter.SeekGE(MVCCKey{Key: sstIter.UnsafeKey().Key})
extOK, extErr = extIter.Valid()
}
}
}
Expand Down