From d5bcb0bc8f153b4d3c23a77b2de4fcbf58a6972a Mon Sep 17 00:00:00 2001 From: David Weitzman Date: Mon, 25 Mar 2019 11:54:07 -0700 Subject: [PATCH 1/3] vtgate engine: command line flag to generate sequences for 0 columns This diff introduces -seq-auto-value-on-zero as a vtgate flag. Until now sequences have only populated NULL columns, which is not consistent with the default behavior of mysql. It's consistent with the NO_AUTO_VALUE_ON_ZERO sql mode. In a perfect world we might want to detect per-keyspace sql modes. For now, it seems simple and adequate to let people select at the vtgate level if they want this behavior or not for all keyspace. The motivation for this diff is that we have some situations where people still test code against direct mysql without vtgate. We've twice had folks commit insert statements with 0 for a sequence column that worked in a local test but failed against vtgate. Aligning behavior of vtgate with mysql seems like the cleanest way to prevent that in the future. Signed-off-by: David Weitzman --- go/vt/vtgate/engine/insert.go | 23 +++++++++++- go/vt/vtgate/engine/insert_test.go | 59 ++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/go/vt/vtgate/engine/insert.go b/go/vt/vtgate/engine/insert.go index d1d4a5763a3..6032cc5b7c8 100644 --- a/go/vt/vtgate/engine/insert.go +++ b/go/vt/vtgate/engine/insert.go @@ -18,6 +18,7 @@ package engine import ( "encoding/json" + "flag" "fmt" "strconv" "strings" @@ -39,6 +40,8 @@ import ( vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" ) +var seqAutoValueOnZero = flag.Bool("seq-auto-value-on-zero", false, "For sequences, act as if NO_AUTO_VALUE_ON_ZERO is not set") + var _ Primitive = (*Insert)(nil) // Insert represents the instructions to perform an insert operation. @@ -273,6 +276,22 @@ func (ins *Insert) execInsertSharded(vcursor VCursor, bindVars map[string]*query return result, nil } +func shouldGenerate(v sqltypes.Value) bool { + if v.IsNull() { + return true + } + // Unless the NO_AUTO_VALUE_ON_ZERO sql mode is active in mysql, it also + // treats 0 as a value that should generate a new ID. + if *seqAutoValueOnZero { + n, err := sqltypes.ToUint64(v) + if err == nil && n == 0 { + return true + } + } + + return false +} + // processGenerate generates new values using a sequence if necessary. // If no value was generated, it returns 0. Values are generated only // for cases where none are supplied. @@ -289,7 +308,7 @@ func (ins *Insert) processGenerate(vcursor VCursor, bindVars map[string]*querypb } count := int64(0) for _, val := range resolved { - if val.IsNull() { + if shouldGenerate(val) { count++ } } @@ -319,7 +338,7 @@ func (ins *Insert) processGenerate(vcursor VCursor, bindVars map[string]*querypb // Fill the holes where no value was supplied. cur := insertID for i, v := range resolved { - if v.IsNull() { + if shouldGenerate(v) { bindVars[SeqVarName+strconv.Itoa(i)] = sqltypes.Int64BindVariable(cur) cur++ } else { diff --git a/go/vt/vtgate/engine/insert_test.go b/go/vt/vtgate/engine/insert_test.go index 5923247d0b3..4e5c5ff67ca 100644 --- a/go/vt/vtgate/engine/insert_test.go +++ b/go/vt/vtgate/engine/insert_test.go @@ -83,6 +83,65 @@ func TestInsertUnshardedGenerate(t *testing.T) { {Value: sqltypes.NULL}, {Value: sqltypes.NewInt64(2)}, {Value: sqltypes.NULL}, + {Value: sqltypes.NewInt64(0)}, + }, + }, + } + + vc := &loggingVCursor{ + shards: []string{"0"}, + results: []*sqltypes.Result{ + sqltypes.MakeTestResult( + sqltypes.MakeTestFields( + "nextval", + "int64", + ), + "4", + ), + {InsertID: 1}, + }, + } + result, err := ins.Execute(vc, map[string]*querypb.BindVariable{}, false) + if err != nil { + t.Fatal(err) + } + vc.ExpectLog(t, []string{ + // Fetch two sequence value. + `ResolveDestinations ks2 [] Destinations:DestinationAnyShard()`, + `ExecuteStandalone dummy_generate n: type:INT64 value:"2" ks2 0`, + // Fill those values into the insert. + `ResolveDestinations ks [] Destinations:DestinationAllShards()`, + `ExecuteMultiShard ks.0: dummy_insert {__seq0: type:INT64 value:"1" __seq1: type:INT64 value:"4" __seq2: type:INT64 value:"2" __seq3: type:INT64 value:"5" __seq4: type:INT64 value:"0" } true true`, + }) + + // The insert id returned by ExecuteMultiShard should be overwritten by processGenerate. + expectResult(t, "Execute", result, &sqltypes.Result{InsertID: 4}) +} + +func TestInsertUnshardedGenerate_Zeros(t *testing.T) { + *seqAutoValueOnZero = true + defer func() { *seqAutoValueOnZero = false }() + + ins := NewQueryInsert( + InsertUnsharded, + &vindexes.Keyspace{ + Name: "ks", + Sharded: false, + }, + "dummy_insert", + ) + ins.Generate = &Generate{ + Keyspace: &vindexes.Keyspace{ + Name: "ks2", + Sharded: false, + }, + Query: "dummy_generate", + Values: sqltypes.PlanValue{ + Values: []sqltypes.PlanValue{ + {Value: sqltypes.NewInt64(1)}, + {Value: sqltypes.NewInt64(0)}, + {Value: sqltypes.NewInt64(2)}, + {Value: sqltypes.NewInt64(0)}, {Value: sqltypes.NewInt64(3)}, }, }, From 8328b2230b474f55aaf4a1beee39c2aa7106491f Mon Sep 17 00:00:00 2001 From: David Weitzman Date: Tue, 7 Jul 2020 15:59:19 -0700 Subject: [PATCH 2/3] Fix compilation Signed-off-by: David Weitzman --- go/vt/vtgate/engine/insert.go | 2 +- go/vt/vtgate/engine/insert_test.go | 25 ++++++++++++------------- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/go/vt/vtgate/engine/insert.go b/go/vt/vtgate/engine/insert.go index 6032cc5b7c8..e842a178a93 100644 --- a/go/vt/vtgate/engine/insert.go +++ b/go/vt/vtgate/engine/insert.go @@ -283,7 +283,7 @@ func shouldGenerate(v sqltypes.Value) bool { // Unless the NO_AUTO_VALUE_ON_ZERO sql mode is active in mysql, it also // treats 0 as a value that should generate a new ID. if *seqAutoValueOnZero { - n, err := sqltypes.ToUint64(v) + n, err := evalengine.ToUint64(v) if err == nil && n == 0 { return true } diff --git a/go/vt/vtgate/engine/insert_test.go b/go/vt/vtgate/engine/insert_test.go index 4e5c5ff67ca..06f75c2f98c 100644 --- a/go/vt/vtgate/engine/insert_test.go +++ b/go/vt/vtgate/engine/insert_test.go @@ -83,24 +83,23 @@ func TestInsertUnshardedGenerate(t *testing.T) { {Value: sqltypes.NULL}, {Value: sqltypes.NewInt64(2)}, {Value: sqltypes.NULL}, - {Value: sqltypes.NewInt64(0)}, + {Value: sqltypes.NewInt64(3)}, }, }, } - vc := &loggingVCursor{ - shards: []string{"0"}, - results: []*sqltypes.Result{ - sqltypes.MakeTestResult( - sqltypes.MakeTestFields( - "nextval", - "int64", - ), - "4", + vc := newDMLTestVCursor("0") + vc.results = []*sqltypes.Result{ + sqltypes.MakeTestResult( + sqltypes.MakeTestFields( + "nextval", + "int64", ), - {InsertID: 1}, - }, + "4", + ), + {InsertID: 1}, } + result, err := ins.Execute(vc, map[string]*querypb.BindVariable{}, false) if err != nil { t.Fatal(err) @@ -111,7 +110,7 @@ func TestInsertUnshardedGenerate(t *testing.T) { `ExecuteStandalone dummy_generate n: type:INT64 value:"2" ks2 0`, // Fill those values into the insert. `ResolveDestinations ks [] Destinations:DestinationAllShards()`, - `ExecuteMultiShard ks.0: dummy_insert {__seq0: type:INT64 value:"1" __seq1: type:INT64 value:"4" __seq2: type:INT64 value:"2" __seq3: type:INT64 value:"5" __seq4: type:INT64 value:"0" } true true`, + `ExecuteMultiShard ks.0: dummy_insert {__seq0: type:INT64 value:"1" __seq1: type:INT64 value:"4" __seq2: type:INT64 value:"2" __seq3: type:INT64 value:"5" __seq4: type:INT64 value:"3" } true true`, }) // The insert id returned by ExecuteMultiShard should be overwritten by processGenerate. From 8ac286834a868349ca11414d9e011bcd434a3f15 Mon Sep 17 00:00:00 2001 From: Derrick Laird Date: Sat, 19 Sep 2020 13:38:05 -0600 Subject: [PATCH 3/3] vtgate: generate sequence for 0 values Signed-off-by: Derrick Laird --- go/vt/vtgate/engine/insert.go | 15 ++++++--------- go/vt/vtgate/engine/insert_test.go | 3 --- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/go/vt/vtgate/engine/insert.go b/go/vt/vtgate/engine/insert.go index e842a178a93..a1babe74f13 100644 --- a/go/vt/vtgate/engine/insert.go +++ b/go/vt/vtgate/engine/insert.go @@ -18,7 +18,6 @@ package engine import ( "encoding/json" - "flag" "fmt" "strconv" "strings" @@ -40,8 +39,6 @@ import ( vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" ) -var seqAutoValueOnZero = flag.Bool("seq-auto-value-on-zero", false, "For sequences, act as if NO_AUTO_VALUE_ON_ZERO is not set") - var _ Primitive = (*Insert)(nil) // Insert represents the instructions to perform an insert operation. @@ -276,17 +273,17 @@ func (ins *Insert) execInsertSharded(vcursor VCursor, bindVars map[string]*query return result, nil } +// shouldGenerate determines if a sequence value should be generated for a given value func shouldGenerate(v sqltypes.Value) bool { if v.IsNull() { return true } + // Unless the NO_AUTO_VALUE_ON_ZERO sql mode is active in mysql, it also - // treats 0 as a value that should generate a new ID. - if *seqAutoValueOnZero { - n, err := evalengine.ToUint64(v) - if err == nil && n == 0 { - return true - } + // treats 0 as a value that should generate a new sequence. + n, err := evalengine.ToUint64(v) + if err == nil && n == 0 { + return true } return false diff --git a/go/vt/vtgate/engine/insert_test.go b/go/vt/vtgate/engine/insert_test.go index 06f75c2f98c..ec59c2a70a1 100644 --- a/go/vt/vtgate/engine/insert_test.go +++ b/go/vt/vtgate/engine/insert_test.go @@ -118,9 +118,6 @@ func TestInsertUnshardedGenerate(t *testing.T) { } func TestInsertUnshardedGenerate_Zeros(t *testing.T) { - *seqAutoValueOnZero = true - defer func() { *seqAutoValueOnZero = false }() - ins := NewQueryInsert( InsertUnsharded, &vindexes.Keyspace{