From 10b5627948a1f35efbd98fcf8abfef89124c990a Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Thu, 13 Jun 2024 13:44:25 -0400 Subject: [PATCH 1/9] Fix and new unit test Signed-off-by: Matt Lord --- go/vt/vtctl/workflow/traffic_switcher.go | 18 +- go/vt/vtctl/workflow/traffic_switcher_test.go | 159 ++++++++++++++++++ 2 files changed, 176 insertions(+), 1 deletion(-) diff --git a/go/vt/vtctl/workflow/traffic_switcher.go b/go/vt/vtctl/workflow/traffic_switcher.go index 42f097f35b0..2272d8de34f 100644 --- a/go/vt/vtctl/workflow/traffic_switcher.go +++ b/go/vt/vtctl/workflow/traffic_switcher.go @@ -1385,7 +1385,11 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s if kvs == nil || kvs.Sharded || len(kvs.Tables) == 0 { return nil } + var err error for tableName, tableDef := range kvs.Tables { + if tableName, err = sqlescape.UnescapeID(tableName); err != nil { + return err + } select { case <-sctx.Done(): return sctx.Err() @@ -1429,7 +1433,10 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s searchGroup, gctx := errgroup.WithContext(ctx) searchCompleted := make(chan struct{}) for _, keyspace := range keyspaces { - keyspace := keyspace // https://golang.org/doc/faq#closures_and_goroutines + keyspace, err := sqlescape.UnescapeID(keyspace) + if err != nil { + return nil, err + } searchGroup.Go(func() error { return searchKeyspace(gctx, searchCompleted, keyspace) }) @@ -1479,6 +1486,15 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence table name %s defined in the %s keyspace", vs.AutoIncrement.Sequence, ts.targetKeyspace) } + // Unescape the table name and keyspace name as they may be escaped in the + // vschema definition if they e.g. contain dashes. + var err error + if tableName, err = sqlescape.UnescapeID(tableName); err != nil { + return nil, false, err + } + if keyspace, err = sqlescape.UnescapeID(keyspace); err != nil { + return nil, false, err + } sm.backingTableName = tableName sm.backingTableKeyspace = keyspace // Set the default keyspace name. We will later check to diff --git a/go/vt/vtctl/workflow/traffic_switcher_test.go b/go/vt/vtctl/workflow/traffic_switcher_test.go index c416baa18f9..6941fbd2ac9 100644 --- a/go/vt/vtctl/workflow/traffic_switcher_test.go +++ b/go/vt/vtctl/workflow/traffic_switcher_test.go @@ -17,10 +17,17 @@ limitations under the License. package workflow import ( + "context" + "fmt" + "reflect" "testing" + "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "vitess.io/vitess/go/vt/proto/vschema" + "vitess.io/vitess/go/vt/topo" "vitess.io/vitess/go/vt/vtgate/vindexes" ) @@ -56,3 +63,155 @@ func TestReverseWorkflowName(t *testing.T) { assert.Equal(t, test.out, got) } } + +func TestGetTargetSequenceMetadata(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + cell := "cell1" + workflow := "wf1" + table := "t1" + sourceKeyspace := &testKeyspace{ + KeyspaceName: "source-ks", + ShardNames: []string{"0"}, + } + targetKeyspace := &testKeyspace{ + KeyspaceName: "targetks", + ShardNames: []string{"-80", "80-"}, + } + vindexes := map[string]*vschema.Vindex{ + "xxhash": { + Type: "xxhash", + }, + "unicode_loose_xxhash": { + Type: "unicode_loose_xxhash", + }, + } + env := newTestEnv(t, ctx, cell, sourceKeyspace, targetKeyspace) + defer env.close() + /* + env.tmc.schema = map[string]*tabletmanagerdatapb.SchemaDefinition{ + "t1": { + TableDefinitions: []*tabletmanagerdatapb.TableDefinition{ + { + Name: "t1", + Columns: []string{ + "my-col", + }, + }, + }, + }, + } + */ + + type testCase struct { + name string + sourceVSchema *vschema.Keyspace + targetVSchema *vschema.Keyspace + want map[string]*sequenceMetadata + err string + } + tests := []testCase{ + { + name: "no sequences", + want: nil, + }, + { + name: "sequences with backticks all over", + sourceVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + "`my-seq1`": { + Type: "sequence", + }, + }, + }, + targetVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + table: { + ColumnVindexes: []*vschema.ColumnVindex{ + { + Name: "xxhash", + Column: "`my-col`", + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "`my-col`", + Sequence: fmt.Sprintf("`%s`.`my-seq1`", sourceKeyspace.KeyspaceName), + }, + }, + }, + }, + // key: ```my-seq1```, value: &{```my-seq1``` ```source-ks``` vt_`source-ks` t1 vt_targetks 0x14000758640} + // column_vindexes:{column:"`my-col`" name:"xxhash"} auto_increment:{column:"`my-col`" sequence:"`source-ks`.`my-seq1`"} + want: map[string]*sequenceMetadata{ + "my-seq1": { + backingTableName: "my-seq1", + backingTableKeyspace: "source-ks", + backingTableDBName: "vt_source-ks", + usingTableName: table, + usingTableDBName: "vt_targetks", + usingTableDefinition: &vschema.Table{ + ColumnVindexes: []*vschema.ColumnVindex{ + { + Column: "`my-col`", + Name: "xxhash", + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "`my-col`", + Sequence: fmt.Sprintf("`%s`.`my-seq1`", sourceKeyspace.KeyspaceName), + }, + }, + }, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := env.ts.SaveVSchema(ctx, sourceKeyspace.KeyspaceName, tc.sourceVSchema) + require.NoError(t, err) + err = env.ts.SaveVSchema(ctx, targetKeyspace.KeyspaceName, tc.targetVSchema) + require.NoError(t, err) + err = env.ts.RebuildSrvVSchema(ctx, nil) + require.NoError(t, err) + sources := make(map[string]*MigrationSource, len(sourceKeyspace.ShardNames)) + targets := make(map[string]*MigrationTarget, len(targetKeyspace.ShardNames)) + for i, shard := range sourceKeyspace.ShardNames { + tablet := env.tablets[sourceKeyspace.KeyspaceName][startingSourceTabletUID+(i*tabletUIDStep)] + sources[shard] = &MigrationSource{ + primary: &topo.TabletInfo{ + Tablet: tablet, + }, + } + } + for i, shard := range targetKeyspace.ShardNames { + tablet := env.tablets[targetKeyspace.KeyspaceName][startingTargetTabletUID+(i*tabletUIDStep)] + targets[shard] = &MigrationTarget{ + primary: &topo.TabletInfo{ + Tablet: tablet, + }, + } + } + ts := &trafficSwitcher{ + id: 1, + ws: env.ws, + workflow: workflow, + tables: []string{table}, + sourceKeyspace: sourceKeyspace.KeyspaceName, + targetKeyspace: targetKeyspace.KeyspaceName, + sources: sources, + targets: targets, + } + //t.Logf("DEBUG: ts: %+v", ts) + got, err := ts.getTargetSequenceMetadata(ctx) + if tc.err != "" { + require.EqualError(t, err, tc.err) + } else { + require.NoError(t, err) + } + require.True(t, reflect.DeepEqual(tc.want, got), "want: %v, got: %v", tc.want, got) + }) + } +} From c000f0782c2afe2ecdbd50c429ef529177951d74 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Thu, 13 Jun 2024 13:47:50 -0400 Subject: [PATCH 2/9] Update e2e test Signed-off-by: Matt Lord --- go/test/endtoend/vreplication/config_test.go | 2 +- go/vt/vtctl/workflow/traffic_switcher.go | 2 ++ go/vt/vtctl/workflow/traffic_switcher_test.go | 3 --- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/go/test/endtoend/vreplication/config_test.go b/go/test/endtoend/vreplication/config_test.go index a37ebe77b94..e3de0b5ab67 100644 --- a/go/test/endtoend/vreplication/config_test.go +++ b/go/test/endtoend/vreplication/config_test.go @@ -147,7 +147,7 @@ create table nopk (name varchar(128), age int unsigned); ], "auto_increment": { "column": "cid", - "sequence": "customer_seq" + "sequence": "` + "`customer_seq`" + `" } }, "customer_name": { diff --git a/go/vt/vtctl/workflow/traffic_switcher.go b/go/vt/vtctl/workflow/traffic_switcher.go index 2272d8de34f..a5e5086fd23 100644 --- a/go/vt/vtctl/workflow/traffic_switcher.go +++ b/go/vt/vtctl/workflow/traffic_switcher.go @@ -1387,6 +1387,7 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s } var err error for tableName, tableDef := range kvs.Tables { + // The table name can be escaped in the vschema definition. if tableName, err = sqlescape.UnescapeID(tableName); err != nil { return err } @@ -1433,6 +1434,7 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s searchGroup, gctx := errgroup.WithContext(ctx) searchCompleted := make(chan struct{}) for _, keyspace := range keyspaces { + // The keyspace name can be escaped in the vschema definition. keyspace, err := sqlescape.UnescapeID(keyspace) if err != nil { return nil, err diff --git a/go/vt/vtctl/workflow/traffic_switcher_test.go b/go/vt/vtctl/workflow/traffic_switcher_test.go index 6941fbd2ac9..9409edbd32a 100644 --- a/go/vt/vtctl/workflow/traffic_switcher_test.go +++ b/go/vt/vtctl/workflow/traffic_switcher_test.go @@ -142,8 +142,6 @@ func TestGetTargetSequenceMetadata(t *testing.T) { }, }, }, - // key: ```my-seq1```, value: &{```my-seq1``` ```source-ks``` vt_`source-ks` t1 vt_targetks 0x14000758640} - // column_vindexes:{column:"`my-col`" name:"xxhash"} auto_increment:{column:"`my-col`" sequence:"`source-ks`.`my-seq1`"} want: map[string]*sequenceMetadata{ "my-seq1": { backingTableName: "my-seq1", @@ -204,7 +202,6 @@ func TestGetTargetSequenceMetadata(t *testing.T) { sources: sources, targets: targets, } - //t.Logf("DEBUG: ts: %+v", ts) got, err := ts.getTargetSequenceMetadata(ctx) if tc.err != "" { require.EqualError(t, err, tc.err) From b57e3aa7577720051e9f5079d4f8fecec73e9f31 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Thu, 13 Jun 2024 23:29:12 -0400 Subject: [PATCH 3/9] WiP updates Signed-off-by: Matt Lord --- go/test/endtoend/vreplication/config_test.go | 6 +- go/vt/vtctl/workflow/traffic_switcher.go | 114 ++++++++++++++---- go/vt/vtctl/workflow/traffic_switcher_test.go | 77 ++++++++---- go/vt/vttablet/tabletserver/schema/engine.go | 2 +- 4 files changed, 148 insertions(+), 51 deletions(-) diff --git a/go/test/endtoend/vreplication/config_test.go b/go/test/endtoend/vreplication/config_test.go index e3de0b5ab67..eb2a495ba59 100644 --- a/go/test/endtoend/vreplication/config_test.go +++ b/go/test/endtoend/vreplication/config_test.go @@ -147,7 +147,7 @@ create table nopk (name varchar(128), age int unsigned); ], "auto_increment": { "column": "cid", - "sequence": "` + "`customer_seq`" + `" + "sequence": "` + "`customer_seq`" + `" } }, "customer_name": { @@ -295,7 +295,7 @@ create table nopk (name varchar(128), age int unsigned); ], "auto_increment": { "column": "cid", - "sequence": "customer_seq" + "sequence": "` + "`customer_seq`" + `" } }, "orders": { @@ -345,7 +345,7 @@ create table nopk (name varchar(128), age int unsigned); ], "auto_increment": { "column": "cid", - "sequence": "customer_seq" + "sequence": "` + "`customer_seq`" + `" } }, "orders": { diff --git a/go/vt/vtctl/workflow/traffic_switcher.go b/go/vt/vtctl/workflow/traffic_switcher.go index a5e5086fd23..c1faf42ef8c 100644 --- a/go/vt/vtctl/workflow/traffic_switcher.go +++ b/go/vt/vtctl/workflow/traffic_switcher.go @@ -1389,7 +1389,8 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s for tableName, tableDef := range kvs.Tables { // The table name can be escaped in the vschema definition. if tableName, err = sqlescape.UnescapeID(tableName); err != nil { - return err + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid table name in keyspace %s: %v", + keyspace, err) } select { case <-sctx.Done(): @@ -1434,10 +1435,10 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s searchGroup, gctx := errgroup.WithContext(ctx) searchCompleted := make(chan struct{}) for _, keyspace := range keyspaces { - // The keyspace name can be escaped in the vschema definition. + // The keyspace name could be escaped so we need to unescape it. keyspace, err := sqlescape.UnescapeID(keyspace) if err != nil { - return nil, err + return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid keyspace name %s: %v", keyspace, err) } searchGroup.Go(func() error { return searchKeyspace(gctx, searchCompleted, keyspace) @@ -1469,43 +1470,75 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa targetDBName := targets[0].GetPrimary().DbName() sequencesByBackingTable := make(map[string]*sequenceMetadata) - for _, table := range ts.Tables() { - vs, ok := vschema.Tables[table] - if !ok || vs.GetAutoIncrement().GetSequence() == "" { + for _, table := range ts.tables { + seqTable, ok := vschema.Tables[table] + if !ok { + // Try the escaped table name as it can be escaped in the vschema. + seqTable, ok = vschema.Tables[sqlescape.EscapeID(table)] + } + if !ok || seqTable.GetAutoIncrement().GetSequence() == "" { continue } + var err error + // Be sure that the table and DB name is now unescaped. + table, err = sqlescape.UnescapeID(table) + if err != nil { + return nil, false, err + } + targetDBName, err = sqlescape.UnescapeID(targetDBName) + if err != nil { + return nil, false, err + } sm := &sequenceMetadata{ - backingTableName: vs.AutoIncrement.Sequence, - usingTableName: table, - usingTableDefinition: vs, - usingTableDBName: targetDBName, + usingTableName: table, + usingTableDBName: targetDBName, } // If the sequence table is fully qualified in the vschema then // we don't need to find it later. - if strings.Contains(vs.AutoIncrement.Sequence, ".") { - keyspace, tableName, found := strings.Cut(vs.AutoIncrement.Sequence, ".") + if strings.Contains(seqTable.AutoIncrement.Sequence, ".") { + keyspace, tableName, found := strings.Cut(seqTable.AutoIncrement.Sequence, ".") if !found { return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence table name %s defined in the %s keyspace", - vs.AutoIncrement.Sequence, ts.targetKeyspace) + seqTable.AutoIncrement.Sequence, ts.targetKeyspace) } // Unescape the table name and keyspace name as they may be escaped in the // vschema definition if they e.g. contain dashes. - var err error - if tableName, err = sqlescape.UnescapeID(tableName); err != nil { - return nil, false, err - } if keyspace, err = sqlescape.UnescapeID(keyspace); err != nil { - return nil, false, err + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid keyspace in qualified sequence table name %s defined in the %s keyspace", + seqTable.AutoIncrement.Sequence, ts.targetKeyspace) + } + if tableName, err = sqlescape.UnescapeID(tableName); err != nil { + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid qualified sequence table name %s defined in the %s keyspace", + seqTable.AutoIncrement.Sequence, ts.targetKeyspace) } - sm.backingTableName = tableName sm.backingTableKeyspace = keyspace + sm.backingTableName = tableName + // Update the definition with the unescaped values. + seqTable.AutoIncrement.Sequence = fmt.Sprintf("%s.%s", keyspace, tableName) // Set the default keyspace name. We will later check to // see if the tablet we send requests to is using a dbname // override and use that if it is. sm.backingTableDBName = "vt_" + keyspace } else { + sm.backingTableName, err = sqlescape.UnescapeID(seqTable.AutoIncrement.Sequence) + if err != nil { + return nil, false, err + } + seqTable.AutoIncrement.Sequence = sm.backingTableName allFullyQualified = false } + // The column names can be escaped in the vschema definition. + for i := range seqTable.ColumnVindexes { + seqTable.ColumnVindexes[i].Column, err = sqlescape.UnescapeID(seqTable.ColumnVindexes[i].Column) + if err != nil { + return nil, false, err + } + } + seqTable.AutoIncrement.Column, err = sqlescape.UnescapeID(seqTable.AutoIncrement.Column) + if err != nil { + return nil, false, err + } + sm.usingTableDefinition = seqTable sequencesByBackingTable[sm.backingTableName] = sm } @@ -1534,10 +1567,25 @@ func (ts *trafficSwitcher) initializeTargetSequences(ctx context.Context, sequen return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "no primary tablet found for target shard %s/%s", ts.targetKeyspace, target.GetShard().ShardName()) } + usingCol, err := sqlescape.EnsureEscaped(sequenceMetadata.usingTableDefinition.AutoIncrement.Column) + if err != nil { + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid column name %s specified for sequence table %s: %v", + sequenceMetadata.usingTableDefinition.AutoIncrement.Column, sequenceMetadata.usingTableName, err) + } + usingDB, err := sqlescape.EnsureEscaped(sequenceMetadata.usingTableDBName) + if err != nil { + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid database name %s for sequence table %s: %v", + sequenceMetadata.usingTableDBName, sequenceMetadata.usingTableName, err) + } + usingTable, err := sqlescape.EnsureEscaped(sequenceMetadata.usingTableName) + if err != nil { + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence table name %s: %v", + sequenceMetadata.usingTableName, err) + } query := sqlparser.BuildParsedQuery(sqlGetMaxSequenceVal, - sqlescape.EscapeID(sequenceMetadata.usingTableDefinition.AutoIncrement.Column), - sqlescape.EscapeID(sequenceMetadata.usingTableDBName), - sqlescape.EscapeID(sequenceMetadata.usingTableName), + usingCol, + usingDB, + usingTable, ) qr, terr := ts.ws.tmc.ExecuteFetchAsApp(ictx, primary.Tablet, true, &tabletmanagerdatapb.ExecuteFetchAsAppRequest{ Query: []byte(query.Query), @@ -1598,9 +1646,19 @@ func (ts *trafficSwitcher) initializeTargetSequences(ctx context.Context, sequen if sequenceTablet.DbNameOverride != "" { sequenceMetadata.backingTableDBName = sequenceTablet.DbNameOverride } + backingDB, err := sqlescape.EnsureEscaped(sequenceMetadata.backingTableDBName) + if err != nil { + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid database name %s in sequence backing table %s: %v", + sequenceMetadata.backingTableDBName, sequenceMetadata.backingTableName, err) + } + backingTable, err := sqlescape.EnsureEscaped(sequenceMetadata.backingTableName) + if err != nil { + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence backing table name %s: %v", + sequenceMetadata.backingTableName, err) + } query := sqlparser.BuildParsedQuery(sqlInitSequenceTable, - sqlescape.EscapeID(sequenceMetadata.backingTableDBName), - sqlescape.EscapeID(sequenceMetadata.backingTableName), + backingDB, + backingTable, nextVal, nextVal, nextVal, @@ -1633,7 +1691,13 @@ func (ts *trafficSwitcher) initializeTargetSequences(ctx context.Context, sequen return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "failed to get primary tablet for keyspace %s: %v", sequenceMetadata.backingTableKeyspace, ierr) } - ierr = ts.TabletManagerClient().ResetSequences(ictx, ti.Tablet, []string{sequenceMetadata.backingTableName}) + // ResetSequences interfaces with the schema engine and the actual + // table identifiers DO NOT contain the backticks. So we have to + // ensure that the table name is unescaped. + if backingTable, err = sqlescape.UnescapeID(backingTable); err != nil { + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid table name %s: %v", backingTable, err) + } + ierr = ts.TabletManagerClient().ResetSequences(ictx, ti.Tablet, []string{backingTable}) if ierr != nil { return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "failed to reset the sequence cache for backing table %s on shard %s/%s using tablet %s: %v", sequenceMetadata.backingTableName, sequenceShard.Keyspace(), sequenceShard.ShardName(), sequenceShard.PrimaryAlias, ierr) diff --git a/go/vt/vtctl/workflow/traffic_switcher_test.go b/go/vt/vtctl/workflow/traffic_switcher_test.go index 9409edbd32a..d7b67d981af 100644 --- a/go/vt/vtctl/workflow/traffic_switcher_test.go +++ b/go/vt/vtctl/workflow/traffic_switcher_test.go @@ -69,7 +69,8 @@ func TestGetTargetSequenceMetadata(t *testing.T) { defer cancel() cell := "cell1" workflow := "wf1" - table := "t1" + table := "`t1`" + unescapedTable := "t1" sourceKeyspace := &testKeyspace{ KeyspaceName: "source-ks", ShardNames: []string{"0"}, @@ -82,26 +83,9 @@ func TestGetTargetSequenceMetadata(t *testing.T) { "xxhash": { Type: "xxhash", }, - "unicode_loose_xxhash": { - Type: "unicode_loose_xxhash", - }, } env := newTestEnv(t, ctx, cell, sourceKeyspace, targetKeyspace) defer env.close() - /* - env.tmc.schema = map[string]*tabletmanagerdatapb.SchemaDefinition{ - "t1": { - TableDefinitions: []*tabletmanagerdatapb.TableDefinition{ - { - Name: "t1", - Columns: []string{ - "my-col", - }, - }, - }, - }, - } - */ type testCase struct { name string @@ -116,7 +100,7 @@ func TestGetTargetSequenceMetadata(t *testing.T) { want: nil, }, { - name: "sequences with backticks all over", + name: "sequences with backticks and qualified table", sourceVSchema: &vschema.Keyspace{ Vindexes: vindexes, Tables: map[string]*vschema.Table{ @@ -147,18 +131,67 @@ func TestGetTargetSequenceMetadata(t *testing.T) { backingTableName: "my-seq1", backingTableKeyspace: "source-ks", backingTableDBName: "vt_source-ks", - usingTableName: table, + usingTableName: unescapedTable, usingTableDBName: "vt_targetks", usingTableDefinition: &vschema.Table{ ColumnVindexes: []*vschema.ColumnVindex{ { - Column: "`my-col`", + Column: "my-col", Name: "xxhash", }, }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "my-col", + Sequence: fmt.Sprintf("%s.my-seq1", sourceKeyspace.KeyspaceName), + }, + }, + }, + }, + }, + { + name: "sequences with backticks", + sourceVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + "`my-seq1`": { + Type: "sequence", + }, + }, + }, + targetVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + table: { + ColumnVindexes: []*vschema.ColumnVindex{ + { + Name: "xxhash", + Column: "`my-col`", + }, + }, AutoIncrement: &vschema.AutoIncrement{ Column: "`my-col`", - Sequence: fmt.Sprintf("`%s`.`my-seq1`", sourceKeyspace.KeyspaceName), + Sequence: "`my-seq1`", + }, + }, + }, + }, + want: map[string]*sequenceMetadata{ + "my-seq1": { + backingTableName: "my-seq1", + backingTableKeyspace: "source-ks", + backingTableDBName: "vt_source-ks", + usingTableName: unescapedTable, + usingTableDBName: "vt_targetks", + usingTableDefinition: &vschema.Table{ + ColumnVindexes: []*vschema.ColumnVindex{ + { + Column: "my-col", + Name: "xxhash", + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "my-col", + Sequence: "my-seq1", }, }, }, diff --git a/go/vt/vttablet/tabletserver/schema/engine.go b/go/vt/vttablet/tabletserver/schema/engine.go index da3d1e999e1..5babed271ca 100644 --- a/go/vt/vttablet/tabletserver/schema/engine.go +++ b/go/vt/vttablet/tabletserver/schema/engine.go @@ -919,7 +919,7 @@ func (se *Engine) ResetSequences(tables []string) error { for _, tableName := range tables { if table, ok := se.tables[tableName]; ok { if table.SequenceInfo != nil { - log.Infof("Resetting sequence info for table %v: %s", tableName, table.SequenceInfo) + log.Infof("Resetting sequence info for table %s: %+v", tableName, table.SequenceInfo) table.SequenceInfo.Reset() } } else { From 8bce07c6d992f8d25e0618757bda5a2fdc0253ee Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 14 Jun 2024 13:19:24 -0400 Subject: [PATCH 4/9] Improve errors and move more endtoend tests to vtctldclient only Signed-off-by: Matt Lord --- go/test/endtoend/vreplication/fk_ext_test.go | 4 +- go/test/endtoend/vreplication/fk_test.go | 2 +- .../vreplication/partial_movetables_test.go | 8 +-- .../vreplication/vreplication_test.go | 15 +++-- go/vt/vtctl/workflow/traffic_switcher.go | 61 +++++++++---------- 5 files changed, 42 insertions(+), 48 deletions(-) diff --git a/go/test/endtoend/vreplication/fk_ext_test.go b/go/test/endtoend/vreplication/fk_ext_test.go index 4e493da5baf..e17247ab46b 100644 --- a/go/test/endtoend/vreplication/fk_ext_test.go +++ b/go/test/endtoend/vreplication/fk_ext_test.go @@ -248,7 +248,7 @@ func doReshard(t *testing.T, keyspace, workflowName, sourceShards, targetShards sourceShards: sourceShards, targetShards: targetShards, skipSchemaCopy: true, - }, workflowFlavorVtctl) + }, workflowFlavorVtctld) rs.Create() waitForWorkflowState(t, vc, fmt.Sprintf("%s.%s", keyspace, workflowName), binlogdatapb.VReplicationWorkflowState_Running.String()) for _, targetTab := range targetTabs { @@ -355,7 +355,7 @@ func doMoveTables(t *testing.T, sourceKeyspace, targetKeyspace, workflowName, ta }, sourceKeyspace: sourceKeyspace, atomicCopy: atomicCopy, - }, workflowFlavorRandom) + }, workflowFlavorVtctld) mt.Create() waitForWorkflowState(t, vc, fmt.Sprintf("%s.%s", targetKeyspace, workflowName), binlogdatapb.VReplicationWorkflowState_Running.String()) diff --git a/go/test/endtoend/vreplication/fk_test.go b/go/test/endtoend/vreplication/fk_test.go index 09692930c5c..72cd278002f 100644 --- a/go/test/endtoend/vreplication/fk_test.go +++ b/go/test/endtoend/vreplication/fk_test.go @@ -34,7 +34,7 @@ import ( binlogdatapb "vitess.io/vitess/go/vt/proto/binlogdata" ) -const testWorkflowFlavor = workflowFlavorRandom +const testWorkflowFlavor = workflowFlavorVtctld // TestFKWorkflow runs a MoveTables workflow with atomic copy for a db with foreign key constraints. // It inserts initial data, then simulates load. We insert both child rows with foreign keys and those without, diff --git a/go/test/endtoend/vreplication/partial_movetables_test.go b/go/test/endtoend/vreplication/partial_movetables_test.go index 4236bff95a3..b971a05a467 100644 --- a/go/test/endtoend/vreplication/partial_movetables_test.go +++ b/go/test/endtoend/vreplication/partial_movetables_test.go @@ -53,7 +53,7 @@ func testCancel(t *testing.T) { sourceKeyspace: sourceKeyspace, tables: table, sourceShards: shard, - }, workflowFlavorRandom) + }, workflowFlavorVtctld) mt.Create() checkDenyList := func(keyspace string, expected bool) { @@ -390,9 +390,5 @@ func testPartialMoveTablesBasic(t *testing.T, flavor workflowFlavor) { // We test with both the vtctlclient and vtctldclient flavors. func TestPartialMoveTablesBasic(t *testing.T) { currentWorkflowType = binlogdatapb.VReplicationWorkflowType_MoveTables - for _, flavor := range workflowFlavors { - t.Run(workflowFlavorNames[flavor], func(t *testing.T) { - testPartialMoveTablesBasic(t, flavor) - }) - } + testPartialMoveTablesBasic(t, workflowFlavorVtctld) } diff --git a/go/test/endtoend/vreplication/vreplication_test.go b/go/test/endtoend/vreplication/vreplication_test.go index db58f2880c2..52874b5839c 100644 --- a/go/test/endtoend/vreplication/vreplication_test.go +++ b/go/test/endtoend/vreplication/vreplication_test.go @@ -1563,17 +1563,16 @@ func switchWrites(t *testing.T, workflowType, ksWorkflow string, reverse bool) { } const SwitchWritesTimeout = "91s" // max: 3 tablet picker 30s waits + 1 ensureCanSwitch(t, workflowType, "", ksWorkflow) - // Use vtctldclient for MoveTables SwitchTraffic ~ 50% of the time. - if workflowType == binlogdatapb.VReplicationWorkflowType_MoveTables.String() && time.Now().Second()%2 == 0 { - parts := strings.Split(ksWorkflow, ".") - require.Equal(t, 2, len(parts)) - moveTablesAction(t, command, defaultCellName, parts[1], sourceKs, parts[0], "", "--timeout="+SwitchWritesTimeout, "--tablet-types=primary") + targetKs, workflow, found := strings.Cut(ksWorkflow, ".") + require.True(t, found) + if workflowType == binlogdatapb.VReplicationWorkflowType_MoveTables.String() { + moveTablesAction(t, command, defaultCellName, workflow, sourceKs, targetKs, "", "--timeout="+SwitchWritesTimeout, "--tablet-types=primary") return } - output, err := vc.VtctlClient.ExecuteCommandWithOutput(workflowType, "--", "--tablet_types=primary", - "--timeout="+SwitchWritesTimeout, "--initialize-target-sequences", command, ksWorkflow) + output, err := vc.VtctldClient.ExecuteCommandWithOutput(workflowType, "--tablet-types=primary", "--workflow", workflow, + "--target-keyspace", targetKs, command, "--timeout="+SwitchWritesTimeout, "--initialize-target-sequences") if output != "" { - fmt.Printf("Output of switching writes with vtctlclient for %s:\n++++++\n%s\n--------\n", ksWorkflow, output) + fmt.Printf("Output of switching writes with vtctldclient for %s:\n++++++\n%s\n--------\n", ksWorkflow, output) } // printSwitchWritesExtraDebug is useful when debugging failures in Switch writes due to corner cases/races _ = printSwitchWritesExtraDebug diff --git a/go/vt/vtctl/workflow/traffic_switcher.go b/go/vt/vtctl/workflow/traffic_switcher.go index c1faf42ef8c..d1309eb0c72 100644 --- a/go/vt/vtctl/workflow/traffic_switcher.go +++ b/go/vt/vtctl/workflow/traffic_switcher.go @@ -1385,12 +1385,12 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s if kvs == nil || kvs.Sharded || len(kvs.Tables) == 0 { return nil } - var err error for tableName, tableDef := range kvs.Tables { // The table name can be escaped in the vschema definition. - if tableName, err = sqlescape.UnescapeID(tableName); err != nil { - return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid table name in keyspace %s: %v", - keyspace, err) + escapedTableName, err := sqlescape.UnescapeID(tableName) + if err != nil { + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid table name %s in keyspace %s: %v", + tableName, keyspace, err) } select { case <-sctx.Done(): @@ -1402,9 +1402,10 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s if complete := func() bool { smMu.Lock() // Prevent concurrent access to the map defer smMu.Unlock() - sm := sequencesByBackingTable[tableName] + sm := sequencesByBackingTable[escapedTableName] + log.Errorf("DEBUG: sequence table: %s", escapedTableName) if tableDef != nil && tableDef.Type == vindexes.TypeSequence && - sm != nil && tableName == sm.backingTableName { + sm != nil && escapedTableName == sm.backingTableName { tablesFound++ // This is also protected by the mutex sm.backingTableKeyspace = keyspace // Set the default keyspace name. We will later check to @@ -1434,14 +1435,15 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s } searchGroup, gctx := errgroup.WithContext(ctx) searchCompleted := make(chan struct{}) + log.Errorf("DEBUG: sequences: %s", strings.Join(maps.Keys(sequencesByBackingTable), ",")) for _, keyspace := range keyspaces { // The keyspace name could be escaped so we need to unescape it. - keyspace, err := sqlescape.UnescapeID(keyspace) + ks, err := sqlescape.UnescapeID(keyspace) if err != nil { return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid keyspace name %s: %v", keyspace, err) } searchGroup.Go(func() error { - return searchKeyspace(gctx, searchCompleted, keyspace) + return searchKeyspace(gctx, searchCompleted, ks) }) } if err := searchGroup.Wait(); err != nil { @@ -1449,8 +1451,8 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s } if tablesFound != tableCount { - return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "failed to locate all of the backing sequence tables being used; sequence table metadata: %+v", - sequencesByBackingTable) + return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "failed to locate all of the backing sequence tables being used; sequence tables: %s", + strings.Join(maps.Keys(sequencesByBackingTable), ",")) } return sequencesByBackingTable, nil } @@ -1472,25 +1474,18 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa for _, table := range ts.tables { seqTable, ok := vschema.Tables[table] - if !ok { - // Try the escaped table name as it can be escaped in the vschema. - seqTable, ok = vschema.Tables[sqlescape.EscapeID(table)] - } if !ok || seqTable.GetAutoIncrement().GetSequence() == "" { continue } - var err error - // Be sure that the table and DB name is now unescaped. - table, err = sqlescape.UnescapeID(table) - if err != nil { - return nil, false, err - } - targetDBName, err = sqlescape.UnescapeID(targetDBName) + // Be sure that the table name is unescaped as it can be escaped + // in the vschema. + escapedTable, err := sqlescape.UnescapeID(table) if err != nil { - return nil, false, err + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid table name %s defined in the sequence table %+v: %v", + table, seqTable, err) } sm := &sequenceMetadata{ - usingTableName: table, + usingTableName: escapedTable, usingTableDBName: targetDBName, } // If the sequence table is fully qualified in the vschema then @@ -1504,12 +1499,12 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa // Unescape the table name and keyspace name as they may be escaped in the // vschema definition if they e.g. contain dashes. if keyspace, err = sqlescape.UnescapeID(keyspace); err != nil { - return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid keyspace in qualified sequence table name %s defined in the %s keyspace", - seqTable.AutoIncrement.Sequence, ts.targetKeyspace) + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid keyspace in qualified sequence table name %s defined in sequence table %+v: %v", + seqTable.AutoIncrement.Sequence, seqTable, err) } if tableName, err = sqlescape.UnescapeID(tableName); err != nil { - return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid qualified sequence table name %s defined in the %s keyspace", - seqTable.AutoIncrement.Sequence, ts.targetKeyspace) + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid qualified sequence table name %s defined in sequence table %+v: %v", + seqTable.AutoIncrement.Sequence, seqTable, err) } sm.backingTableKeyspace = keyspace sm.backingTableName = tableName @@ -1529,15 +1524,19 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa } // The column names can be escaped in the vschema definition. for i := range seqTable.ColumnVindexes { - seqTable.ColumnVindexes[i].Column, err = sqlescape.UnescapeID(seqTable.ColumnVindexes[i].Column) + escapedColumn, err := sqlescape.UnescapeID(seqTable.ColumnVindexes[i].Column) if err != nil { - return nil, false, err + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence column vindex name %s defined in sequence table %+v: %v", + seqTable.ColumnVindexes[i].Column, seqTable, err) } + seqTable.ColumnVindexes[i].Column = escapedColumn } - seqTable.AutoIncrement.Column, err = sqlescape.UnescapeID(seqTable.AutoIncrement.Column) + escapedAutoIncCol, err := sqlescape.UnescapeID(seqTable.AutoIncrement.Column) if err != nil { - return nil, false, err + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid auto-increment column name %s defined in sequence table %+v: %v", + seqTable.AutoIncrement.Column, seqTable, err) } + seqTable.AutoIncrement.Column = escapedAutoIncCol sm.usingTableDefinition = seqTable sequencesByBackingTable[sm.backingTableName] = sm } From 93ed704084b576ae362fe052942911485983cb06 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 14 Jun 2024 14:57:59 -0400 Subject: [PATCH 5/9] Escape backing table name in vschema as well Signed-off-by: Matt Lord --- go/test/endtoend/vreplication/config_test.go | 2 +- go/vt/vtctl/workflow/traffic_switcher.go | 15 +++++++-------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/go/test/endtoend/vreplication/config_test.go b/go/test/endtoend/vreplication/config_test.go index eb2a495ba59..cd2e10768ca 100644 --- a/go/test/endtoend/vreplication/config_test.go +++ b/go/test/endtoend/vreplication/config_test.go @@ -88,7 +88,7 @@ create table nopk (name varchar(128), age int unsigned); "orders": {}, "loadtest": {}, "customer": {}, - "customer_seq": { + "` + "`customer_seq`" + `": { "type": "sequence" }, "customer2": {}, diff --git a/go/vt/vtctl/workflow/traffic_switcher.go b/go/vt/vtctl/workflow/traffic_switcher.go index d1309eb0c72..ed8a2ba9d97 100644 --- a/go/vt/vtctl/workflow/traffic_switcher.go +++ b/go/vt/vtctl/workflow/traffic_switcher.go @@ -1403,7 +1403,6 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s smMu.Lock() // Prevent concurrent access to the map defer smMu.Unlock() sm := sequencesByBackingTable[escapedTableName] - log.Errorf("DEBUG: sequence table: %s", escapedTableName) if tableDef != nil && tableDef.Type == vindexes.TypeSequence && sm != nil && escapedTableName == sm.backingTableName { tablesFound++ // This is also protected by the mutex @@ -1435,11 +1434,10 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s } searchGroup, gctx := errgroup.WithContext(ctx) searchCompleted := make(chan struct{}) - log.Errorf("DEBUG: sequences: %s", strings.Join(maps.Keys(sequencesByBackingTable), ",")) for _, keyspace := range keyspaces { // The keyspace name could be escaped so we need to unescape it. ks, err := sqlescape.UnescapeID(keyspace) - if err != nil { + if err != nil { // Should never happen return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid keyspace name %s: %v", keyspace, err) } searchGroup.Go(func() error { @@ -1451,7 +1449,7 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s } if tablesFound != tableCount { - return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "failed to locate all of the backing sequence tables being used; sequence tables: %s", + return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "failed to locate all of the backing sequence tables being used: %s", strings.Join(maps.Keys(sequencesByBackingTable), ",")) } return sequencesByBackingTable, nil @@ -1517,7 +1515,8 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa } else { sm.backingTableName, err = sqlescape.UnescapeID(seqTable.AutoIncrement.Sequence) if err != nil { - return nil, false, err + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence table name %s defined in sequence table %+v: %v", + seqTable.AutoIncrement.Sequence, seqTable, err) } seqTable.AutoIncrement.Sequence = sm.backingTableName allFullyQualified = false @@ -1568,17 +1567,17 @@ func (ts *trafficSwitcher) initializeTargetSequences(ctx context.Context, sequen } usingCol, err := sqlescape.EnsureEscaped(sequenceMetadata.usingTableDefinition.AutoIncrement.Column) if err != nil { - return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid column name %s specified for sequence table %s: %v", + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid column name %s specified for sequence in table %s: %v", sequenceMetadata.usingTableDefinition.AutoIncrement.Column, sequenceMetadata.usingTableName, err) } usingDB, err := sqlescape.EnsureEscaped(sequenceMetadata.usingTableDBName) if err != nil { - return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid database name %s for sequence table %s: %v", + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid database name %s specified for sequence in table %s: %v", sequenceMetadata.usingTableDBName, sequenceMetadata.usingTableName, err) } usingTable, err := sqlescape.EnsureEscaped(sequenceMetadata.usingTableName) if err != nil { - return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence table name %s: %v", + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence table name specified for sequence in table %s: %v", sequenceMetadata.usingTableName, err) } query := sqlparser.BuildParsedQuery(sqlGetMaxSequenceVal, From 1c16692e780c5b398bddda431e6a118fc0d76c8c Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 14 Jun 2024 15:24:07 -0400 Subject: [PATCH 6/9] Minor tweaks Signed-off-by: Matt Lord --- go/test/endtoend/vreplication/config_test.go | 4 ++-- go/test/endtoend/vreplication/vdiff2_test.go | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/go/test/endtoend/vreplication/config_test.go b/go/test/endtoend/vreplication/config_test.go index cd2e10768ca..25a4b734259 100644 --- a/go/test/endtoend/vreplication/config_test.go +++ b/go/test/endtoend/vreplication/config_test.go @@ -88,7 +88,7 @@ create table nopk (name varchar(128), age int unsigned); "orders": {}, "loadtest": {}, "customer": {}, - "` + "`customer_seq`" + `": { + "customer_seq": { "type": "sequence" }, "customer2": {}, @@ -295,7 +295,7 @@ create table nopk (name varchar(128), age int unsigned); ], "auto_increment": { "column": "cid", - "sequence": "` + "`customer_seq`" + `" + "sequence": "` + "`product`.`customer_seq`" + `" } }, "orders": { diff --git a/go/test/endtoend/vreplication/vdiff2_test.go b/go/test/endtoend/vreplication/vdiff2_test.go index fb8ed7c8787..f4128b5c036 100644 --- a/go/test/endtoend/vreplication/vdiff2_test.go +++ b/go/test/endtoend/vreplication/vdiff2_test.go @@ -176,6 +176,7 @@ func TestVDiff2(t *testing.T) { // We ONLY add primary tablets in this test. tks, err := vc.AddKeyspace(t, []*Cell{zone3, zone1, zone2}, targetKs, strings.Join(targetShards, ","), customerVSchema, customerSchema, 0, 0, 200, targetKsOpts) require.NoError(t, err) + verifyClusterHealth(t, vc) for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { From 9b09c6ffbb4f6bb90eefd7ebac0aff1f4462cddb Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 14 Jun 2024 15:48:03 -0400 Subject: [PATCH 7/9] Extend unit_test_race timeout too I had previously extended the unit test timeout and in the interim we had split the unit test race out so it had the old timeout. Signed-off-by: Matt Lord --- tools/unit_test_race.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/unit_test_race.sh b/tools/unit_test_race.sh index 3b6a137edf1..86fbcbcf995 100755 --- a/tools/unit_test_race.sh +++ b/tools/unit_test_race.sh @@ -54,7 +54,7 @@ for pkg in $flaky_tests; do max_attempts=3 attempt=1 # Set a timeout because some tests may deadlock when they flake. - until go test -timeout 2m $VT_GO_PARALLEL $pkg -v -race -count=1; do + until go test -timeout 5m $VT_GO_PARALLEL $pkg -v -race -count=1; do echo "FAILED (try $attempt/$max_attempts) in $pkg (return code $?). See above for errors." if [ $((++attempt)) -gt $max_attempts ]; then echo "ERROR: Flaky Go unit tests in package $pkg failed too often (after $max_attempts retries). Please reduce the flakiness." From 28284afd142cdd251fe682198015cf5cb7b0f0c6 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 14 Jun 2024 15:57:37 -0400 Subject: [PATCH 8/9] Minor change on self review Signed-off-by: Matt Lord --- go/vt/vtctl/workflow/traffic_switcher.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/go/vt/vtctl/workflow/traffic_switcher.go b/go/vt/vtctl/workflow/traffic_switcher.go index ed8a2ba9d97..b656aa7e97e 100644 --- a/go/vt/vtctl/workflow/traffic_switcher.go +++ b/go/vt/vtctl/workflow/traffic_switcher.go @@ -1692,10 +1692,11 @@ func (ts *trafficSwitcher) initializeTargetSequences(ctx context.Context, sequen // ResetSequences interfaces with the schema engine and the actual // table identifiers DO NOT contain the backticks. So we have to // ensure that the table name is unescaped. - if backingTable, err = sqlescape.UnescapeID(backingTable); err != nil { - return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid table name %s: %v", backingTable, err) + escapedBackingTable, err := sqlescape.UnescapeID(backingTable) + if err != nil { + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence backing table name %s: %v", backingTable, err) } - ierr = ts.TabletManagerClient().ResetSequences(ictx, ti.Tablet, []string{backingTable}) + ierr = ts.TabletManagerClient().ResetSequences(ictx, ti.Tablet, []string{escapedBackingTable}) if ierr != nil { return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "failed to reset the sequence cache for backing table %s on shard %s/%s using tablet %s: %v", sequenceMetadata.backingTableName, sequenceShard.Keyspace(), sequenceShard.ShardName(), sequenceShard.PrimaryAlias, ierr) From 990cc6a32a4a68f768ed13df091de8315bedf4b3 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Sun, 16 Jun 2024 00:13:55 -0400 Subject: [PATCH 9/9] Correct variable names and cover errors in unit test Signed-off-by: Matt Lord --- go/vt/vtctl/workflow/traffic_switcher.go | 22 ++-- go/vt/vtctl/workflow/traffic_switcher_test.go | 116 ++++++++++++++++++ 2 files changed, 127 insertions(+), 11 deletions(-) diff --git a/go/vt/vtctl/workflow/traffic_switcher.go b/go/vt/vtctl/workflow/traffic_switcher.go index b656aa7e97e..7511315af15 100644 --- a/go/vt/vtctl/workflow/traffic_switcher.go +++ b/go/vt/vtctl/workflow/traffic_switcher.go @@ -1387,7 +1387,7 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s } for tableName, tableDef := range kvs.Tables { // The table name can be escaped in the vschema definition. - escapedTableName, err := sqlescape.UnescapeID(tableName) + unescapedTableName, err := sqlescape.UnescapeID(tableName) if err != nil { return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid table name %s in keyspace %s: %v", tableName, keyspace, err) @@ -1402,9 +1402,9 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s if complete := func() bool { smMu.Lock() // Prevent concurrent access to the map defer smMu.Unlock() - sm := sequencesByBackingTable[escapedTableName] + sm := sequencesByBackingTable[unescapedTableName] if tableDef != nil && tableDef.Type == vindexes.TypeSequence && - sm != nil && escapedTableName == sm.backingTableName { + sm != nil && unescapedTableName == sm.backingTableName { tablesFound++ // This is also protected by the mutex sm.backingTableKeyspace = keyspace // Set the default keyspace name. We will later check to @@ -1477,13 +1477,13 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa } // Be sure that the table name is unescaped as it can be escaped // in the vschema. - escapedTable, err := sqlescape.UnescapeID(table) + unescapedTable, err := sqlescape.UnescapeID(table) if err != nil { return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid table name %s defined in the sequence table %+v: %v", table, seqTable, err) } sm := &sequenceMetadata{ - usingTableName: escapedTable, + usingTableName: unescapedTable, usingTableDBName: targetDBName, } // If the sequence table is fully qualified in the vschema then @@ -1523,19 +1523,19 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa } // The column names can be escaped in the vschema definition. for i := range seqTable.ColumnVindexes { - escapedColumn, err := sqlescape.UnescapeID(seqTable.ColumnVindexes[i].Column) + unescapedColumn, err := sqlescape.UnescapeID(seqTable.ColumnVindexes[i].Column) if err != nil { return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence column vindex name %s defined in sequence table %+v: %v", seqTable.ColumnVindexes[i].Column, seqTable, err) } - seqTable.ColumnVindexes[i].Column = escapedColumn + seqTable.ColumnVindexes[i].Column = unescapedColumn } - escapedAutoIncCol, err := sqlescape.UnescapeID(seqTable.AutoIncrement.Column) + unescapedAutoIncCol, err := sqlescape.UnescapeID(seqTable.AutoIncrement.Column) if err != nil { return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid auto-increment column name %s defined in sequence table %+v: %v", seqTable.AutoIncrement.Column, seqTable, err) } - seqTable.AutoIncrement.Column = escapedAutoIncCol + seqTable.AutoIncrement.Column = unescapedAutoIncCol sm.usingTableDefinition = seqTable sequencesByBackingTable[sm.backingTableName] = sm } @@ -1692,11 +1692,11 @@ func (ts *trafficSwitcher) initializeTargetSequences(ctx context.Context, sequen // ResetSequences interfaces with the schema engine and the actual // table identifiers DO NOT contain the backticks. So we have to // ensure that the table name is unescaped. - escapedBackingTable, err := sqlescape.UnescapeID(backingTable) + unescapedBackingTable, err := sqlescape.UnescapeID(backingTable) if err != nil { return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence backing table name %s: %v", backingTable, err) } - ierr = ts.TabletManagerClient().ResetSequences(ictx, ti.Tablet, []string{escapedBackingTable}) + ierr = ts.TabletManagerClient().ResetSequences(ictx, ti.Tablet, []string{unescapedBackingTable}) if ierr != nil { return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "failed to reset the sequence cache for backing table %s on shard %s/%s using tablet %s: %v", sequenceMetadata.backingTableName, sequenceShard.Keyspace(), sequenceShard.ShardName(), sequenceShard.PrimaryAlias, ierr) diff --git a/go/vt/vtctl/workflow/traffic_switcher_test.go b/go/vt/vtctl/workflow/traffic_switcher_test.go index d7b67d981af..5c0b2aba682 100644 --- a/go/vt/vtctl/workflow/traffic_switcher_test.go +++ b/go/vt/vtctl/workflow/traffic_switcher_test.go @@ -197,6 +197,122 @@ func TestGetTargetSequenceMetadata(t *testing.T) { }, }, }, + { + name: "invalid table name", + sourceVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + "`my-`seq1`": { + Type: "sequence", + }, + }, + }, + targetVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + table: { + ColumnVindexes: []*vschema.ColumnVindex{ + { + Name: "xxhash", + Column: "`my-col`", + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "`my-col`", + Sequence: "`my-seq1`", + }, + }, + }, + }, + err: "invalid table name `my-`seq1` in keyspace source-ks: UnescapeID err: unexpected single backtick at position 3 in 'my-`seq1'", + }, + { + name: "invalid keyspace name", + sourceVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + "`my-seq1`": { + Type: "sequence", + }, + }, + }, + targetVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + table: { + ColumnVindexes: []*vschema.ColumnVindex{ + { + Name: "xxhash", + Column: "`my-col`", + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "`my-col`", + Sequence: "`ks`1`.`my-seq1`", + }, + }, + }, + }, + err: "invalid keyspace in qualified sequence table name `ks`1`.`my-seq1` defined in sequence table column_vindexes:{column:\"`my-col`\" name:\"xxhash\"} auto_increment:{column:\"`my-col`\" sequence:\"`ks`1`.`my-seq1`\"}: UnescapeID err: unexpected single backtick at position 2 in 'ks`1'", + }, + { + name: "invalid auto-inc column name", + sourceVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + "`my-seq1`": { + Type: "sequence", + }, + }, + }, + targetVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + table: { + ColumnVindexes: []*vschema.ColumnVindex{ + { + Name: "xxhash", + Column: "`my-col`", + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "`my`-col`", + Sequence: "`my-seq1`", + }, + }, + }, + }, + err: "invalid auto-increment column name `my`-col` defined in sequence table column_vindexes:{column:\"my-col\" name:\"xxhash\"} auto_increment:{column:\"`my`-col`\" sequence:\"my-seq1\"}: UnescapeID err: unexpected single backtick at position 2 in 'my`-col'", + }, + { + name: "invalid sequence name", + sourceVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + "`my-seq1`": { + Type: "sequence", + }, + }, + }, + targetVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + table: { + ColumnVindexes: []*vschema.ColumnVindex{ + { + Name: "xxhash", + Column: "`my-col`", + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "`my-col`", + Sequence: "`my-`seq1`", + }, + }, + }, + }, + err: "invalid sequence table name `my-`seq1` defined in sequence table column_vindexes:{column:\"`my-col`\" name:\"xxhash\"} auto_increment:{column:\"`my-col`\" sequence:\"`my-`seq1`\"}: UnescapeID err: unexpected single backtick at position 3 in 'my-`seq1'", + }, } for _, tc := range tests {