Skip to content

Commit

Permalink
Merge #98426
Browse files Browse the repository at this point in the history 
98426: storage: Fix CheckSSTConflicts stats calculations r=erikgrinaker a=itsbilal

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

Co-authored-by: Bilal Akhtar <bilal@cockroachlabs.com>
  • Loading branch information
@itsbilal
craig[bot] and itsbilal committed Mar 12, 2023
2 parents 9568438 + 7d00079 commit 2f2e85d
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 2f2e85d

Please sign in to comment.