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 5, 2022
1 parent 3c96ef2 commit d10c6e9
Show file tree
Hide file tree
Showing 6 changed files with 251 additions and 52 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()
_, rightPeekBound := rangeTombstonePeekBounds(
args.Key, args.EndKey, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey())
statsDelta, err = storage.CheckSSTConflicts(ctx, sst, readWriter, start, end, rightPeekBound,
args.DisallowShadowing, args.DisallowShadowingBelow, maxIntents, usePrefixSeek)
if err != nil {
return result.Result{}, errors.Wrap(err, "checking for key collisions")
Expand Down
93 changes: 68 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,52 @@ 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 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, "")},
},
"DisallowShadowing disallows sst range keys shadowing live keys": {
noShadow: true,
Expand All @@ -783,11 +829,10 @@ 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, "")},
},
"DisallowShadowingBelow disallows sst range keys shadowing live keys": {
noShadowBelow: 3,
Expand All @@ -802,18 +847,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 @@ -1726,8 +1726,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 @@ -1740,8 +1738,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 @@ -1763,9 +1759,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 @@ -1613,7 +1613,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, sstEnd.Key.Next(), false, hlc.Timestamp{}, math.MaxInt64, usePrefixSeek)
require.NoError(b, err)
}
}
Expand Down
Loading

0 comments on commit d10c6e9

Please sign in to comment.