Skip to content

Commit

Permalink
storage: Fix GCBytesAge in CheckSSTConflicts with range keys
Browse files Browse the repository at this point in the history 
Change #87303 fixed GCBytesAge in `CheckSSTConflicts` for
points, but not for range keys. This change fixes it for
range keys too, by ensuring the changes to statsDiff happen
at the right timestamp using statsDiff.AgeTo.

Fixes #82920.

Release note: None.
  • Loading branch information
@itsbilal
itsbilal committed Sep 26, 2022
1 parent 2c18ed3 commit 98af860
Show file tree
Hide file tree
Showing 2 changed files with 171 additions and 98 deletions.
166 changes: 80 additions & 86 deletions pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,19 +53,18 @@ func TestEvalAddSSTable(t *testing.T) {
// These are run with IngestAsWrites both disabled and enabled, and
// kv.bulk_io_write.sst_rewrite_concurrency.per_call of 0 and 8.
testcases := map[string]struct {
data kvs
sst kvs
reqTS int64
toReqTS int64 // SSTTimestampToRequestTimestamp with given SST timestamp
noConflict bool // DisallowConflicts
noShadow bool // DisallowShadowing
noShadowBelow int64 // DisallowShadowingBelow
requireReqTS bool // AddSSTableRequireAtRequestTimestamp
expect kvs
expectErr interface{} // error type, substring, substring slice, or true (any)
expectErrRace interface{}
expectStatsEst bool // expect MVCCStats.ContainsEstimates, don't check stats
expectGCBytesEst bool // expect MVCCStats.GCBytesAge to be inaccurate.
data kvs
sst kvs
reqTS int64
toReqTS int64 // SSTTimestampToRequestTimestamp with given SST timestamp
noConflict bool // DisallowConflicts
noShadow bool // DisallowShadowing
noShadowBelow int64 // DisallowShadowingBelow
requireReqTS bool // AddSSTableRequireAtRequestTimestamp
expect kvs
expectErr interface{} // error type, substring, substring slice, or true (any)
expectErrRace interface{}
expectStatsEst bool // expect MVCCStats.ContainsEstimates, don't check stats
}{
// Blind writes.
"blind writes below existing": {
Expand Down Expand Up @@ -771,46 +770,52 @@ func TestEvalAddSSTable(t *testing.T) {
expectErr: &roachpb.WriteTooOldError{},
},
"DisallowConflicts allows 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, "")},
expect: kvs{rangeKV("a", "b", 8, ""), rangeKV("a", "b", 5, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d")},
expectGCBytesEst: true,
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, "")},
expectGCBytesEst: true,
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, "")},
expectGCBytesEst: true,
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, "")},
expectGCBytesEst: true,
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, "")},
expectGCBytesEst: true,
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 allows engine range keys contained within sst range keys 2": {
noConflict: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("b", "d", 5, "")},
sst: kvs{pointKV("a", 7, "a8"), rangeKV("a", "d", 8, "")},
expect: kvs{rangeKV("a", "b", 8, ""), pointKV("a", 7, "a8"), pointKV("a", 6, "d"), rangeKV("b", "d", 8, ""), rangeKV("b", "d", 5, "")},
},
"DisallowConflicts allows engine range keys contained within sst range keys 3": {
noConflict: true,
data: kvs{pointKV("a", 6, "d"), rangeKV("a", "d", 5, "")},
sst: kvs{pointKV("a", 7, "a8"), rangeKV("b", "e", 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", 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, "")},
expectGCBytesEst: true,
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,
Expand All @@ -819,25 +824,22 @@ func TestEvalAddSSTable(t *testing.T) {
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, "")},
expectGCBytesEst: true,
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, "")},
expectGCBytesEst: true,
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")},
expectGCBytesEst: true,
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,
Expand All @@ -858,25 +860,22 @@ func TestEvalAddSSTable(t *testing.T) {
expectErr: "ingested range key collides with an existing one",
},
"DisallowShadowing allows shadowing of keys deleted by engine range tombstones": {
noShadow: true,
data: kvs{rangeKV("a", "b", 7, ""), pointKV("a", 6, "d")},
sst: kvs{pointKV("a", 8, "a8")},
expect: kvs{rangeKV("a", "b", 7, ""), pointKV("a", 8, "a8"), pointKV("a", 6, "d")},
expectGCBytesEst: true,
noShadow: true,
data: kvs{rangeKV("a", "b", 7, ""), pointKV("a", 6, "d")},
sst: kvs{pointKV("a", 8, "a8")},
expect: kvs{rangeKV("a", "b", 7, ""), pointKV("a", 8, "a8"), pointKV("a", 6, "d")},
},
"DisallowShadowing allows idempotent range tombstones": {
noShadow: true,
data: kvs{rangeKV("a", "b", 7, "")},
sst: kvs{rangeKV("a", "b", 7, "")},
expect: kvs{rangeKV("a", "b", 7, "")},
expectGCBytesEst: true,
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, "")},
expectGCBytesEst: true,
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 @@ -885,18 +884,16 @@ func TestEvalAddSSTable(t *testing.T) {
expectErr: "ingested range key collides with an existing one",
},
"DisallowShadowingBelow allows shadowing of keys deleted by engine range tombstones": {
noShadowBelow: 3,
data: kvs{rangeKV("a", "b", 7, ""), pointKV("a", 6, "d")},
sst: kvs{pointKV("a", 8, "a8")},
expect: kvs{rangeKV("a", "b", 7, ""), pointKV("a", 8, "a8"), pointKV("a", 6, "d")},
expectGCBytesEst: true,
noShadowBelow: 3,
data: kvs{rangeKV("a", "b", 7, ""), pointKV("a", 6, "d")},
sst: kvs{pointKV("a", 8, "a8")},
expect: kvs{rangeKV("a", "b", 7, ""), pointKV("a", 8, "a8"), pointKV("a", 6, "d")},
},
"DisallowShadowingBelow allows idempotent range tombstones": {
noShadowBelow: 3,
data: kvs{rangeKV("a", "b", 7, "")},
sst: kvs{rangeKV("a", "b", 7, "")},
expect: kvs{rangeKV("a", "b", 7, "")},
expectGCBytesEst: true,
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,
Expand Down Expand Up @@ -1070,9 +1067,6 @@ func TestEvalAddSSTable(t *testing.T) {
} else {
require.Zero(t, stats.ContainsEstimates, "found estimated stats")
expected := storageutils.EngineStats(t, engine, stats.LastUpdateNanos)
if tc.expectGCBytesEst && expected != nil {
expected.GCBytesAge = stats.GCBytesAge
}
require.Equal(t, expected, stats)
}
})
Expand Down
Loading

0 comments on commit 98af860

Please sign in to comment.