Skip to content

Commit

Permalink
Merge #88578 #88736
Browse files Browse the repository at this point in the history 
88578: storage: Fix GCBytesAge in CheckSSTConflicts with range keys r=erikgrinaker a=itsbilal

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.

88736: Unlink func_as and replace with SCONST r=chengxiong-ruan a=stbof

Replacement needed to prevent missing `func_as` target in SQL grammar.

Release note: None
Release justification: low-risk bugfix. Required to build UDF docs.

Co-authored-by: Bilal Akhtar <[email protected]>
Co-authored-by: Stephanie Bodoff <[email protected]>
  • Loading branch information
@itsbilal
3 people committed Sep 27, 2022
3 parents 898987e + 98af860 + 9a596d1 commit a0bfa6d
Show file tree
Hide file tree
Showing 4 changed files with 175 additions and 101 deletions.
2 changes: 1 addition & 1 deletion docs/generated/sql/bnf/create_func_stmt.bnf
View file
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
create_func_stmt ::=
'CREATE' ( 'OR' 'REPLACE' | ) 'FUNCTION' func_create_name '(' ( ( ( ( func_arg | func_arg | func_arg ) ) ( ( ',' ( func_arg | func_arg | func_arg ) ) )* ) | ) ')' 'RETURNS' ( | ) ( func_arg_type ) ( ( ( ( 'AS' func_as | 'LANGUAGE' 'SQL' | ( 'CALLED' 'ON' 'NULL' 'INPUT' | 'RETURNS' 'NULL' 'ON' 'NULL' 'INPUT' | 'STRICT' | 'IMMUTABLE' | 'STABLE' | 'VOLATILE' | 'LEAKPROOF' | 'NOT' 'LEAKPROOF' ) ) ) ( ( ( 'AS' func_as | 'LANGUAGE' 'SQL' | ( 'CALLED' 'ON' 'NULL' 'INPUT' | 'RETURNS' 'NULL' 'ON' 'NULL' 'INPUT' | 'STRICT' | 'IMMUTABLE' | 'STABLE' | 'VOLATILE' | 'LEAKPROOF' | 'NOT' 'LEAKPROOF' ) ) ) )* ) | ) opt_routine_body
'CREATE' ( 'OR' 'REPLACE' | ) 'FUNCTION' func_create_name '(' ( ( ( ( func_arg | func_arg | func_arg ) ) ( ( ',' ( func_arg | func_arg | func_arg ) ) )* ) | ) ')' 'RETURNS' ( | ) ( func_arg_type ) ( ( ( ( 'AS' ( 'SCONST' ) | 'LANGUAGE' 'SQL' | ( 'CALLED' 'ON' 'NULL' 'INPUT' | 'RETURNS' 'NULL' 'ON' 'NULL' 'INPUT' | 'STRICT' | 'IMMUTABLE' | 'STABLE' | 'VOLATILE' | 'LEAKPROOF' | 'NOT' 'LEAKPROOF' ) ) ) ( ( ( 'AS' ( 'SCONST' ) | 'LANGUAGE' 'SQL' | ( 'CALLED' 'ON' 'NULL' 'INPUT' | 'RETURNS' 'NULL' 'ON' 'NULL' 'INPUT' | 'STRICT' | 'IMMUTABLE' | 'STABLE' | 'VOLATILE' | 'LEAKPROOF' | 'NOT' 'LEAKPROOF' ) ) ) )* ) | ) opt_routine_body
5 changes: 3 additions & 2 deletions pkg/cmd/docgen/diagrams.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -765,9 +765,10 @@ var specs = []stmtSpec{
},
{
name: "create_func_stmt",
inline: []string{"opt_or_replace", "opt_func_arg_with_default_list", "opt_return_set", "func_return_type", "opt_create_func_opt_list", "create_func_opt_list", "common_func_opt_item", "create_func_opt_item", "routine_return_stmt", "func_arg_with_default_list", "func_arg_with_default"},
unlink: []string{"opt_or_replace", "opt_func_arg_with_default_list", "opt_return_set", "func_return_type", "opt_create_func_opt_list", "create_func_opt_list", "create_func_opt_item", "common_func_opt_item", "routine_return_stmt", "non_reserved_word_or_sconst", "func_arg_with_default_list", "func_arg_with_default", "a_expr"},
inline: []string{"opt_or_replace", "opt_func_arg_with_default_list", "opt_return_set", "func_return_type", "opt_create_func_opt_list", "create_func_opt_list", "common_func_opt_item", "create_func_opt_item", "routine_return_stmt", "func_arg_with_default_list", "func_arg_with_default", "func_as"},
unlink: []string{"opt_or_replace", "opt_func_arg_with_default_list", "opt_return_set", "func_return_type", "opt_create_func_opt_list", "create_func_opt_list", "create_func_opt_item", "common_func_opt_item", "routine_return_stmt", "non_reserved_word_or_sconst", "func_arg_with_default_list", "func_arg_with_default", "a_expr", "func_as"},
replace: map[string]string{
"func_as": "'SCONST'",
"non_reserved_word_or_sconst": "'SQL'",
"'DEFAULT'": "",
"'SETOF'": "",
Expand Down
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 a0bfa6d

Please sign in to comment.