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

storage: Incrementally calculate range key stats in CheckSSTConflicts #85673

Merged
merged 1 commit into from
Aug 13, 2022
+467 −56
Merged

storage: Incrementally calculate range key stats in CheckSSTConflicts #85673

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
5 changes: 4 additions & 1 deletion pkg/kv/kvserver/batcheval/cmd_add_sstable.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1722,7 +1722,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, sstStart.Key, sstEnd.Key.Next(), 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
Open in desktop
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