Skip to content

Commit

Permalink
storage: Fix CheckSSTConflicts stats calculations
Browse files Browse the repository at this point in the history 
Previously we were missing some cases of range key
overlaps with other range keys, or were too eagerly
stepping over engine keys that did not conflict with
sstable keys, resulting in incorrect stats. This change
adds more targeted test cases to TestEvalAddSSTable to
test for those previously-unaccounted cases, and fixes them
in CheckSSTConflicts.

Informs #94140 and #98408.
Fixes #94141.

Epic: none

Release note: None
  • Loading branch information
@itsbilal
itsbilal committed Mar 12, 2023
1 parent 5b2a567 commit 7d00079
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 38 deletions.
50 changes: 50 additions & 0 deletions pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go
View file
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
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

0 comments on commit 7d00079

Please sign in to comment.