From 549dcac5c8d1d4f58efe98cd92cd999067ee0dbe Mon Sep 17 00:00:00 2001 From: Jacques Grove Date: Thu, 5 Dec 2019 17:54:51 -0800 Subject: [PATCH 1/2] Revert "set the type of 'dual' table as 'TypeReference'" This reverts commit 001ec0777a75795526e071bf2570700c950ea632. --- go/vt/vtgate/planbuilder/expr.go | 2 ++ go/vt/vtgate/planbuilder/from.go | 20 ++++++++++----- go/vt/vtgate/planbuilder/route.go | 9 +++++++ .../planbuilder/testdata/select_cases.txt | 12 +++++---- .../testdata/unsupported_cases.txt | 6 ++--- go/vt/vtgate/vindexes/vschema.go | 4 ++- go/vt/vtgate/vindexes/vschema_test.go | 25 ++++++------------- 7 files changed, 46 insertions(+), 32 deletions(-) diff --git a/go/vt/vtgate/planbuilder/expr.go b/go/vt/vtgate/planbuilder/expr.go index 2e4bd4baeaa..9ffe0ce0154 100644 --- a/go/vt/vtgate/planbuilder/expr.go +++ b/go/vt/vtgate/planbuilder/expr.go @@ -156,6 +156,8 @@ func (pb *primitiveBuilder) findOrigin(expr sqlparser.Expr) (pullouts []*pullout case node.Name.EqualString("last_insert_id"): if rb, isRoute := pb.bldr.(*route); !isRoute || !rb.removeShardedOptions() { return false, errors.New("unsupported: LAST_INSERT_ID is only allowed for unsharded keyspaces") + } else if rb.containsDualTable() { + return false, errors.New("unsupported: LAST_INSERT_ID is not allowed for dual table") } } return true, nil diff --git a/go/vt/vtgate/planbuilder/from.go b/go/vt/vtgate/planbuilder/from.go index 7f919ca21c0..90a03b9ebda 100644 --- a/go/vt/vtgate/planbuilder/from.go +++ b/go/vt/vtgate/planbuilder/from.go @@ -19,6 +19,7 @@ package planbuilder import ( "errors" "fmt" + "strings" "vitess.io/vitess/go/sqltypes" "vitess.io/vitess/go/vt/sqlparser" @@ -240,12 +241,19 @@ func (pb *primitiveBuilder) buildTablePrimitive(tableExpr *sqlparser.AliasedTabl eroute.TargetDestination = destTarget eroute.TargetTabletType = destTableType default: - // Pinned tables have their keyspace ids already assigned. - // Use the Binary vindex, which is the identity function - // for keyspace id. - eroute = engine.NewSimpleRoute(engine.SelectEqualUnique, vst.Keyspace) - eroute.Vindex, _ = vindexes.NewBinary("binary", nil) - eroute.Values = []sqltypes.PlanValue{{Value: sqltypes.MakeTrusted(sqltypes.VarBinary, vst.Pinned)}} + // In some cases, JDBC Driver may produce tons of pointless queries like 'select 1 from dual', + // 'select @@session.tx_read_only from dual',etc. To avoid causing significant burden on one + // specific vttablet, we treat those 'dual' table queries as SelectReference. + if strings.EqualFold(tableName.Name.String(), "dual") && tableName.Qualifier.String() == "" { + eroute = engine.NewSimpleRoute(engine.SelectReference, vst.Keyspace) + } else { + // Pinned tables have their keyspace ids already assigned. + // Use the Binary vindex, which is the identity function + // for keyspace id. Currently only dual tables are pinned. + eroute = engine.NewSimpleRoute(engine.SelectEqualUnique, vst.Keyspace) + eroute.Vindex, _ = vindexes.NewBinary("binary", nil) + eroute.Values = []sqltypes.PlanValue{{Value: sqltypes.MakeTrusted(sqltypes.VarBinary, vst.Pinned)}} + } } // set table name into route eroute.TableName = vst.Name.String() diff --git a/go/vt/vtgate/planbuilder/route.go b/go/vt/vtgate/planbuilder/route.go index 1f58eadbda8..f694e68bdf4 100644 --- a/go/vt/vtgate/planbuilder/route.go +++ b/go/vt/vtgate/planbuilder/route.go @@ -503,6 +503,15 @@ func (rb *route) removeShardedOptions() bool { }) } +func (rb *route) containsDualTable() bool { + for _, ro := range rb.routeOptions { + if ro.vschemaTable.Name.String() == "dual" { + return true + } + } + return false +} + // removeOptionsWithUnmatchedKeyspace removes all options that don't match // the specified keyspace. It returns false if no such options exist. func (rb *route) removeOptionsWithUnmatchedKeyspace(keyspace string) bool { diff --git a/go/vt/vtgate/planbuilder/testdata/select_cases.txt b/go/vt/vtgate/planbuilder/testdata/select_cases.txt index bf0ce701b1c..f89e3c49410 100644 --- a/go/vt/vtgate/planbuilder/testdata/select_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/select_cases.txt @@ -376,7 +376,7 @@ { "Original": "select database() from dual", "Instructions": { - "Opcode": "SelectReference", + "Opcode": "SelectUnsharded", "Keyspace": { "Name": "main", "Sharded": false @@ -392,9 +392,9 @@ "NEXT used on a non-sequence table" # last_insert_id for unsharded route -"select last_insert_id() from main.unsharded" +"select last_insert_id() from unsharded" { - "Original": "select last_insert_id() from main.unsharded", + "Original": "select last_insert_id() from unsharded", "Instructions": { "Opcode": "SelectUnsharded", "Keyspace": { @@ -412,7 +412,7 @@ { "Original": "select @@session.auto_increment_increment from dual", "Instructions": { - "Opcode": "SelectReference", + "Opcode": "SelectUnsharded", "Keyspace": { "Name": "main", "Sharded": false @@ -449,13 +449,15 @@ { "Original": "select @@session.auto_increment_increment from user.dual", "Instructions": { - "Opcode": "SelectReference", + "Opcode": "SelectEqualUnique", "Keyspace": { "Name": "user", "Sharded": true }, "Query": "select @@session.auto_increment_increment from dual", "FieldQuery": "select @@session.auto_increment_increment from dual where 1 != 1", + "Vindex": "binary", + "Values": ["\u0000"], "Table": "dual" } } diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt index 77cee7da742..8792eeb37af 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt @@ -71,9 +71,9 @@ "select last_insert_id() from user" "unsupported: LAST_INSERT_ID is only allowed for unsharded keyspaces" -# last_insert_id for dual -"select last_insert_id()" -"unsupported: LAST_INSERT_ID is only allowed for unsharded keyspaces" +# last_insert_id for dual table +"select last_insert_id() from dual" +"unsupported: LAST_INSERT_ID is not allowed for dual table" # natural join "select * from user natural join user_extra" diff --git a/go/vt/vtgate/vindexes/vschema.go b/go/vt/vtgate/vindexes/vschema.go index 8f519fb7093..5242081ab09 100644 --- a/go/vt/vtgate/vindexes/vschema.go +++ b/go/vt/vtgate/vindexes/vschema.go @@ -367,7 +367,9 @@ func addDual(vschema *VSchema) { t := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ks.Keyspace, - Type: TypeReference, + } + if ks.Keyspace.Sharded { + t.Pinned = []byte{0} } ks.Tables["dual"] = t if first == "" || first > ksname { diff --git a/go/vt/vtgate/vindexes/vschema_test.go b/go/vt/vtgate/vindexes/vschema_test.go index 200636c2388..b831a03e571 100644 --- a/go/vt/vtgate/vindexes/vschema_test.go +++ b/go/vt/vtgate/vindexes/vschema_test.go @@ -172,7 +172,6 @@ func TestUnshardedVSchema(t *testing.T) { dual := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ks, - Type: TypeReference, } want := &VSchema{ RoutingRules: map[string]*RoutingRule{}, @@ -236,7 +235,6 @@ func TestVSchemaColumns(t *testing.T) { dual := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ks, - Type: TypeReference, } want := &VSchema{ RoutingRules: map[string]*RoutingRule{}, @@ -303,7 +301,6 @@ func TestVSchemaColumnListAuthoritative(t *testing.T) { dual := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ks, - Type: TypeReference, } want := &VSchema{ RoutingRules: map[string]*RoutingRule{}, @@ -384,7 +381,7 @@ func TestVSchemaPinned(t *testing.T) { dual := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ks, - Type: TypeReference, + Pinned: []byte{0}, } want := &VSchema{ RoutingRules: map[string]*RoutingRule{}, @@ -488,7 +485,7 @@ func TestShardedVSchemaOwned(t *testing.T) { dual := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ks, - Type: TypeReference, + Pinned: []byte{0}, } want := &VSchema{ RoutingRules: map[string]*RoutingRule{}, @@ -705,12 +702,11 @@ func TestVSchemaRoutingRules(t *testing.T) { dual1 := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ks1, - Type: TypeReference, + Pinned: []byte{0}, } dual2 := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ks2, - Type: TypeReference, } want := &VSchema{ RoutingRules: map[string]*RoutingRule{ @@ -948,7 +944,7 @@ func TestShardedVSchemaMultiColumnVindex(t *testing.T) { dual := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ks, - Type: TypeReference, + Pinned: []byte{0}, } want := &VSchema{ RoutingRules: map[string]*RoutingRule{}, @@ -1048,7 +1044,7 @@ func TestShardedVSchemaNotOwned(t *testing.T) { dual := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ks, - Type: TypeReference, + Pinned: []byte{0}, } want := &VSchema{ RoutingRules: map[string]*RoutingRule{}, @@ -1175,12 +1171,10 @@ func TestBuildVSchemaDupSeq(t *testing.T) { duala := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ksa, - Type: TypeReference, } dualb := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ksb, - Type: TypeReference, } want := &VSchema{ RoutingRules: map[string]*RoutingRule{}, @@ -1248,12 +1242,10 @@ func TestBuildVSchemaDupTable(t *testing.T) { duala := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ksa, - Type: TypeReference, } dualb := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ksb, - Type: TypeReference, } want := &VSchema{ RoutingRules: map[string]*RoutingRule{}, @@ -1384,12 +1376,12 @@ func TestBuildVSchemaDupVindex(t *testing.T) { duala := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ksa, - Type: TypeReference, + Pinned: []byte{0}, } dualb := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ksb, - Type: TypeReference, + Pinned: []byte{0}, } want := &VSchema{ RoutingRules: map[string]*RoutingRule{}, @@ -1671,12 +1663,11 @@ func TestSequence(t *testing.T) { duala := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: ksu, - Type: TypeReference, } dualb := &Table{ Name: sqlparser.NewTableIdent("dual"), Keyspace: kss, - Type: TypeReference, + Pinned: []byte{0}, } want := &VSchema{ RoutingRules: map[string]*RoutingRule{}, From e0b339664f834a161747cb2261ef38d74ef9067b Mon Sep 17 00:00:00 2001 From: Jacques Grove Date: Thu, 5 Dec 2019 17:54:57 -0800 Subject: [PATCH 2/2] Revert "treat the route of sql with pseudo table as SelectReference" This reverts commit 27b5aae3f9219bd2bfcad6ff80f8410904996c54. Signed-off-by: Jacques Grove --- go/vt/vtgate/planbuilder/expr.go | 2 -- go/vt/vtgate/planbuilder/from.go | 20 ++++++------------- go/vt/vtgate/planbuilder/route.go | 9 --------- .../planbuilder/testdata/select_cases.txt | 10 +++++----- .../testdata/unsupported_cases.txt | 4 ---- 5 files changed, 11 insertions(+), 34 deletions(-) diff --git a/go/vt/vtgate/planbuilder/expr.go b/go/vt/vtgate/planbuilder/expr.go index 9ffe0ce0154..2e4bd4baeaa 100644 --- a/go/vt/vtgate/planbuilder/expr.go +++ b/go/vt/vtgate/planbuilder/expr.go @@ -156,8 +156,6 @@ func (pb *primitiveBuilder) findOrigin(expr sqlparser.Expr) (pullouts []*pullout case node.Name.EqualString("last_insert_id"): if rb, isRoute := pb.bldr.(*route); !isRoute || !rb.removeShardedOptions() { return false, errors.New("unsupported: LAST_INSERT_ID is only allowed for unsharded keyspaces") - } else if rb.containsDualTable() { - return false, errors.New("unsupported: LAST_INSERT_ID is not allowed for dual table") } } return true, nil diff --git a/go/vt/vtgate/planbuilder/from.go b/go/vt/vtgate/planbuilder/from.go index 90a03b9ebda..8f47e0822ee 100644 --- a/go/vt/vtgate/planbuilder/from.go +++ b/go/vt/vtgate/planbuilder/from.go @@ -19,7 +19,6 @@ package planbuilder import ( "errors" "fmt" - "strings" "vitess.io/vitess/go/sqltypes" "vitess.io/vitess/go/vt/sqlparser" @@ -241,19 +240,12 @@ func (pb *primitiveBuilder) buildTablePrimitive(tableExpr *sqlparser.AliasedTabl eroute.TargetDestination = destTarget eroute.TargetTabletType = destTableType default: - // In some cases, JDBC Driver may produce tons of pointless queries like 'select 1 from dual', - // 'select @@session.tx_read_only from dual',etc. To avoid causing significant burden on one - // specific vttablet, we treat those 'dual' table queries as SelectReference. - if strings.EqualFold(tableName.Name.String(), "dual") && tableName.Qualifier.String() == "" { - eroute = engine.NewSimpleRoute(engine.SelectReference, vst.Keyspace) - } else { - // Pinned tables have their keyspace ids already assigned. - // Use the Binary vindex, which is the identity function - // for keyspace id. Currently only dual tables are pinned. - eroute = engine.NewSimpleRoute(engine.SelectEqualUnique, vst.Keyspace) - eroute.Vindex, _ = vindexes.NewBinary("binary", nil) - eroute.Values = []sqltypes.PlanValue{{Value: sqltypes.MakeTrusted(sqltypes.VarBinary, vst.Pinned)}} - } + // Pinned tables have their keyspace ids already assigned. + // Use the Binary vindex, which is the identity function + // for keyspace id. Currently only dual tables are pinned. + eroute = engine.NewSimpleRoute(engine.SelectEqualUnique, vst.Keyspace) + eroute.Vindex, _ = vindexes.NewBinary("binary", nil) + eroute.Values = []sqltypes.PlanValue{{Value: sqltypes.MakeTrusted(sqltypes.VarBinary, vst.Pinned)}} } // set table name into route eroute.TableName = vst.Name.String() diff --git a/go/vt/vtgate/planbuilder/route.go b/go/vt/vtgate/planbuilder/route.go index f694e68bdf4..1f58eadbda8 100644 --- a/go/vt/vtgate/planbuilder/route.go +++ b/go/vt/vtgate/planbuilder/route.go @@ -503,15 +503,6 @@ func (rb *route) removeShardedOptions() bool { }) } -func (rb *route) containsDualTable() bool { - for _, ro := range rb.routeOptions { - if ro.vschemaTable.Name.String() == "dual" { - return true - } - } - return false -} - // removeOptionsWithUnmatchedKeyspace removes all options that don't match // the specified keyspace. It returns false if no such options exist. func (rb *route) removeOptionsWithUnmatchedKeyspace(keyspace string) bool { diff --git a/go/vt/vtgate/planbuilder/testdata/select_cases.txt b/go/vt/vtgate/planbuilder/testdata/select_cases.txt index f89e3c49410..e8885a33a28 100644 --- a/go/vt/vtgate/planbuilder/testdata/select_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/select_cases.txt @@ -392,18 +392,18 @@ "NEXT used on a non-sequence table" # last_insert_id for unsharded route -"select last_insert_id() from unsharded" +"select last_insert_id() from main.dual" { - "Original": "select last_insert_id() from unsharded", + "Original": "select last_insert_id() from main.dual", "Instructions": { "Opcode": "SelectUnsharded", "Keyspace": { "Name": "main", "Sharded": false }, - "Query": "select last_insert_id() from unsharded", - "FieldQuery": "select last_insert_id() from unsharded where 1 != 1", - "Table": "unsharded" + "Query": "select last_insert_id() from dual", + "FieldQuery": "select last_insert_id() from dual where 1 != 1", + "Table": "dual" } } diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt index 8792eeb37af..c10604dfa72 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt @@ -71,10 +71,6 @@ "select last_insert_id() from user" "unsupported: LAST_INSERT_ID is only allowed for unsharded keyspaces" -# last_insert_id for dual table -"select last_insert_id() from dual" -"unsupported: LAST_INSERT_ID is not allowed for dual table" - # natural join "select * from user natural join user_extra" "unsupported: natural join"