From 187844431b9c70260f75f9388aeadeab7a5590a6 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 13 Apr 2020 19:01:36 +0530 Subject: [PATCH 01/10] set at global scope not allowed Signed-off-by: Harshit Gangal --- go/vt/vtgate/executor_set_test.go | 2 +- go/vt/vtgate/planbuilder/set.go | 24 ++++++++++++++++-------- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/go/vt/vtgate/executor_set_test.go b/go/vt/vtgate/executor_set_test.go index 5dc1423de38..6cf23168966 100644 --- a/go/vt/vtgate/executor_set_test.go +++ b/go/vt/vtgate/executor_set_test.go @@ -117,7 +117,7 @@ func TestExecutorSet(t *testing.T) { err: "unsupported in set: global", }, { in: "set global @@session.client_found_rows = 1", - err: "unsupported in set: mixed using of variable scope", + err: "unsupported in set: global", }, { in: "set client_found_rows = 'aa'", err: "unexpected value type for client_found_rows: string", diff --git a/go/vt/vtgate/planbuilder/set.go b/go/vt/vtgate/planbuilder/set.go index 4722a405fc5..91c91180493 100644 --- a/go/vt/vtgate/planbuilder/set.go +++ b/go/vt/vtgate/planbuilder/set.go @@ -17,27 +17,35 @@ limitations under the License. package planbuilder import ( + vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/sqlparser" + "vitess.io/vitess/go/vt/vterrors" "vitess.io/vitess/go/vt/vtgate/engine" ) func buildSetPlan(sql string, stmt *sqlparser.Set, vschema ContextVSchema) (engine.Primitive, error) { var setOps []engine.SetOp + if stmt.Scope == sqlparser.GlobalStr { + return nil, vterrors.New(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported in set: global") + } + for _, expr := range stmt.Exprs { + var setOp engine.SetOp switch expr.Name.AtCount() { case sqlparser.SingleAt: + pv, err := sqlparser.NewPlanValue(expr.Expr) + if err != nil { + return nil, err + } + setOp = &engine.UserDefinedVariable{ + Name: expr.Name.Lowered(), + PlanValue: pv, + } default: return nil, ErrPlanNotSupported } - pv, err := sqlparser.NewPlanValue(expr.Expr) - if err != nil { - return nil, err - } - setOps = append(setOps, &engine.UserDefinedVariable{ - Name: expr.Name.Lowered(), - PlanValue: pv, - }) + setOps = append(setOps, setOp) } return &engine.Set{ Ops: setOps, From 042bc4a17e528613b56e643c8bc9bbba99ce90ff Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 13 Apr 2020 19:58:21 +0530 Subject: [PATCH 02/10] set system variable ignore operation Signed-off-by: Harshit Gangal --- go/vt/vtgate/engine/set.go | 24 +++++++++++++++ go/vt/vtgate/planbuilder/plan_test.go | 1 + go/vt/vtgate/planbuilder/set.go | 29 ++++++++++++++++++- .../planbuilder/testdata/set_sysvar_cases.txt | 20 +++++++++++++ 4 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt diff --git a/go/vt/vtgate/engine/set.go b/go/vt/vtgate/engine/set.go index f4e2b025602..0191cfa6d70 100644 --- a/go/vt/vtgate/engine/set.go +++ b/go/vt/vtgate/engine/set.go @@ -18,6 +18,7 @@ package engine import ( "vitess.io/vitess/go/sqltypes" + "vitess.io/vitess/go/vt/log" querypb "vitess.io/vitess/go/vt/proto/query" ) @@ -40,6 +41,12 @@ type ( Name string PlanValue sqltypes.PlanValue } + + // SysVarIgnore implements the SetOp interface to execute changes to system locally. + SysVarIgnore struct { + Name string + PlanValue sqltypes.PlanValue + } ) var _ Primitive = (*Set)(nil) @@ -106,3 +113,20 @@ func (u *UserDefinedVariable) Execute(vcursor VCursor, bindVars map[string]*quer } return vcursor.SetUDV(u.Name, value) } + +var _ SetOp = (*SysVarIgnore)(nil) + +//VariableName implements the SetOp interface method. +func (svi *SysVarIgnore) VariableName() string { + return svi.Name +} + +//Execute implements the SetOp interface method. +func (svi *SysVarIgnore) Execute(vcursor VCursor, bindVars map[string]*querypb.BindVariable) error { + value, err := svi.PlanValue.ResolveValue(bindVars) + if err != nil { + return err + } + log.Warningf("Ignored inapplicable SET %v = %v", svi.Name, value.String()) + return nil +} diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index 509e14b37f9..27f71054096 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -170,6 +170,7 @@ func TestPlan(t *testing.T) { testFile(t, "memory_sort_cases.txt", testOutputTempDir, vschemaWrapper) testFile(t, "use_cases.txt", testOutputTempDir, vschemaWrapper) testFile(t, "set_cases.txt", testOutputTempDir, vschemaWrapper) + testFile(t, "set_sysvar_cases.txt", testOutputTempDir, vschemaWrapper) } func TestOne(t *testing.T) { diff --git a/go/vt/vtgate/planbuilder/set.go b/go/vt/vtgate/planbuilder/set.go index 91c91180493..29f4e1ee2d7 100644 --- a/go/vt/vtgate/planbuilder/set.go +++ b/go/vt/vtgate/planbuilder/set.go @@ -25,14 +25,24 @@ import ( func buildSetPlan(sql string, stmt *sqlparser.Set, vschema ContextVSchema) (engine.Primitive, error) { var setOps []engine.SetOp + var setOp engine.SetOp + var err error if stmt.Scope == sqlparser.GlobalStr { return nil, vterrors.New(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported in set: global") } for _, expr := range stmt.Exprs { - var setOp engine.SetOp switch expr.Name.AtCount() { + case sqlparser.NoAt: + planFunc, ok := sysVarPlanningFunc[expr.Name.Lowered()] + if !ok { + return nil, ErrPlanNotSupported + } + setOp, err = planFunc(expr) + if err != nil { + return nil, err + } case sqlparser.SingleAt: pv, err := sqlparser.NewPlanValue(expr.Expr) if err != nil { @@ -51,3 +61,20 @@ func buildSetPlan(sql string, stmt *sqlparser.Set, vschema ContextVSchema) (engi Ops: setOps, }, nil } + +var sysVarPlanningFunc = map[string]func(expr *sqlparser.SetExpr) (*engine.SysVarIgnore, error){} + +func init() { + sysVarPlanningFunc["debug"] = buildSetOpIgnore +} + +func buildSetOpIgnore(expr *sqlparser.SetExpr) (*engine.SysVarIgnore, error) { + pv, err := sqlparser.NewPlanValue(expr.Expr) + if err != nil { + return nil, err + } + return &engine.SysVarIgnore{ + Name: expr.Name.Lowered(), + PlanValue: pv, + }, nil +} diff --git a/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt b/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt new file mode 100644 index 00000000000..51194f0f309 --- /dev/null +++ b/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt @@ -0,0 +1,20 @@ +# set ignore plan +"set debug = '+P'" +{ + "QueryType": "SET", + "Original": "set debug = '+P'", + "Instructions": { + "OperatorType": "Set", + "Variant": "", + "Ops": [ + { + "Name": "debug", + "PlanValue": "+P" + } + ] + } +} + +# set plan building not supported +"set innodb_strict_mode = OFF" +"plan building not supported" From 368ef4622e29dfafae13db4e3388187815f02d00 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 13 Apr 2020 19:58:21 +0530 Subject: [PATCH 03/10] set system variable check and ignore operation Signed-off-by: Harshit Gangal --- go/vt/vtgate/engine/set.go | 58 ++++++++++++++++++++++++++++- go/vt/vtgate/planbuilder/set.go | 65 +++++++++++++++++++++++++-------- 2 files changed, 106 insertions(+), 17 deletions(-) diff --git a/go/vt/vtgate/engine/set.go b/go/vt/vtgate/engine/set.go index 0191cfa6d70..e9fa482b60b 100644 --- a/go/vt/vtgate/engine/set.go +++ b/go/vt/vtgate/engine/set.go @@ -18,8 +18,12 @@ package engine import ( "vitess.io/vitess/go/sqltypes" + "vitess.io/vitess/go/vt/key" "vitess.io/vitess/go/vt/log" querypb "vitess.io/vitess/go/vt/proto/query" + vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" + "vitess.io/vitess/go/vt/vterrors" + "vitess.io/vitess/go/vt/vtgate/vindexes" ) type ( @@ -42,11 +46,22 @@ type ( PlanValue sqltypes.PlanValue } - // SysVarIgnore implements the SetOp interface to execute changes to system locally. + // SysVarIgnore implements the SetOp interface to ignore the settings. SysVarIgnore struct { Name string PlanValue sqltypes.PlanValue } + + // SysVarCheckAndIgnore implements the SetOp interface to check underlying setting and ignore if same. + SysVarCheckAndIgnore struct { + Name string + Keyspace *vindexes.Keyspace + TargetDestination key.Destination + CurrentSysVarQuery string + ResolveExpr bool + PlanValue sqltypes.PlanValue + NewSysVarQuery string + } ) var _ Primitive = (*Set)(nil) @@ -130,3 +145,44 @@ func (svi *SysVarIgnore) Execute(vcursor VCursor, bindVars map[string]*querypb.B log.Warningf("Ignored inapplicable SET %v = %v", svi.Name, value.String()) return nil } + +var _ SetOp = (*SysVarCheckAndIgnore)(nil) + +//VariableName implements the SetOp interface method +func (svci SysVarCheckAndIgnore) VariableName() string { + return svci.Name +} + +//Execute implements the SetOp interface method +func (svci SysVarCheckAndIgnore) Execute(vcursor VCursor, bindVars map[string]*querypb.BindVariable) error { + var err error + var result *sqltypes.Result + var cValue, nValue sqltypes.Value + rss, _, err := vcursor.ResolveDestinations(svci.Keyspace.Name, nil, []key.Destination{svci.TargetDestination}) + if err != nil { + return vterrors.Wrap(err, "SysVarCheckAndIgnore") + } + + if len(rss) != 1 { + return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Unexpected error, DestinationKeyspaceID mapping to multiple shards: %v", svci.TargetDestination) + } + if svci.ResolveExpr { + nValue, err = svci.PlanValue.ResolveValue(bindVars) + } else { + result, err = execShard(vcursor, svci.NewSysVarQuery, bindVars, rss[0], false /* rollbackOnError */, false /* canAutocommit */) + nValue = result.Rows[0][0] + } + if err != nil { + return err + } + result, err = execShard(vcursor, svci.CurrentSysVarQuery, bindVars, rss[0], false /* rollbackOnError */, false /* canAutocommit */) + if err != nil { + return err + } + cValue = result.Rows[0][0] + if cValue.String() != nValue.String() { + return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Modification not allowed using set construct for: %s", svci.Name) + } + + return nil +} diff --git a/go/vt/vtgate/planbuilder/set.go b/go/vt/vtgate/planbuilder/set.go index 29f4e1ee2d7..18cf07da617 100644 --- a/go/vt/vtgate/planbuilder/set.go +++ b/go/vt/vtgate/planbuilder/set.go @@ -17,12 +17,22 @@ limitations under the License. package planbuilder import ( + "fmt" + + "vitess.io/vitess/go/vt/key" vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/vterrors" "vitess.io/vitess/go/vt/vtgate/engine" ) +var sysVarPlanningFunc = map[string]func(expr *sqlparser.SetExpr, vschema ContextVSchema) (engine.SetOp, error){} + +func init() { + sysVarPlanningFunc["debug"] = buildSetOpIgnore + sysVarPlanningFunc["sql_mode"] = buildSetOpCheckAndIgnore +} + func buildSetPlan(sql string, stmt *sqlparser.Set, vschema ContextVSchema) (engine.Primitive, error) { var setOps []engine.SetOp var setOp engine.SetOp @@ -34,15 +44,6 @@ func buildSetPlan(sql string, stmt *sqlparser.Set, vschema ContextVSchema) (engi for _, expr := range stmt.Exprs { switch expr.Name.AtCount() { - case sqlparser.NoAt: - planFunc, ok := sysVarPlanningFunc[expr.Name.Lowered()] - if !ok { - return nil, ErrPlanNotSupported - } - setOp, err = planFunc(expr) - if err != nil { - return nil, err - } case sqlparser.SingleAt: pv, err := sqlparser.NewPlanValue(expr.Expr) if err != nil { @@ -52,6 +53,15 @@ func buildSetPlan(sql string, stmt *sqlparser.Set, vschema ContextVSchema) (engi Name: expr.Name.Lowered(), PlanValue: pv, } + case sqlparser.DoubleAt: + planFunc, ok := sysVarPlanningFunc[expr.Name.Lowered()] + if !ok { + return nil, ErrPlanNotSupported + } + setOp, err = planFunc(expr, vschema) + if err != nil { + return nil, err + } default: return nil, ErrPlanNotSupported } @@ -62,13 +72,7 @@ func buildSetPlan(sql string, stmt *sqlparser.Set, vschema ContextVSchema) (engi }, nil } -var sysVarPlanningFunc = map[string]func(expr *sqlparser.SetExpr) (*engine.SysVarIgnore, error){} - -func init() { - sysVarPlanningFunc["debug"] = buildSetOpIgnore -} - -func buildSetOpIgnore(expr *sqlparser.SetExpr) (*engine.SysVarIgnore, error) { +func buildSetOpIgnore(expr *sqlparser.SetExpr, _ ContextVSchema) (engine.SetOp, error) { pv, err := sqlparser.NewPlanValue(expr.Expr) if err != nil { return nil, err @@ -78,3 +82,32 @@ func buildSetOpIgnore(expr *sqlparser.SetExpr) (*engine.SysVarIgnore, error) { PlanValue: pv, }, nil } + +func buildSetOpCheckAndIgnore(expr *sqlparser.SetExpr, vschema ContextVSchema) (engine.SetOp, error) { + resolveExpr := true + pv, err := sqlparser.NewPlanValue(expr.Expr) + if err != nil { + resolveExpr = false + } + keyspace, err := vschema.DefaultKeyspace() + if err != nil { + return nil, err + } + + dest := vschema.Destination() + if dest == nil { + dest = key.DestinationAnyShard{} + } + buf := sqlparser.NewTrackedBuffer(nil) + buf.Myprintf("%v", expr.Expr) + + return &engine.SysVarCheckAndIgnore{ + Name: expr.Name.Lowered(), + Keyspace: keyspace, + TargetDestination: dest, + CurrentSysVarQuery: fmt.Sprintf("select @@%s from dual", expr.Name.Lowered()), + ResolveExpr: resolveExpr, + PlanValue: pv, + NewSysVarQuery: fmt.Sprintf("select %s from dual", buf.String()), + }, nil +} From 32bafe025a5dbea6e72a8a42f8da506043d79f87 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Tue, 14 Apr 2020 21:27:37 +0530 Subject: [PATCH 04/10] added planbuilder test for sysvar check and ignore op Signed-off-by: Harshit Gangal --- .../planbuilder/testdata/set_sysvar_cases.txt | 29 +++++++++++++++++-- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt b/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt index 51194f0f309..eb427f746f6 100644 --- a/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt @@ -1,8 +1,8 @@ # set ignore plan -"set debug = '+P'" +"set @@debug = '+P'" { "QueryType": "SET", - "Original": "set debug = '+P'", + "Original": "set @@debug = '+P'", "Instructions": { "OperatorType": "Set", "Variant": "", @@ -15,6 +15,29 @@ } } +# set check and ignore plan +"set @@sql_mode = concat(@@sql_mode, ',NO_AUTO_CREATE_USER')" +{ + "QueryType": "SET", + "Original": "set @@sql_mode = concat(@@sql_mode, ',NO_AUTO_CREATE_USER')", + "Instructions": { + "OperatorType": "Set", + "Variant": "", + "Ops": [ + { + "Name": "sql_mode", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "TargetDestination": null, + "CurrentSysVarQuery": "select @@sql_mode from dual", + "NewSysVarQuery": "select concat(@@sql_mode, ',NO_AUTO_CREATE_USER') from dual" + } + ] + } +} + # set plan building not supported -"set innodb_strict_mode = OFF" +"set @@innodb_strict_mode = OFF" "plan building not supported" From fb3d3fc0f8747d5a450d91cb699125e97e182032 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Tue, 14 Apr 2020 21:28:21 +0530 Subject: [PATCH 05/10] added end to end test for set sysvar Signed-off-by: Harshit Gangal --- .../endtoend/vtgate/setstatement/main_test.go | 85 ++++++++++++++++++ .../vtgate/setstatement/sysvar_test.go | 64 ++++++++++++++ .../endtoend/vtgate/setstatement/udv_test.go | 88 +------------------ 3 files changed, 153 insertions(+), 84 deletions(-) create mode 100644 go/test/endtoend/vtgate/setstatement/main_test.go create mode 100644 go/test/endtoend/vtgate/setstatement/sysvar_test.go diff --git a/go/test/endtoend/vtgate/setstatement/main_test.go b/go/test/endtoend/vtgate/setstatement/main_test.go new file mode 100644 index 00000000000..2c6208236fe --- /dev/null +++ b/go/test/endtoend/vtgate/setstatement/main_test.go @@ -0,0 +1,85 @@ +package setstatement + +import ( + "flag" + "os" + "testing" + + "vitess.io/vitess/go/mysql" + "vitess.io/vitess/go/sqltypes" + "vitess.io/vitess/go/test/endtoend/cluster" +) + +var ( + clusterInstance *cluster.LocalProcessCluster + keyspaceName = "ks" + cell = "zone1" + hostname = "localhost" + sqlSchema = ` + create table test( + id bigint, + val1 varchar(16), + val2 int, + val3 float, + primary key(id) + )Engine=InnoDB;` + + vSchema = ` + { + "sharded":true, + "vindexes": { + "hash_index": { + "type": "hash" + } + }, + "tables": { + "test":{ + "column_vindexes": [ + { + "column": "id", + "name": "hash_index" + } + ] + } + } + } + ` +) + +func TestMain(m *testing.M) { + defer cluster.PanicHandler(nil) + flag.Parse() + + exitCode := func() int { + clusterInstance = cluster.NewCluster(cell, hostname) + defer clusterInstance.Teardown() + + // Start topo server + if err := clusterInstance.StartTopo(); err != nil { + return 1 + } + + // Start keyspace + keyspace := &cluster.Keyspace{ + Name: keyspaceName, + SchemaSQL: sqlSchema, + VSchema: vSchema, + } + if err := clusterInstance.StartKeyspace(*keyspace, []string{"-80", "80-"}, 1, false); err != nil { + return 1 + } + + // Start vtgate + if err := clusterInstance.StartVtgate(); err != nil { + return 1 + } + + return m.Run() + }() + os.Exit(exitCode) +} + +func exec(t *testing.T, conn *mysql.Conn, query string) (*sqltypes.Result, error) { + t.Helper() + return conn.ExecuteFetch(query, 1000, true) +} diff --git a/go/test/endtoend/vtgate/setstatement/sysvar_test.go b/go/test/endtoend/vtgate/setstatement/sysvar_test.go new file mode 100644 index 00000000000..77f6a4578a3 --- /dev/null +++ b/go/test/endtoend/vtgate/setstatement/sysvar_test.go @@ -0,0 +1,64 @@ +package setstatement + +import ( + "context" + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" + + "vitess.io/vitess/go/mysql" + "vitess.io/vitess/go/test/endtoend/cluster" +) + +func TestSetSysVar(t *testing.T) { + defer cluster.PanicHandler(t) + ctx := context.Background() + vtParams := mysql.ConnParams{ + Host: "localhost", + Port: clusterInstance.VtgateMySQLPort, + } + type queriesWithExpectations struct { + query string + expectedRows string + rowsAffected int + errMsg string + } + + queries := []queriesWithExpectations{{ + query: `set @@debug = 'T'`, + expectedRows: ``, rowsAffected: 0, + }, { + query: `set @@sql_mode = @@sql_mode`, + expectedRows: ``, rowsAffected: 0, + }, { + query: `set @@sql_mode = concat(@@sql_mode,"")`, + expectedRows: ``, rowsAffected: 0, + }, { + query: `set @@sql_mode = concat(@@sql_mode,"ALLOW_INVALID_DATES")`, + errMsg: "Modification not allowed using set construct for: sql_mode", + }} + + conn, err := mysql.Connect(ctx, &vtParams) + require.NoError(t, err) + defer conn.Close() + + for i, q := range queries { + t.Run(fmt.Sprintf("%d-%s", i, q.query), func(t *testing.T) { + qr, err := exec(t, conn, q.query) + if q.errMsg != "" { + require.Contains(t, err.Error(), q.errMsg) + } else { + require.Nil(t, err) + require.Equal(t, uint64(q.rowsAffected), qr.RowsAffected, "rows affected wrong for query: %s", q.query) + if q.expectedRows != "" { + result := fmt.Sprintf("%v", qr.Rows) + if diff := cmp.Diff(q.expectedRows, result); diff != "" { + t.Errorf("%s\nfor query: %s", diff, q.query) + } + } + } + }) + } +} diff --git a/go/test/endtoend/vtgate/setstatement/udv_test.go b/go/test/endtoend/vtgate/setstatement/udv_test.go index 86baee4adce..f7e15ebb690 100644 --- a/go/test/endtoend/vtgate/setstatement/udv_test.go +++ b/go/test/endtoend/vtgate/setstatement/udv_test.go @@ -2,98 +2,17 @@ package setstatement import ( "context" - "flag" "fmt" - "os" "testing" "github.com/google/go-cmp/cmp" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "vitess.io/vitess/go/mysql" - "vitess.io/vitess/go/sqltypes" "vitess.io/vitess/go/test/endtoend/cluster" ) -var ( - clusterInstance *cluster.LocalProcessCluster - keyspaceName = "ks" - cell = "zone1" - hostname = "localhost" - sqlSchema = ` - create table test( - id bigint, - val1 varchar(16), - val2 int, - val3 float, - primary key(id) - )Engine=InnoDB;` - - vSchema = ` - { - "sharded":true, - "vindexes": { - "hash_index": { - "type": "hash" - } - }, - "tables": { - "test":{ - "column_vindexes": [ - { - "column": "id", - "name": "hash_index" - } - ] - } - } - } - ` -) - -func TestMain(m *testing.M) { - defer cluster.PanicHandler(nil) - flag.Parse() - - exitCode := func() int { - clusterInstance = cluster.NewCluster(cell, hostname) - defer clusterInstance.Teardown() - - // Start topo server - if err := clusterInstance.StartTopo(); err != nil { - return 1 - } - - // Start keyspace - keyspace := &cluster.Keyspace{ - Name: keyspaceName, - SchemaSQL: sqlSchema, - VSchema: vSchema, - } - if err := clusterInstance.StartKeyspace(*keyspace, []string{"-80", "80-"}, 1, false); err != nil { - return 1 - } - - // Start vtgate - if err := clusterInstance.StartVtgate(); err != nil { - return 1 - } - - return m.Run() - }() - os.Exit(exitCode) -} - -func exec(t *testing.T, conn *mysql.Conn, query string) *sqltypes.Result { - t.Helper() - qr, err := conn.ExecuteFetch(query, 1000, true) - require.Nil(t, err) - return qr -} - -func TestSet(t *testing.T) { +func TestSetUDV(t *testing.T) { defer cluster.PanicHandler(t) ctx := context.Background() vtParams := mysql.ConnParams{ @@ -150,8 +69,9 @@ func TestSet(t *testing.T) { for i, q := range queries { t.Run(fmt.Sprintf("%d-%s", i, q.query), func(t *testing.T) { - qr := exec(t, conn, q.query) - assert.Equal(t, uint64(q.rowsAffected), qr.RowsAffected, "rows affected wrong for query: %s", q.query) + qr, err := exec(t, conn, q.query) + require.Nil(t, err) + require.Equal(t, uint64(q.rowsAffected), qr.RowsAffected, "rows affected wrong for query: %s", q.query) if q.expectedRows != "" { result := fmt.Sprintf("%v", qr.Rows) if diff := cmp.Diff(q.expectedRows, result); diff != "" { From 2795069ec541356ac927151331fa4e2227717d19 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 15 Apr 2020 12:20:22 +0530 Subject: [PATCH 06/10] added set op type for plan description Signed-off-by: Harshit Gangal --- go/vt/vtgate/engine/set.go | 42 ++++++++++++++++++- go/vt/vtgate/planbuilder/set.go | 2 +- .../vtgate/planbuilder/testdata/set_cases.txt | 3 ++ .../planbuilder/testdata/set_sysvar_cases.txt | 14 ++++--- .../testdata/unsupported_cases.txt | 2 +- 5 files changed, 54 insertions(+), 9 deletions(-) diff --git a/go/vt/vtgate/engine/set.go b/go/vt/vtgate/engine/set.go index e9fa482b60b..9bf9c212d9a 100644 --- a/go/vt/vtgate/engine/set.go +++ b/go/vt/vtgate/engine/set.go @@ -17,6 +17,8 @@ limitations under the License. package engine import ( + "encoding/json" + "vitess.io/vitess/go/sqltypes" "vitess.io/vitess/go/vt/key" "vitess.io/vitess/go/vt/log" @@ -115,6 +117,18 @@ func (s *Set) description() PrimitiveDescription { var _ SetOp = (*UserDefinedVariable)(nil) +//MarshalJSON provides the type to SetOp for plan json +func (u *UserDefinedVariable) MarshalJSON() ([]byte, error) { + return json.Marshal(struct { + Type string + UserDefinedVariable + }{ + Type: "UserDefinedVariable", + UserDefinedVariable: *u, + }) + +} + //VariableName implements the SetOp interface method. func (u *UserDefinedVariable) VariableName() string { return u.Name @@ -131,6 +145,18 @@ func (u *UserDefinedVariable) Execute(vcursor VCursor, bindVars map[string]*quer var _ SetOp = (*SysVarIgnore)(nil) +//MarshalJSON provides the type to SetOp for plan json +func (svi *SysVarIgnore) MarshalJSON() ([]byte, error) { + return json.Marshal(struct { + Type string + SysVarIgnore + }{ + Type: "SysVarIgnore", + SysVarIgnore: *svi, + }) + +} + //VariableName implements the SetOp interface method. func (svi *SysVarIgnore) VariableName() string { return svi.Name @@ -148,13 +174,25 @@ func (svi *SysVarIgnore) Execute(vcursor VCursor, bindVars map[string]*querypb.B var _ SetOp = (*SysVarCheckAndIgnore)(nil) +//MarshalJSON provides the type to SetOp for plan json +func (svci *SysVarCheckAndIgnore) MarshalJSON() ([]byte, error) { + return json.Marshal(struct { + Type string + SysVarCheckAndIgnore + }{ + Type: "SysVarCheckAndIgnore", + SysVarCheckAndIgnore: *svci, + }) + +} + //VariableName implements the SetOp interface method -func (svci SysVarCheckAndIgnore) VariableName() string { +func (svci *SysVarCheckAndIgnore) VariableName() string { return svci.Name } //Execute implements the SetOp interface method -func (svci SysVarCheckAndIgnore) Execute(vcursor VCursor, bindVars map[string]*querypb.BindVariable) error { +func (svci *SysVarCheckAndIgnore) Execute(vcursor VCursor, bindVars map[string]*querypb.BindVariable) error { var err error var result *sqltypes.Result var cValue, nValue sqltypes.Value diff --git a/go/vt/vtgate/planbuilder/set.go b/go/vt/vtgate/planbuilder/set.go index 18cf07da617..08303bbb380 100644 --- a/go/vt/vtgate/planbuilder/set.go +++ b/go/vt/vtgate/planbuilder/set.go @@ -29,7 +29,7 @@ import ( var sysVarPlanningFunc = map[string]func(expr *sqlparser.SetExpr, vschema ContextVSchema) (engine.SetOp, error){} func init() { - sysVarPlanningFunc["debug"] = buildSetOpIgnore + sysVarPlanningFunc["default_storage_engine"] = buildSetOpIgnore sysVarPlanningFunc["sql_mode"] = buildSetOpCheckAndIgnore } diff --git a/go/vt/vtgate/planbuilder/testdata/set_cases.txt b/go/vt/vtgate/planbuilder/testdata/set_cases.txt index 99d4f72e920..b790194c7ca 100644 --- a/go/vt/vtgate/planbuilder/testdata/set_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/set_cases.txt @@ -8,6 +8,7 @@ "Variant": "", "Ops": [ { + "Type": "UserDefinedVariable", "Name": "foo", "PlanValue": 42 } @@ -25,10 +26,12 @@ "Variant": "", "Ops": [ { + "Type": "UserDefinedVariable", "Name": "foo", "PlanValue": 42 }, { + "Type": "UserDefinedVariable", "Name": "bar", "PlanValue": ":__vtudvfoo" } diff --git a/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt b/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt index eb427f746f6..f9bba7e1368 100644 --- a/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt @@ -1,15 +1,16 @@ # set ignore plan -"set @@debug = '+P'" +"set @@default_storage_engine = 'DONOTCHANGEME'" { "QueryType": "SET", - "Original": "set @@debug = '+P'", + "Original": "set @@default_storage_engine = 'DONOTCHANGEME'", "Instructions": { "OperatorType": "Set", "Variant": "", "Ops": [ { - "Name": "debug", - "PlanValue": "+P" + "Type": "SysVarIgnore", + "Name": "default_storage_engine", + "PlanValue": "DONOTCHANGEME" } ] } @@ -25,13 +26,16 @@ "Variant": "", "Ops": [ { + "Type": "SysVarCheckAndIgnore", "Name": "sql_mode", "Keyspace": { "Name": "main", "Sharded": false }, - "TargetDestination": null, + "TargetDestination": {}, "CurrentSysVarQuery": "select @@sql_mode from dual", + "ResolveExpr": false, + "PlanValue": null, "NewSysVarQuery": "select concat(@@sql_mode, ',NO_AUTO_CREATE_USER') from dual" } ] diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt index b9173fb4094..d2bc6dac7dc 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt @@ -11,7 +11,7 @@ "expression is too complex ':__vtudvfoo + 1'" # set user defined and system variable -"set @foo = 42, @bar = @foo, @@sql_mode = @@sql_mode" +"set @foo = 42, @bar = @foo, @@wait_timeout = 28800" "plan building not supported" # SHOW From 556a494fcec9ea3e0969c51c7a6c87cd41044b15 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 15 Apr 2020 12:47:54 +0530 Subject: [PATCH 07/10] Modified SysVarCheckAndIgnore to send query to vttablet for equality check Signed-off-by: Harshit Gangal --- go/vt/vtgate/engine/set.go | 29 ++++--------------- go/vt/vtgate/planbuilder/set.go | 17 ++++------- .../planbuilder/testdata/set_sysvar_cases.txt | 5 +--- 3 files changed, 12 insertions(+), 39 deletions(-) diff --git a/go/vt/vtgate/engine/set.go b/go/vt/vtgate/engine/set.go index 9bf9c212d9a..7408f49698d 100644 --- a/go/vt/vtgate/engine/set.go +++ b/go/vt/vtgate/engine/set.go @@ -56,13 +56,10 @@ type ( // SysVarCheckAndIgnore implements the SetOp interface to check underlying setting and ignore if same. SysVarCheckAndIgnore struct { - Name string - Keyspace *vindexes.Keyspace - TargetDestination key.Destination - CurrentSysVarQuery string - ResolveExpr bool - PlanValue sqltypes.PlanValue - NewSysVarQuery string + Name string + Keyspace *vindexes.Keyspace + TargetDestination key.Destination + CheckSysVarQuery string } ) @@ -193,9 +190,6 @@ func (svci *SysVarCheckAndIgnore) VariableName() string { //Execute implements the SetOp interface method func (svci *SysVarCheckAndIgnore) Execute(vcursor VCursor, bindVars map[string]*querypb.BindVariable) error { - var err error - var result *sqltypes.Result - var cValue, nValue sqltypes.Value rss, _, err := vcursor.ResolveDestinations(svci.Keyspace.Name, nil, []key.Destination{svci.TargetDestination}) if err != nil { return vterrors.Wrap(err, "SysVarCheckAndIgnore") @@ -204,23 +198,12 @@ func (svci *SysVarCheckAndIgnore) Execute(vcursor VCursor, bindVars map[string]* if len(rss) != 1 { return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Unexpected error, DestinationKeyspaceID mapping to multiple shards: %v", svci.TargetDestination) } - if svci.ResolveExpr { - nValue, err = svci.PlanValue.ResolveValue(bindVars) - } else { - result, err = execShard(vcursor, svci.NewSysVarQuery, bindVars, rss[0], false /* rollbackOnError */, false /* canAutocommit */) - nValue = result.Rows[0][0] - } + result, err := execShard(vcursor, svci.CheckSysVarQuery, bindVars, rss[0], false /* rollbackOnError */, false /* canAutocommit */) if err != nil { return err } - result, err = execShard(vcursor, svci.CurrentSysVarQuery, bindVars, rss[0], false /* rollbackOnError */, false /* canAutocommit */) - if err != nil { - return err - } - cValue = result.Rows[0][0] - if cValue.String() != nValue.String() { + if result.RowsAffected != 1 { return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Modification not allowed using set construct for: %s", svci.Name) } - return nil } diff --git a/go/vt/vtgate/planbuilder/set.go b/go/vt/vtgate/planbuilder/set.go index 08303bbb380..91290a69ab6 100644 --- a/go/vt/vtgate/planbuilder/set.go +++ b/go/vt/vtgate/planbuilder/set.go @@ -84,11 +84,6 @@ func buildSetOpIgnore(expr *sqlparser.SetExpr, _ ContextVSchema) (engine.SetOp, } func buildSetOpCheckAndIgnore(expr *sqlparser.SetExpr, vschema ContextVSchema) (engine.SetOp, error) { - resolveExpr := true - pv, err := sqlparser.NewPlanValue(expr.Expr) - if err != nil { - resolveExpr = false - } keyspace, err := vschema.DefaultKeyspace() if err != nil { return nil, err @@ -98,16 +93,14 @@ func buildSetOpCheckAndIgnore(expr *sqlparser.SetExpr, vschema ContextVSchema) ( if dest == nil { dest = key.DestinationAnyShard{} } + buf := sqlparser.NewTrackedBuffer(nil) buf.Myprintf("%v", expr.Expr) return &engine.SysVarCheckAndIgnore{ - Name: expr.Name.Lowered(), - Keyspace: keyspace, - TargetDestination: dest, - CurrentSysVarQuery: fmt.Sprintf("select @@%s from dual", expr.Name.Lowered()), - ResolveExpr: resolveExpr, - PlanValue: pv, - NewSysVarQuery: fmt.Sprintf("select %s from dual", buf.String()), + Name: expr.Name.Lowered(), + Keyspace: keyspace, + TargetDestination: dest, + CheckSysVarQuery: fmt.Sprintf("select 1 from dual where @@%s = %s", expr.Name.Lowered(), buf.String()), }, nil } diff --git a/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt b/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt index f9bba7e1368..fa0bced7e0f 100644 --- a/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt @@ -33,10 +33,7 @@ "Sharded": false }, "TargetDestination": {}, - "CurrentSysVarQuery": "select @@sql_mode from dual", - "ResolveExpr": false, - "PlanValue": null, - "NewSysVarQuery": "select concat(@@sql_mode, ',NO_AUTO_CREATE_USER') from dual" + "CheckSysVarQuery": "select 1 from dual where @@sql_mode = concat(@@sql_mode, ',NO_AUTO_CREATE_USER')" } ] } From d0618ed0d98117ccf1552a355439dafbb12581f5 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Wed, 15 Apr 2020 10:49:56 +0200 Subject: [PATCH 08/10] Minor refactoring of VCursor Signed-off-by: Andres Taylor --- go/vt/vtgate/engine/fake_vcursor_test.go | 8 +- go/vt/vtgate/engine/primitive.go | 160 ++++++++++++----------- go/vt/vtgate/engine/route.go | 2 +- go/vt/vtgate/engine/set.go | 2 +- go/vt/vtgate/engine/update_target.go | 2 +- go/vt/vtgate/vcursor_impl.go | 4 + 6 files changed, 99 insertions(+), 79 deletions(-) diff --git a/go/vt/vtgate/engine/fake_vcursor_test.go b/go/vt/vtgate/engine/fake_vcursor_test.go index 2b7acc6d809..e9837e9f507 100644 --- a/go/vt/vtgate/engine/fake_vcursor_test.go +++ b/go/vt/vtgate/engine/fake_vcursor_test.go @@ -54,7 +54,9 @@ func (t noopVCursor) SetUDV(key string, value interface{}) error { func (t noopVCursor) ExecuteVSchema(keyspace string, vschemaDDL *sqlparser.DDL) error { panic("implement me") } - +func (t noopVCursor) Session() SessionActions { + return t +} func (t noopVCursor) SetTarget(target string) error { panic("implement me") } @@ -136,6 +138,10 @@ func (f *loggingVCursor) ExecuteVSchema(keyspace string, vschemaDDL *sqlparser.D panic("implement me") } +func (f *loggingVCursor) Session() SessionActions { + return f +} + func (f *loggingVCursor) SetTarget(target string) error { f.log = append(f.log, fmt.Sprintf("Target set to %s", target)) return nil diff --git a/go/vt/vtgate/engine/primitive.go b/go/vt/vtgate/engine/primitive.go index 3cecd6dd5a4..72670d2d7ad 100644 --- a/go/vt/vtgate/engine/primitive.go +++ b/go/vt/vtgate/engine/primitive.go @@ -42,61 +42,101 @@ const ( ListVarName = "__vals" ) -// VCursor defines the interface the engine will use -// to execute routes. -type VCursor interface { - // Context returns the context of the current request. - Context() context.Context +type ( + // VCursor defines the interface the engine will use + // to execute routes. + VCursor interface { + // Context returns the context of the current request. + Context() context.Context - // MaxMemoryRows returns the maxMemoryRows flag value. - MaxMemoryRows() int + // MaxMemoryRows returns the maxMemoryRows flag value. + MaxMemoryRows() int - // SetContextTimeout updates the context and sets a timeout. - SetContextTimeout(timeout time.Duration) context.CancelFunc + // SetContextTimeout updates the context and sets a timeout. + SetContextTimeout(timeout time.Duration) context.CancelFunc - // RecordWarning stores the given warning in the current session - RecordWarning(warning *querypb.QueryWarning) + // V3 functions. + Execute(method string, query string, bindvars map[string]*querypb.BindVariable, rollbackOnError bool, co vtgatepb.CommitOrder) (*sqltypes.Result, error) + AutocommitApproval() bool - // V3 functions. - Execute(method string, query string, bindvars map[string]*querypb.BindVariable, rollbackOnError bool, co vtgatepb.CommitOrder) (*sqltypes.Result, error) - AutocommitApproval() bool + // Shard-level functions. + ExecuteMultiShard(rss []*srvtopo.ResolvedShard, queries []*querypb.BoundQuery, rollbackOnError, canAutocommit bool) (*sqltypes.Result, []error) + ExecuteStandalone(query string, bindvars map[string]*querypb.BindVariable, rs *srvtopo.ResolvedShard) (*sqltypes.Result, error) + StreamExecuteMulti(query string, rss []*srvtopo.ResolvedShard, bindVars []map[string]*querypb.BindVariable, callback func(reply *sqltypes.Result) error) error - // Shard-level functions. - ExecuteMultiShard(rss []*srvtopo.ResolvedShard, queries []*querypb.BoundQuery, rollbackOnError, canAutocommit bool) (*sqltypes.Result, []error) - ExecuteStandalone(query string, bindvars map[string]*querypb.BindVariable, rs *srvtopo.ResolvedShard) (*sqltypes.Result, error) - StreamExecuteMulti(query string, rss []*srvtopo.ResolvedShard, bindVars []map[string]*querypb.BindVariable, callback func(reply *sqltypes.Result) error) error + // Keyspace ID level functions. + ExecuteKeyspaceID(keyspace string, ksid []byte, query string, bindVars map[string]*querypb.BindVariable, rollbackOnError, autocommit bool) (*sqltypes.Result, error) - // Keyspace ID level functions. - ExecuteKeyspaceID(keyspace string, ksid []byte, query string, bindVars map[string]*querypb.BindVariable, rollbackOnError, autocommit bool) (*sqltypes.Result, error) + // Resolver methods, from key.Destination to srvtopo.ResolvedShard. + // Will replace all of the Topo functions. + ResolveDestinations(keyspace string, ids []*querypb.Value, destinations []key.Destination) ([]*srvtopo.ResolvedShard, [][]*querypb.Value, error) - // Resolver methods, from key.Destination to srvtopo.ResolvedShard. - // Will replace all of the Topo functions. - ResolveDestinations(keyspace string, ids []*querypb.Value, destinations []key.Destination) ([]*srvtopo.ResolvedShard, [][]*querypb.Value, error) + ExecuteVSchema(keyspace string, vschemaDDL *sqlparser.DDL) error - SetTarget(target string) error - SetUDV(key string, value interface{}) error + Session() SessionActions + } - ExecuteVSchema(keyspace string, vschemaDDL *sqlparser.DDL) error -} + //SessionActions gives primitives ability to interact with the session state + SessionActions interface { + // RecordWarning stores the given warning in the current session + RecordWarning(warning *querypb.QueryWarning) -// Plan represents the execution strategy for a given query. -// For now it's a simple wrapper around the real instructions. -// An instruction (aka Primitive) is typically a tree where -// each node does its part by combining the results of the -// sub-nodes. -type Plan struct { - Type sqlparser.StatementType // The type of query we have - Original string // Original is the original query. - Instructions Primitive // Instructions contains the instructions needed to fulfil the query. - sqlparser.BindVarNeeds // Stores BindVars needed to be provided as part of expression rewriting - - mu sync.Mutex // Mutex to protect the fields below - ExecCount uint64 // Count of times this plan was executed - ExecTime time.Duration // Total execution time - ShardQueries uint64 // Total number of shard queries - Rows uint64 // Total number of rows - Errors uint64 // Total number of errors -} + SetTarget(target string) error + SetUDV(key string, value interface{}) error + } + + // Plan represents the execution strategy for a given query. + // For now it's a simple wrapper around the real instructions. + // An instruction (aka Primitive) is typically a tree where + // each node does its part by combining the results of the + // sub-nodes. + Plan struct { + Type sqlparser.StatementType // The type of query we have + Original string // Original is the original query. + Instructions Primitive // Instructions contains the instructions needed to fulfil the query. + sqlparser.BindVarNeeds // Stores BindVars needed to be provided as part of expression rewriting + + mu sync.Mutex // Mutex to protect the fields below + ExecCount uint64 // Count of times this plan was executed + ExecTime time.Duration // Total execution time + ShardQueries uint64 // Total number of shard queries + Rows uint64 // Total number of rows + Errors uint64 // Total number of errors + } + + // Match is used to check if a Primitive matches + Match func(node Primitive) bool + + // Primitive is the building block of the engine execution plan. They form a tree structure, where the leaves typically + // issue queries to one or more vttablet. + // During execution, the Primitive's pass Result objects up the tree structure, until reaching the root, + // and its result is passed to the client. + Primitive interface { + RouteType() string + GetKeyspaceName() string + GetTableName() string + Execute(vcursor VCursor, bindVars map[string]*querypb.BindVariable, wantfields bool) (*sqltypes.Result, error) + StreamExecute(vcursor VCursor, bindVars map[string]*querypb.BindVariable, wantields bool, callback func(*sqltypes.Result) error) error + GetFields(vcursor VCursor, bindVars map[string]*querypb.BindVariable) (*sqltypes.Result, error) + NeedsTransaction() bool + + // The inputs to this Primitive + Inputs() []Primitive + + // description is the description, sans the inputs, of this Primitive. + // to get the plan description with all children, use PrimitiveToPlanDescription() + description() PrimitiveDescription + } + + // noInputs default implementation for Primitives that are leaves + noInputs struct{} + + // noTxNeeded is a default implementation for Primitives that don't need transaction handling + noTxNeeded struct{} + + // txNeeded is a default implementation for Primitives that need transaction handling + txNeeded struct{} +) // AddStats updates the plan execution statistics func (p *Plan) AddStats(execCount uint64, execTime time.Duration, shardQueries, rows, errors uint64) { @@ -121,9 +161,6 @@ func (p *Plan) Stats() (execCount uint64, execTime time.Duration, shardQueries, return } -// Match is used to check if a Primitive matches -type Match func(node Primitive) bool - // Find will return the first Primitive that matches the evaluate function. If no match is found, nil will be returned func Find(isMatch Match, start Primitive) Primitive { if isMatch(start) { @@ -180,42 +217,15 @@ func (p *Plan) MarshalJSON() ([]byte, error) { return json.Marshal(marshalPlan) } -// Primitive is the building block of the engine execution plan. They form a tree structure, where the leaves typically -// issue queries to one or more vttablet. -// During execution, the Primitive's pass Result objects up the tree structure, until reaching the root, -// and its result is passed to the client. -type Primitive interface { - RouteType() string - GetKeyspaceName() string - GetTableName() string - Execute(vcursor VCursor, bindVars map[string]*querypb.BindVariable, wantfields bool) (*sqltypes.Result, error) - StreamExecute(vcursor VCursor, bindVars map[string]*querypb.BindVariable, wantields bool, callback func(*sqltypes.Result) error) error - GetFields(vcursor VCursor, bindVars map[string]*querypb.BindVariable) (*sqltypes.Result, error) - NeedsTransaction() bool - - // The inputs to this Primitive - Inputs() []Primitive - - // description is the description, sans the inputs, of this Primitive. - // to get the plan description with all children, use PrimitiveToPlanDescription() - description() PrimitiveDescription -} - -type noInputs struct{} - // Inputs implements no inputs func (noInputs) Inputs() []Primitive { return nil } -type noTxNeeded struct{} - func (noTxNeeded) NeedsTransaction() bool { return false } -type txNeeded struct{} - func (txNeeded) NeedsTransaction() bool { return true } diff --git a/go/vt/vtgate/engine/route.go b/go/vt/vtgate/engine/route.go index 9fff1e5afeb..6846ac5498e 100644 --- a/go/vt/vtgate/engine/route.go +++ b/go/vt/vtgate/engine/route.go @@ -254,7 +254,7 @@ func (route *Route) execute(vcursor VCursor, bindVars map[string]*querypb.BindVa for _, err := range errs { if err != nil { serr := mysql.NewSQLErrorFromError(err).(*mysql.SQLError) - vcursor.RecordWarning(&querypb.QueryWarning{Code: uint32(serr.Num), Message: err.Error()}) + vcursor.Session().RecordWarning(&querypb.QueryWarning{Code: uint32(serr.Num), Message: err.Error()}) } } // fall through diff --git a/go/vt/vtgate/engine/set.go b/go/vt/vtgate/engine/set.go index 7408f49698d..4e151f97a02 100644 --- a/go/vt/vtgate/engine/set.go +++ b/go/vt/vtgate/engine/set.go @@ -137,7 +137,7 @@ func (u *UserDefinedVariable) Execute(vcursor VCursor, bindVars map[string]*quer if err != nil { return err } - return vcursor.SetUDV(u.Name, value) + return vcursor.Session().SetUDV(u.Name, value) } var _ SetOp = (*SysVarIgnore)(nil) diff --git a/go/vt/vtgate/engine/update_target.go b/go/vt/vtgate/engine/update_target.go index f297c43e941..f637d4166e7 100644 --- a/go/vt/vtgate/engine/update_target.go +++ b/go/vt/vtgate/engine/update_target.go @@ -60,7 +60,7 @@ func (updTarget UpdateTarget) GetTableName() string { // Execute implements the Primitive interface func (updTarget UpdateTarget) Execute(vcursor VCursor, bindVars map[string]*query.BindVariable, wantfields bool) (*sqltypes.Result, error) { - err := vcursor.SetTarget(updTarget.Target) + err := vcursor.Session().SetTarget(updTarget.Target) if err != nil { return nil, err } diff --git a/go/vt/vtgate/vcursor_impl.go b/go/vt/vtgate/vcursor_impl.go index 02a167846ed..25ec1cb4ce2 100644 --- a/go/vt/vtgate/vcursor_impl.go +++ b/go/vt/vtgate/vcursor_impl.go @@ -312,6 +312,10 @@ func (vc *vcursorImpl) ResolveDestinations(keyspace string, ids []*querypb.Value return vc.resolver.ResolveDestinations(vc.ctx, keyspace, vc.tabletType, ids, destinations) } +func (vc *vcursorImpl) Session() engine.SessionActions { + return vc +} + func (vc *vcursorImpl) SetTarget(target string) error { keyspace, tabletType, _, err := parseDestinationTarget(target, vc.vschema) if err != nil { From b56705bc7b6cc0f742bb3637af99d0eaadb85e3b Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 15 Apr 2020 18:30:27 +0530 Subject: [PATCH 09/10] added set primitive tests Signed-off-by: Harshit Gangal --- .../endtoend/vtgate/setstatement/main_test.go | 16 ++ .../vtgate/setstatement/sysvar_test.go | 18 +- .../endtoend/vtgate/setstatement/udv_test.go | 18 +- go/vt/vtgate/engine/send_test.go | 16 ++ go/vt/vtgate/engine/set.go | 6 +- go/vt/vtgate/engine/set_test.go | 199 ++++++++++++++++++ 6 files changed, 269 insertions(+), 4 deletions(-) create mode 100644 go/vt/vtgate/engine/set_test.go diff --git a/go/test/endtoend/vtgate/setstatement/main_test.go b/go/test/endtoend/vtgate/setstatement/main_test.go index 2c6208236fe..1bb654e694b 100644 --- a/go/test/endtoend/vtgate/setstatement/main_test.go +++ b/go/test/endtoend/vtgate/setstatement/main_test.go @@ -1,3 +1,19 @@ +/* +Copyright 2020 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package setstatement import ( diff --git a/go/test/endtoend/vtgate/setstatement/sysvar_test.go b/go/test/endtoend/vtgate/setstatement/sysvar_test.go index 77f6a4578a3..ca32f7f30d7 100644 --- a/go/test/endtoend/vtgate/setstatement/sysvar_test.go +++ b/go/test/endtoend/vtgate/setstatement/sysvar_test.go @@ -1,3 +1,19 @@ +/* +Copyright 2020 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package setstatement import ( @@ -50,7 +66,7 @@ func TestSetSysVar(t *testing.T) { if q.errMsg != "" { require.Contains(t, err.Error(), q.errMsg) } else { - require.Nil(t, err) + require.NoError(t, err) require.Equal(t, uint64(q.rowsAffected), qr.RowsAffected, "rows affected wrong for query: %s", q.query) if q.expectedRows != "" { result := fmt.Sprintf("%v", qr.Rows) diff --git a/go/test/endtoend/vtgate/setstatement/udv_test.go b/go/test/endtoend/vtgate/setstatement/udv_test.go index f7e15ebb690..8e2cff6e72d 100644 --- a/go/test/endtoend/vtgate/setstatement/udv_test.go +++ b/go/test/endtoend/vtgate/setstatement/udv_test.go @@ -1,3 +1,19 @@ +/* +Copyright 2020 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package setstatement import ( @@ -70,7 +86,7 @@ func TestSetUDV(t *testing.T) { for i, q := range queries { t.Run(fmt.Sprintf("%d-%s", i, q.query), func(t *testing.T) { qr, err := exec(t, conn, q.query) - require.Nil(t, err) + require.NoError(t, err) require.Equal(t, uint64(q.rowsAffected), qr.RowsAffected, "rows affected wrong for query: %s", q.query) if q.expectedRows != "" { result := fmt.Sprintf("%v", qr.Rows) diff --git a/go/vt/vtgate/engine/send_test.go b/go/vt/vtgate/engine/send_test.go index 5c8bf8efa03..4604ce0e488 100644 --- a/go/vt/vtgate/engine/send_test.go +++ b/go/vt/vtgate/engine/send_test.go @@ -1,3 +1,19 @@ +/* +Copyright 2020 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package engine import ( diff --git a/go/vt/vtgate/engine/set.go b/go/vt/vtgate/engine/set.go index 4e151f97a02..e4e7e1ab856 100644 --- a/go/vt/vtgate/engine/set.go +++ b/go/vt/vtgate/engine/set.go @@ -18,10 +18,12 @@ package engine import ( "encoding/json" + "fmt" + + "vitess.io/vitess/go/mysql" "vitess.io/vitess/go/sqltypes" "vitess.io/vitess/go/vt/key" - "vitess.io/vitess/go/vt/log" querypb "vitess.io/vitess/go/vt/proto/query" vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/vterrors" @@ -165,7 +167,7 @@ func (svi *SysVarIgnore) Execute(vcursor VCursor, bindVars map[string]*querypb.B if err != nil { return err } - log.Warningf("Ignored inapplicable SET %v = %v", svi.Name, value.String()) + vcursor.Session().RecordWarning(&querypb.QueryWarning{Code: mysql.ERNotSupportedYet, Message: fmt.Sprintf("Ignored inapplicable SET %v = %v", svi.Name, value.String())}) return nil } diff --git a/go/vt/vtgate/engine/set_test.go b/go/vt/vtgate/engine/set_test.go new file mode 100644 index 00000000000..49551e75f9d --- /dev/null +++ b/go/vt/vtgate/engine/set_test.go @@ -0,0 +1,199 @@ +/* +Copyright 2020 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package engine + +import ( + "testing" + + "vitess.io/vitess/go/sqltypes" + "vitess.io/vitess/go/vt/key" + "vitess.io/vitess/go/vt/vtgate/vindexes" + + "github.com/stretchr/testify/require" + + querypb "vitess.io/vitess/go/vt/proto/query" +) + +func TestSetTable(t *testing.T) { + type testCase struct { + testName string + setOps []SetOp + qr []*sqltypes.Result + expectedQueryLog []string + expectedWarning []*querypb.QueryWarning + expectedError string + } + + tests := []testCase{ + { + testName: "nil set ops", + expectedQueryLog: []string{}, + }, + { + testName: "udv", + setOps: []SetOp{ + &UserDefinedVariable{ + Name: "x", + PlanValue: sqltypes.PlanValue{ + Value: sqltypes.NewInt64(42), + }, + }, + }, + expectedQueryLog: []string{ + `UDV set with (x,INT64(42))`, + }, + }, + { + testName: "sysvar ignore", + setOps: []SetOp{ + &SysVarIgnore{ + Name: "x", + PlanValue: sqltypes.PlanValue{ + Value: sqltypes.NewInt64(42), + }, + }, + }, + expectedWarning: []*querypb.QueryWarning{ + {Code: 1235, Message: "Ignored inapplicable SET x = INT64(42)"}, + }, + }, + { + testName: "sysvar check and ignore", + setOps: []SetOp{ + &SysVarCheckAndIgnore{ + Name: "x", + Keyspace: &vindexes.Keyspace{ + Name: "ks", + Sharded: true, + }, + TargetDestination: key.DestinationAnyShard{}, + CheckSysVarQuery: "dummy_query", + }, + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult( + sqltypes.MakeTestFields( + "id", + "int64", + ), + "1", + )}, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, + `ExecuteMultiShard ks.-20: dummy_query {} false false`, + }, + }, + { + testName: "sysvar check and error", + setOps: []SetOp{ + &SysVarCheckAndIgnore{ + Name: "x", + Keyspace: &vindexes.Keyspace{ + Name: "ks", + Sharded: true, + }, + TargetDestination: key.DestinationAnyShard{}, + CheckSysVarQuery: "dummy_query", + }, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, + `ExecuteMultiShard ks.-20: dummy_query {} false false`, + }, + expectedError: "Modification not allowed using set construct for: x", + }, + { + testName: "sysvar checkAndIgnore multi destination error", + setOps: []SetOp{ + &SysVarCheckAndIgnore{ + Name: "x", + Keyspace: &vindexes.Keyspace{ + Name: "ks", + Sharded: true, + }, + TargetDestination: key.DestinationAllShards{}, + CheckSysVarQuery: "dummy_query", + }, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationAllShards()`, + }, + expectedError: "Unexpected error, DestinationKeyspaceID mapping to multiple shards: DestinationAllShards()", + }, + { + testName: "udv_ignr_chignr", + setOps: []SetOp{ + &UserDefinedVariable{ + Name: "x", + PlanValue: sqltypes.PlanValue{ + Value: sqltypes.NewInt64(1), + }, + }, + &SysVarIgnore{ + Name: "y", + PlanValue: sqltypes.PlanValue{ + Value: sqltypes.NewInt64(2), + }, + }, + &SysVarCheckAndIgnore{ + Name: "z", + Keyspace: &vindexes.Keyspace{ + Name: "ks", + Sharded: true, + }, + TargetDestination: key.DestinationAnyShard{}, + CheckSysVarQuery: "dummy_query", + }, + }, + expectedQueryLog: []string{ + `UDV set with (x,INT64(1))`, + `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, + `ExecuteMultiShard ks.-20: dummy_query {} false false`, + }, + expectedWarning: []*querypb.QueryWarning{ + {Code: 1235, Message: "Ignored inapplicable SET y = INT64(2)"}, + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult( + sqltypes.MakeTestFields( + "id", + "int64", + ), + "1", + )}, + }, + } + + for _, tc := range tests { + t.Run(tc.testName, func(t *testing.T) { + set := &Set{ + Ops: tc.setOps, + } + vc := &loggingVCursor{ + shards: []string{"-20", "20-"}, + results: tc.qr, + } + _, err := set.Execute(vc, map[string]*querypb.BindVariable{}, false) + if tc.expectedError == "" { + require.NoError(t, err) + } else { + require.EqualError(t, err, tc.expectedError) + } + + vc.ExpectLog(t, tc.expectedQueryLog) + vc.ExpectWarnings(t, tc.expectedWarning) + }) + } +} From 470a63ccc3fdd35d0c974abb3d133f06184ac631 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Wed, 15 Apr 2020 19:22:15 +0530 Subject: [PATCH 10/10] modified sysvarIgnore op to store expr string than plan value Signed-off-by: Harshit Gangal --- .../vtgate/setstatement/sysvar_test.go | 19 ++++++++++++++----- go/vt/vtgate/engine/set.go | 10 +++------- go/vt/vtgate/engine/set_test.go | 12 ++++-------- go/vt/vtgate/planbuilder/set.go | 11 +++++------ .../planbuilder/testdata/set_sysvar_cases.txt | 2 +- 5 files changed, 27 insertions(+), 27 deletions(-) diff --git a/go/test/endtoend/vtgate/setstatement/sysvar_test.go b/go/test/endtoend/vtgate/setstatement/sysvar_test.go index ca32f7f30d7..3db7aaadb81 100644 --- a/go/test/endtoend/vtgate/setstatement/sysvar_test.go +++ b/go/test/endtoend/vtgate/setstatement/sysvar_test.go @@ -36,15 +36,17 @@ func TestSetSysVar(t *testing.T) { Port: clusterInstance.VtgateMySQLPort, } type queriesWithExpectations struct { - query string - expectedRows string - rowsAffected int - errMsg string + query string + expectedRows string + rowsAffected int + errMsg string + expectedWarning string } queries := []queriesWithExpectations{{ - query: `set @@debug = 'T'`, + query: `set @@default_storage_engine = INNODB`, expectedRows: ``, rowsAffected: 0, + expectedWarning: "[[VARCHAR(\"Warning\") UINT16(1235) VARCHAR(\"Ignored inapplicable SET default_storage_engine = INNODB\")]]", }, { query: `set @@sql_mode = @@sql_mode`, expectedRows: ``, rowsAffected: 0, @@ -74,6 +76,13 @@ func TestSetSysVar(t *testing.T) { t.Errorf("%s\nfor query: %s", diff, q.query) } } + if q.expectedWarning != "" { + qr, err := exec(t, conn, "show warnings") + require.NoError(t, err) + if got, want := fmt.Sprintf("%v", qr.Rows), q.expectedWarning; got != want { + t.Errorf("select:\n%v want\n%v", got, want) + } + } } }) } diff --git a/go/vt/vtgate/engine/set.go b/go/vt/vtgate/engine/set.go index e4e7e1ab856..42d5a173f2c 100644 --- a/go/vt/vtgate/engine/set.go +++ b/go/vt/vtgate/engine/set.go @@ -52,8 +52,8 @@ type ( // SysVarIgnore implements the SetOp interface to ignore the settings. SysVarIgnore struct { - Name string - PlanValue sqltypes.PlanValue + Name string + Expr string } // SysVarCheckAndIgnore implements the SetOp interface to check underlying setting and ignore if same. @@ -163,11 +163,7 @@ func (svi *SysVarIgnore) VariableName() string { //Execute implements the SetOp interface method. func (svi *SysVarIgnore) Execute(vcursor VCursor, bindVars map[string]*querypb.BindVariable) error { - value, err := svi.PlanValue.ResolveValue(bindVars) - if err != nil { - return err - } - vcursor.Session().RecordWarning(&querypb.QueryWarning{Code: mysql.ERNotSupportedYet, Message: fmt.Sprintf("Ignored inapplicable SET %v = %v", svi.Name, value.String())}) + vcursor.Session().RecordWarning(&querypb.QueryWarning{Code: mysql.ERNotSupportedYet, Message: fmt.Sprintf("Ignored inapplicable SET %v = %v", svi.Name, svi.Expr)}) return nil } diff --git a/go/vt/vtgate/engine/set_test.go b/go/vt/vtgate/engine/set_test.go index 49551e75f9d..a243445b4c6 100644 --- a/go/vt/vtgate/engine/set_test.go +++ b/go/vt/vtgate/engine/set_test.go @@ -62,13 +62,11 @@ func TestSetTable(t *testing.T) { setOps: []SetOp{ &SysVarIgnore{ Name: "x", - PlanValue: sqltypes.PlanValue{ - Value: sqltypes.NewInt64(42), - }, + Expr: "42", }, }, expectedWarning: []*querypb.QueryWarning{ - {Code: 1235, Message: "Ignored inapplicable SET x = INT64(42)"}, + {Code: 1235, Message: "Ignored inapplicable SET x = 42"}, }, }, { @@ -144,9 +142,7 @@ func TestSetTable(t *testing.T) { }, &SysVarIgnore{ Name: "y", - PlanValue: sqltypes.PlanValue{ - Value: sqltypes.NewInt64(2), - }, + Expr: "2", }, &SysVarCheckAndIgnore{ Name: "z", @@ -164,7 +160,7 @@ func TestSetTable(t *testing.T) { `ExecuteMultiShard ks.-20: dummy_query {} false false`, }, expectedWarning: []*querypb.QueryWarning{ - {Code: 1235, Message: "Ignored inapplicable SET y = INT64(2)"}, + {Code: 1235, Message: "Ignored inapplicable SET y = 2"}, }, qr: []*sqltypes.Result{sqltypes.MakeTestResult( sqltypes.MakeTestFields( diff --git a/go/vt/vtgate/planbuilder/set.go b/go/vt/vtgate/planbuilder/set.go index 91290a69ab6..9330e9dd46e 100644 --- a/go/vt/vtgate/planbuilder/set.go +++ b/go/vt/vtgate/planbuilder/set.go @@ -73,13 +73,12 @@ func buildSetPlan(sql string, stmt *sqlparser.Set, vschema ContextVSchema) (engi } func buildSetOpIgnore(expr *sqlparser.SetExpr, _ ContextVSchema) (engine.SetOp, error) { - pv, err := sqlparser.NewPlanValue(expr.Expr) - if err != nil { - return nil, err - } + buf := sqlparser.NewTrackedBuffer(nil) + buf.Myprintf("%v", expr.Expr) + return &engine.SysVarIgnore{ - Name: expr.Name.Lowered(), - PlanValue: pv, + Name: expr.Name.Lowered(), + Expr: buf.String(), }, nil } diff --git a/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt b/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt index fa0bced7e0f..5d554ab27a7 100644 --- a/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/set_sysvar_cases.txt @@ -10,7 +10,7 @@ { "Type": "SysVarIgnore", "Name": "default_storage_engine", - "PlanValue": "DONOTCHANGEME" + "Expr": "'DONOTCHANGEME'" } ] }