Skip to content

Commit

Permalink
storage: Incrementally calculate range key stats in CheckSSTConflicts
Browse files Browse the repository at this point in the history 
This change updates CheckSSTConflicts to incrementally
calculate stats in the presence of range keys in the
SST being ingested. This avoids expensive stats recomputation
after AddSSTable, as previously we were marking stats as
estimates if an SST with range keys was added.

Fixes #83405.

Release note: None.
  • Loading branch information
@itsbilal
itsbilal committed Aug 11, 2022
1 parent 5845f4b commit d7f523a
Show file tree
Hide file tree
Showing 7 changed files with 467 additions and 56 deletions.
5 changes: 4 additions & 1 deletion pkg/kv/kvserver/batcheval/cmd_add_sstable.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,10 @@ func EvalAddSSTable(
// bounded with a small number on the SST side.
usePrefixSeek = usePrefixSeek || args.MVCCStats.KeyCount < 100
}
statsDelta, err = storage.CheckSSTConflicts(ctx, sst, readWriter, start, end,
desc := cArgs.EvalCtx.Desc()
leftPeekBound, rightPeekBound := rangeTombstonePeekBounds(
args.Key, args.EndKey, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey())
statsDelta, err = storage.CheckSSTConflicts(ctx, sst, readWriter, start, end, leftPeekBound, rightPeekBound,
args.DisallowShadowing, args.DisallowShadowingBelow, maxIntents, usePrefixSeek)
if err != nil {
return result.Result{}, errors.Wrap(err, "checking for key collisions")
Expand Down
123 changes: 98 additions & 25 deletions pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -739,11 +739,16 @@ func TestEvalAddSSTable(t *testing.T) {
},
// MVCC Range tombstone cases.
"DisallowConflicts allows sst range keys": {
noConflict: true,
expectStatsEst: true,
data: kvs{pointKV("a", 6, "d")},
sst: kvs{rangeKV("a", "b", 8, ""), pointKV("a", 7, "a8")},
expect: kvs{rangeKV("a", "b", 8, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d")},
noConflict: true,
data: kvs{pointKV("a", 6, "d")},
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 fragmented sst range keys": {
noConflict: true,
data: kvs{pointKV("a", 6, "d")},
sst: kvs{rangeKV("a", "b", 8, ""), pointKV("a", 7, "a8"), rangeKV("c", "d", 8, "")},
expect: kvs{rangeKV("a", "b", 8, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d"), rangeKV("c", "d", 8, "")},
},
"DisallowConflicts disallows sst range keys below engine point key": {
noConflict: true,
Expand All @@ -764,11 +769,76 @@ func TestEvalAddSSTable(t *testing.T) {
expectErr: &roachpb.WriteTooOldError{},
},
"DisallowConflicts allows sst range keys above engine range keys": {
noConflict: true,
expectStatsEst: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("a", "b", 5, "")},
sst: kvs{pointKV("a", 7, "a8"), rangeKV("a", "b", 8, "")},
expect: kvs{rangeKV("a", "b", 8, ""), rangeKV("a", "b", 5, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d")},
noConflict: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("a", "b", 5, "")},
sst: kvs{pointKV("a", 7, "a8"), rangeKV("a", "b", 8, "")},
expect: kvs{rangeKV("a", "b", 8, ""), rangeKV("a", "b", 5, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d")},
},
"DisallowConflicts allows fragmented sst range keys above engine range keys": {
noConflict: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("a", "b", 5, "")},
sst: kvs{pointKV("a", 7, "a8"), rangeKV("a", "b", 8, ""), rangeKV("c", "d", 8, "")},
expect: kvs{rangeKV("a", "b", 8, ""), rangeKV("a", "b", 5, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d"), rangeKV("c", "d", 8, "")},
},
"DisallowConflicts allows fragmented straddling sst range keys": {
noConflict: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("b", "d", 5, "")},
sst: kvs{pointKV("a", 7, "a8"), rangeKV("a", "c", 8, ""), rangeKV("c", "d", 7, "")},
expect: kvs{rangeKV("a", "b", 8, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d"), rangeKV("b", "c", 8, ""), rangeKV("b", "c", 5, ""), rangeKV("c", "d", 7, ""), rangeKV("c", "d", 5, "")},
},
"DisallowConflicts allows fragmented straddling sst range keys with no points": {
noConflict: true,
data: kvs{rangeKV("b", "d", 5, "")},
sst: kvs{rangeKV("a", "c", 8, ""), rangeKV("c", "d", 7, "")},
expect: kvs{rangeKV("a", "b", 8, ""), rangeKV("b", "c", 8, ""), rangeKV("b", "c", 5, ""), rangeKV("c", "d", 7, ""), rangeKV("c", "d", 5, "")},
},
"DisallowConflicts allows engine range keys contained within sst range keys": {
noConflict: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("b", "d", 5, "")},
sst: kvs{pointKV("a", 7, "a8"), rangeKV("a", "e", 8, "")},
expect: kvs{rangeKV("a", "b", 8, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d"), rangeKV("b", "d", 8, ""), rangeKV("b", "d", 5, ""), rangeKV("d", "e", 8, "")},
},
"DisallowConflicts does not skip over engine range keys covering no sst points": {
noConflict: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("b", "c", 6, ""), rangeKV("c", "d", 5, "")},
sst: kvs{pointKV("a", 7, "a8"), rangeKV("a", "e", 8, "")},
expect: kvs{rangeKV("a", "b", 8, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d"), rangeKV("b", "c", 8, ""), rangeKV("b", "c", 6, ""), rangeKV("c", "d", 8, ""), rangeKV("c", "d", 5, ""), rangeKV("d", "e", 8, "")},
},
"DisallowConflicts does not allow conflict with engine range key covering no sst points": {
noConflict: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("b", "c", 9, ""), rangeKV("c", "d", 5, "")},
sst: kvs{pointKV("a", 7, "a8"), rangeKV("a", "e", 8, "")},
expectErr: &roachpb.WriteTooOldError{},
},
"DisallowConflicts allows sst range keys contained within engine range keys": {
noConflict: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("a", "e", 5, "")},
sst: kvs{pointKV("a", 7, "a8"), rangeKV("b", "d", 8, "")},
expect: kvs{rangeKV("a", "b", 5, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d"), rangeKV("b", "d", 8, ""), rangeKV("b", "d", 5, ""), rangeKV("d", "e", 5, "")},
},
"DisallowConflicts allows sst range key fragmenting engine range keys": {
noConflict: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("a", "c", 5, ""), rangeKV("c", "e", 6, "")},
sst: kvs{pointKV("a", 7, "a8"), rangeKV("b", "d", 8, "")},
expect: kvs{rangeKV("a", "b", 5, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d"), rangeKV("b", "c", 8, ""), rangeKV("b", "c", 5, ""), rangeKV("c", "d", 8, ""), rangeKV("c", "d", 6, ""), rangeKV("d", "e", 6, "")},
},
"DisallowConflicts calculates stats correctly for merged range keys": {
noConflict: true,
data: kvs{rangeKV("a", "c", 8, ""), pointKV("a", 6, "d"), rangeKV("d", "e", 8, "")},
sst: kvs{pointKV("a", 10, "de"), rangeKV("c", "d", 8, ""), pointKV("f", 10, "de")},
expect: kvs{rangeKV("a", "e", 8, ""), pointKV("a", 10, "de"), pointKV("a", 6, "d"), pointKV("f", 10, "de")},
},
"DisallowConflicts calculates stats correctly for merged range keys 2": {
noConflict: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("c", "d", 8, "")},
sst: kvs{rangeKV("a", "c", 8, ""), rangeKV("d", "e", 8, ""), pointKV("f", 8, "foo")},
expect: kvs{rangeKV("a", "e", 8, ""), pointKV("a", 6, "d"), pointKV("f", 8, "foo")},
},
"DisallowConflicts calculates stats correctly for merged range keys 3": {
noConflict: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("c", "d", 8, ""), rangeKV("e", "f", 8, "")},
sst: kvs{rangeKV("a", "c", 8, ""), rangeKV("d", "e", 8, ""), pointKV("g", 8, "foo")},
expect: kvs{rangeKV("a", "f", 8, ""), pointKV("a", 6, "d"), pointKV("g", 8, "foo")},
},
"DisallowShadowing disallows sst range keys shadowing live keys": {
noShadow: true,
Expand All @@ -783,11 +853,16 @@ func TestEvalAddSSTable(t *testing.T) {
expect: kvs{rangeKV("a", "b", 7, ""), pointKV("a", 8, "a8"), pointKV("a", 6, "d")},
},
"DisallowShadowing allows idempotent range tombstones": {
noShadow: true,
expectStatsEst: true,
data: kvs{rangeKV("a", "b", 7, "")},
sst: kvs{rangeKV("a", "b", 7, "")},
expect: kvs{rangeKV("a", "b", 7, "")},
noShadow: true,
data: kvs{rangeKV("a", "b", 7, "")},
sst: kvs{rangeKV("a", "b", 7, "")},
expect: kvs{rangeKV("a", "b", 7, "")},
},
"DisallowShadowing calculates stats correctly for merged range keys with idempotence": {
noShadow: true,
data: kvs{rangeKV("b", "d", 8, ""), rangeKV("e", "f", 8, "")},
sst: kvs{rangeKV("a", "c", 8, ""), rangeKV("d", "e", 8, "")},
expect: kvs{rangeKV("a", "f", 8, "")},
},
"DisallowShadowingBelow disallows sst range keys shadowing live keys": {
noShadowBelow: 3,
Expand All @@ -802,18 +877,16 @@ func TestEvalAddSSTable(t *testing.T) {
expect: kvs{rangeKV("a", "b", 7, ""), pointKV("a", 8, "a8"), pointKV("a", 6, "d")},
},
"DisallowShadowingBelow allows idempotent range tombstones": {
noShadowBelow: 3,
expectStatsEst: true,
data: kvs{rangeKV("a", "b", 7, "")},
sst: kvs{rangeKV("a", "b", 7, "")},
expect: kvs{rangeKV("a", "b", 7, "")},
noShadowBelow: 3,
data: kvs{rangeKV("a", "b", 7, "")},
sst: kvs{rangeKV("a", "b", 7, "")},
expect: kvs{rangeKV("a", "b", 7, "")},
},
"DisallowConflict with allowed shadowing disallows idempotent range tombstones": {
noConflict: true,
expectStatsEst: true,
data: kvs{rangeKV("a", "b", 7, "")},
sst: kvs{rangeKV("a", "b", 7, "")},
expectErr: "ingested range key collides with an existing one",
noConflict: true,
data: kvs{rangeKV("a", "b", 7, "")},
sst: kvs{rangeKV("a", "b", 7, "")},
expectErr: "ingested range key collides with an existing one",
},
}
testutils.RunTrueAndFalse(t, "IngestAsWrites", func(t *testing.T, ingestAsWrites bool) {
Expand Down
7 changes: 0 additions & 7 deletions pkg/roachpb/api.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1748,8 +1748,6 @@ message AddSSTableRequest {
// reader has already observed, changing the value at that timestamp and above
// it. Use with SSTTimestampToRequestTimestamp to guarantee serializability.
//
// MVCC range tombstones are not currently supported with DisallowConflicts.
//
// Added in 22.1, so check the MVCCAddSSTable version gate before using.
//
// TODO(erikgrinaker): It might be possible to avoid this parameter if we
Expand All @@ -1762,8 +1760,6 @@ message AddSSTableRequest {
// writing above keys that have an existing/visible value (but will write
// above tombstones).
//
// MVCC range tombstones are not currently supported with DisallowShadowing.
//
// TODO(erikgrinaker): Consider removing this in 22.1 if all callers have
// been migrated to DisallowShadowingBelow.
bool disallow_shadowing = 3;
Expand All @@ -1785,9 +1781,6 @@ message AddSSTableRequest {
// If this parameter is used, the value of DisallowShadowing is ignored, so
// callers may pass both for forward and backwards compatibility.
//
// MVCC range tombstones are not currently supported with
// DisallowShadowingBelow.
//
// Added in 22.1, so check the MVCCAddSSTable version gate before using.
util.hlc.Timestamp disallow_shadowing_below = 8 [(gogoproto.nullable) = false];

Expand Down
2 changes: 1 addition & 1 deletion pkg/storage/bench_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1738,7 +1738,7 @@ func runCheckSSTConflicts(

b.ResetTimer()
for i := 0; i < b.N; i++ {
_, err := CheckSSTConflicts(context.Background(), sstFile.Data(), eng, sstStart, sstEnd, false, hlc.Timestamp{}, math.MaxInt64, usePrefixSeek)
_, err := CheckSSTConflicts(context.Background(), sstFile.Data(), eng, sstStart, sstEnd, nil, nil, false, hlc.Timestamp{}, math.MaxInt64, usePrefixSeek)
require.NoError(b, err)
}
}
Expand Down
14 changes: 14 additions & 0 deletions pkg/storage/mvcc_key.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -661,6 +661,20 @@ func (v MVCCRangeKeyVersions) Covers(ts hlc.Timestamp) bool {
return !v.IsEmpty() && ts.LessEq(v[0].Timestamp)
}

// Equal returns whether versions in the specified MVCCRangeKeyVersions match
// exactly (in timestamps and values) with those in itself.
func (v MVCCRangeKeyVersions) Equal(other MVCCRangeKeyVersions) bool {
if len(v) != len(other) {
return false
}
for i := range v {
if !v[i].Equal(other[i]) {
return false
}
}
return true
}

// Excise removes the versions in the given [from, to] span (inclusive, in
// order) in place, returning true if any versions were removed.
func (v *MVCCRangeKeyVersions) Excise(from, to hlc.Timestamp) bool {
Expand Down
Loading

0 comments on commit d7f523a

Please sign in to comment.