From 1f7d1733f3ffa5d0a7bf6f425af054b1be23305c Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 27 Sep 2024 17:23:24 -0400 Subject: [PATCH 1/3] Support both vindex col def formats when initing target sequences Signed-off-by: Matt Lord --- go/vt/vtctl/workflow/traffic_switcher.go | 25 ++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/go/vt/vtctl/workflow/traffic_switcher.go b/go/vt/vtctl/workflow/traffic_switcher.go index 0dab2e159c2..1dd034544c5 100644 --- a/go/vt/vtctl/workflow/traffic_switcher.go +++ b/go/vt/vtctl/workflow/traffic_switcher.go @@ -1521,7 +1521,7 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa // in the vschema. 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", + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid table name %q defined in the sequence table %+v: %v", table, seqTable, err) } sm := &sequenceMetadata{ @@ -1533,17 +1533,17 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa 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", + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence table name %q defined in the %q keyspace", 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. 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 sequence table %+v: %v", + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid keyspace in qualified sequence table name %q 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 sequence table %+v: %v", + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid qualified sequence table name %q defined in sequence table %+v: %v", seqTable.AutoIncrement.Sequence, seqTable, err) } sm.backingTableKeyspace = keyspace @@ -1557,7 +1557,7 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa } else { sm.backingTableName, err = sqlescape.UnescapeID(seqTable.AutoIncrement.Sequence) if err != nil { - return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence table name %s defined in sequence table %+v: %v", + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence table name %q defined in sequence table %+v: %v", seqTable.AutoIncrement.Sequence, seqTable, err) } seqTable.AutoIncrement.Sequence = sm.backingTableName @@ -1565,16 +1565,25 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa } // The column names can be escaped in the vschema definition. for i := range seqTable.ColumnVindexes { - unescapedColumn, err := sqlescape.UnescapeID(seqTable.ColumnVindexes[i].Column) + var ( + unescapedColumn string + err error + ) + if len(seqTable.ColumnVindexes[i].Columns) > 0 { + unescapedColumn, err = sqlescape.UnescapeID(seqTable.ColumnVindexes[i].Columns[0]) // AutoIncrement definitions can only be on a single column + } else { + // This is the legacy vschema definition. + 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", + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence column vindex name %q defined in sequence table %+v: %v", seqTable.ColumnVindexes[i].Column, seqTable, err) } seqTable.ColumnVindexes[i].Column = unescapedColumn } 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", + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid auto-increment column name %q defined in sequence table %+v: %v", seqTable.AutoIncrement.Column, seqTable, err) } seqTable.AutoIncrement.Column = unescapedAutoIncCol From e536072bff3d9ec21e5a5521c1bd17b0d4429a56 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 27 Sep 2024 17:59:35 -0400 Subject: [PATCH 2/3] Add unit test Signed-off-by: Matt Lord --- go/vt/vtctl/workflow/traffic_switcher.go | 9 +- go/vt/vtctl/workflow/traffic_switcher_test.go | 97 +++++++++++++++++-- 2 files changed, 95 insertions(+), 11 deletions(-) diff --git a/go/vt/vtctl/workflow/traffic_switcher.go b/go/vt/vtctl/workflow/traffic_switcher.go index 1dd034544c5..b762ad42f5f 100644 --- a/go/vt/vtctl/workflow/traffic_switcher.go +++ b/go/vt/vtctl/workflow/traffic_switcher.go @@ -1431,7 +1431,7 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s // The table name can be escaped in the vschema definition. unescapedTableName, err := sqlescape.UnescapeID(tableName) if err != nil { - return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid table name %s in keyspace %s: %v", + return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid table name %q in keyspace %s: %v", tableName, keyspace, err) } select { @@ -1480,7 +1480,7 @@ func (ts *trafficSwitcher) getTargetSequenceMetadata(ctx context.Context) (map[s // The keyspace name could be escaped so we need to unescape it. ks, err := sqlescape.UnescapeID(keyspace) if err != nil { // Should never happen - return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid keyspace name %s: %v", keyspace, err) + return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid keyspace name %q: %v", keyspace, err) } searchGroup.Go(func() error { return searchKeyspace(gctx, searchCompleted, ks) @@ -1533,7 +1533,7 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa 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 %q defined in the %q keyspace", + return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence table name %q defined in the %s keyspace", seqTable.AutoIncrement.Sequence, ts.targetKeyspace) } // Unescape the table name and keyspace name as they may be escaped in the @@ -1571,15 +1571,16 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa ) if len(seqTable.ColumnVindexes[i].Columns) > 0 { unescapedColumn, err = sqlescape.UnescapeID(seqTable.ColumnVindexes[i].Columns[0]) // AutoIncrement definitions can only be on a single column + seqTable.ColumnVindexes[i].Columns[0] = unescapedColumn } else { // This is the legacy vschema definition. unescapedColumn, err = sqlescape.UnescapeID(seqTable.ColumnVindexes[i].Column) + seqTable.ColumnVindexes[i].Column = unescapedColumn } if err != nil { return nil, false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid sequence column vindex name %q defined in sequence table %+v: %v", seqTable.ColumnVindexes[i].Column, seqTable, err) } - seqTable.ColumnVindexes[i].Column = unescapedColumn } unescapedAutoIncCol, err := sqlescape.UnescapeID(seqTable.AutoIncrement.Column) if err != nil { diff --git a/go/vt/vtctl/workflow/traffic_switcher_test.go b/go/vt/vtctl/workflow/traffic_switcher_test.go index 6f05a787f53..495b00d7144 100644 --- a/go/vt/vtctl/workflow/traffic_switcher_test.go +++ b/go/vt/vtctl/workflow/traffic_switcher_test.go @@ -19,7 +19,6 @@ package workflow import ( "context" "fmt" - "reflect" "strconv" "strings" "testing" @@ -74,6 +73,7 @@ func TestGetTargetSequenceMetadata(t *testing.T) { cell := "cell1" workflow := "wf1" table := "`t1`" + table2 := "t2" unescapedTable := "t1" sourceKeyspace := &testKeyspace{ KeyspaceName: "source-ks", @@ -201,6 +201,89 @@ func TestGetTargetSequenceMetadata(t *testing.T) { }, }, }, + { + name: "sequences using vindexes with both column definition structures", + sourceVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + "seq1": { + Type: "sequence", + }, + "seq2": { + Type: "sequence", + }, + }, + }, + targetVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + table: { + ColumnVindexes: []*vschema.ColumnVindex{ + { + Name: "xxhash", + Column: "col1", + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "col1", + Sequence: fmt.Sprintf("%s.seq1", sourceKeyspace.KeyspaceName), + }, + }, + table2: { + ColumnVindexes: []*vschema.ColumnVindex{ + { + Name: "xxhash", + Columns: []string{"col2"}, + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "col2", + Sequence: fmt.Sprintf("%s.seq2", sourceKeyspace.KeyspaceName), + }, + }, + }, + }, + want: map[string]*sequenceMetadata{ + "seq1": { + backingTableName: "seq1", + backingTableKeyspace: "source-ks", + backingTableDBName: "vt_source-ks", + usingTableName: unescapedTable, + usingTableDBName: "vt_targetks", + usingTableDefinition: &vschema.Table{ + ColumnVindexes: []*vschema.ColumnVindex{ + { + Column: "col1", + Name: "xxhash", + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "col1", + Sequence: fmt.Sprintf("%s.seq1", sourceKeyspace.KeyspaceName), + }, + }, + }, + "seq2": { + backingTableName: "seq2", + backingTableKeyspace: "source-ks", + backingTableDBName: "vt_source-ks", + usingTableName: table2, + usingTableDBName: "vt_targetks", + usingTableDefinition: &vschema.Table{ + ColumnVindexes: []*vschema.ColumnVindex{ + { + Columns: []string{"col2"}, + Name: "xxhash", + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "col2", + Sequence: fmt.Sprintf("%s.seq2", sourceKeyspace.KeyspaceName), + }, + }, + }, + }, + }, { name: "invalid table name", sourceVSchema: &vschema.Keyspace{ @@ -228,7 +311,7 @@ func TestGetTargetSequenceMetadata(t *testing.T) { }, }, }, - err: "invalid table name `my-`seq1` in keyspace source-ks: UnescapeID err: unexpected single backtick at position 3 in '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", @@ -257,7 +340,7 @@ func TestGetTargetSequenceMetadata(t *testing.T) { }, }, }, - 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'", + 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", @@ -286,7 +369,7 @@ func TestGetTargetSequenceMetadata(t *testing.T) { }, }, }, - 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'", + 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", @@ -315,7 +398,7 @@ func TestGetTargetSequenceMetadata(t *testing.T) { }, }, }, - 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'", + 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'", }, } @@ -349,7 +432,7 @@ func TestGetTargetSequenceMetadata(t *testing.T) { id: 1, ws: env.ws, workflow: workflow, - tables: []string{table}, + tables: []string{table, table2}, sourceKeyspace: sourceKeyspace.KeyspaceName, targetKeyspace: targetKeyspace.KeyspaceName, sources: sources, @@ -361,7 +444,7 @@ func TestGetTargetSequenceMetadata(t *testing.T) { } else { require.NoError(t, err) } - require.True(t, reflect.DeepEqual(tc.want, got), "want: %v, got: %v", tc.want, got) + require.EqualValues(t, tc.want, got) }) } } From ab831073f8bd7b09d7b84d98921359f37880327d Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Sat, 28 Sep 2024 01:57:24 -0400 Subject: [PATCH 3/3] Support multi-col vindexes Signed-off-by: Matt Lord --- go/vt/vtctl/workflow/traffic_switcher.go | 6 ++- go/vt/vtctl/workflow/traffic_switcher_test.go | 49 +++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/go/vt/vtctl/workflow/traffic_switcher.go b/go/vt/vtctl/workflow/traffic_switcher.go index b762ad42f5f..65dcecb9623 100644 --- a/go/vt/vtctl/workflow/traffic_switcher.go +++ b/go/vt/vtctl/workflow/traffic_switcher.go @@ -1570,8 +1570,10 @@ func (ts *trafficSwitcher) findSequenceUsageInKeyspace(vschema *vschemapb.Keyspa err error ) if len(seqTable.ColumnVindexes[i].Columns) > 0 { - unescapedColumn, err = sqlescape.UnescapeID(seqTable.ColumnVindexes[i].Columns[0]) // AutoIncrement definitions can only be on a single column - seqTable.ColumnVindexes[i].Columns[0] = unescapedColumn + for n := range seqTable.ColumnVindexes[i].Columns { + unescapedColumn, err = sqlescape.UnescapeID(seqTable.ColumnVindexes[i].Columns[n]) + seqTable.ColumnVindexes[i].Columns[n] = unescapedColumn + } } else { // This is the legacy vschema definition. unescapedColumn, err = sqlescape.UnescapeID(seqTable.ColumnVindexes[i].Column) diff --git a/go/vt/vtctl/workflow/traffic_switcher_test.go b/go/vt/vtctl/workflow/traffic_switcher_test.go index 495b00d7144..4b3d17036c1 100644 --- a/go/vt/vtctl/workflow/traffic_switcher_test.go +++ b/go/vt/vtctl/workflow/traffic_switcher_test.go @@ -284,6 +284,55 @@ func TestGetTargetSequenceMetadata(t *testing.T) { }, }, }, + { + name: "sequence with table having mult-col vindex", + sourceVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + "seq1": { + Type: "sequence", + }, + }, + }, + targetVSchema: &vschema.Keyspace{ + Vindexes: vindexes, + Tables: map[string]*vschema.Table{ + table: { + ColumnVindexes: []*vschema.ColumnVindex{ + { + Name: "xxhash", + Columns: []string{"col3", "col4"}, + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "col1", + Sequence: fmt.Sprintf("%s.seq1", sourceKeyspace.KeyspaceName), + }, + }, + }, + }, + want: map[string]*sequenceMetadata{ + "seq1": { + backingTableName: "seq1", + backingTableKeyspace: "source-ks", + backingTableDBName: "vt_source-ks", + usingTableName: unescapedTable, + usingTableDBName: "vt_targetks", + usingTableDefinition: &vschema.Table{ + ColumnVindexes: []*vschema.ColumnVindex{ + { + Columns: []string{"col3", "col4"}, + Name: "xxhash", + }, + }, + AutoIncrement: &vschema.AutoIncrement{ + Column: "col1", + Sequence: fmt.Sprintf("%s.seq1", sourceKeyspace.KeyspaceName), + }, + }, + }, + }, + }, { name: "invalid table name", sourceVSchema: &vschema.Keyspace{