From ca46cb1ab6ccf671a05665107e7e33c75d5ea7db Mon Sep 17 00:00:00 2001 From: "vitess-bot[bot]" <108069721+vitess-bot[bot]@users.noreply.github.com> Date: Sun, 25 Feb 2024 18:08:29 +0200 Subject: [PATCH 1/4] Cherry-pick 47e137539bf70eade637edcbcbe151205ffebcee with conflicts --- .../vrepl_suite/testdata/enum-reorder/alter | 1 + .../testdata/enum-reorder/create.sql | 26 ++ go/vt/schemadiff/types.go | 8 + go/vt/vttablet/onlineddl/executor.go | 7 + go/vt/vttablet/onlineddl/vrepl.go | 23 +- go/vt/vttablet/onlineddl/vrepl/columns.go | 2 +- .../vttablet/onlineddl/vrepl/columns_test.go | 245 ++++++++++++++++++ go/vt/vttablet/onlineddl/vrepl/foreign_key.go | 58 +++++ .../vreplication/replicator_plan.go | 9 +- 9 files changed, 372 insertions(+), 7 deletions(-) create mode 100644 go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-reorder/alter create mode 100644 go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-reorder/create.sql create mode 100644 go/vt/vttablet/onlineddl/vrepl/foreign_key.go diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-reorder/alter b/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-reorder/alter new file mode 100644 index 00000000000..6e011c14192 --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-reorder/alter @@ -0,0 +1 @@ +change e e enum('blue', 'green', 'red') not null diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-reorder/create.sql b/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-reorder/create.sql new file mode 100644 index 00000000000..84ebd4094c1 --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-reorder/create.sql @@ -0,0 +1,26 @@ +drop table if exists onlineddl_test; +create table onlineddl_test ( + id int auto_increment, + i int not null, + e enum('red', 'green', 'blue') not null, + primary key(id) +) auto_increment=1; + +insert into onlineddl_test values (null, 11, 'red'); +insert into onlineddl_test values (null, 13, 'green'); +insert into onlineddl_test values (null, 17, 'blue'); + +drop event if exists onlineddl_test; +delimiter ;; +create event onlineddl_test + on schedule every 1 second + starts current_timestamp + ends current_timestamp + interval 60 second + on completion not preserve + enable + do +begin + insert into onlineddl_test values (null, 211, 'red'); + insert into onlineddl_test values (null, 213, 'green'); + insert into onlineddl_test values (null, 217, 'blue'); +end ;; diff --git a/go/vt/schemadiff/types.go b/go/vt/schemadiff/types.go index 86e5a8d06bf..9429eb4001e 100644 --- a/go/vt/schemadiff/types.go +++ b/go/vt/schemadiff/types.go @@ -108,6 +108,14 @@ const ( AlterTableAlgorithmStrategyCopy ) +<<<<<<< HEAD +======= +const ( + EnumReorderStrategyAllow int = iota + EnumReorderStrategyReject +) + +>>>>>>> 47e137539b (VReplication/OnlineDDL: reordering enum values (#15103)) // DiffHints is an assortment of rules for diffing entities type DiffHints struct { StrictIndexOrdering bool diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index 66e81aef949..8a4e70921fe 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -2625,7 +2625,14 @@ func (e *Executor) evaluateDeclarativeDiff(ctx context.Context, onlineDDL *schem if newShowCreateTable == "" { return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "unexpected: cannot find table or view even as it was just created: %v", onlineDDL.Table) } +<<<<<<< HEAD hints := &schemadiff.DiffHints{AutoIncrementStrategy: schemadiff.AutoIncrementApplyHigher} +======= + senv := schemadiff.NewEnv(e.env.Environment(), e.env.Environment().CollationEnv().DefaultConnectionCharset()) + hints := &schemadiff.DiffHints{ + AutoIncrementStrategy: schemadiff.AutoIncrementApplyHigher, + } +>>>>>>> 47e137539b (VReplication/OnlineDDL: reordering enum values (#15103)) switch ddlStmt.(type) { case *sqlparser.CreateTable: diff, err = schemadiff.DiffCreateTablesQueries(existingShowCreateTable, newShowCreateTable, hints) diff --git a/go/vt/vttablet/onlineddl/vrepl.go b/go/vt/vttablet/onlineddl/vrepl.go index cc669e11c11..50f9eb838d0 100644 --- a/go/vt/vttablet/onlineddl/vrepl.go +++ b/go/vt/vttablet/onlineddl/vrepl.go @@ -434,11 +434,26 @@ func (v *VRepl) analyzeTables(ctx context.Context, conn *dbconnpool.DBConnection for i := range v.sourceSharedColumns.Columns() { sourceColumn := v.sourceSharedColumns.Columns()[i] mappedColumn := v.targetSharedColumns.Columns()[i] - if sourceColumn.Type == vrepl.EnumColumnType && mappedColumn.Type != vrepl.EnumColumnType && mappedColumn.Charset != "" { - // A column is converted from ENUM type to textual type - v.targetSharedColumns.SetEnumToTextConversion(mappedColumn.Name, sourceColumn.EnumValues) - v.enumToTextMap[sourceColumn.Name] = sourceColumn.EnumValues + if sourceColumn.Type == vrepl.EnumColumnType { + switch { + // Either this is an ENUM column that stays an ENUM, or it is converted to a textual type. + // We take note of the enum values, and make it available in vreplication's Filter.Rule.ConvertEnumToText. + // This, in turn, will be used by vplayer (in TablePlan) like so: + // - In the binary log, enum values are integers. + // - Upon seeing this map, PlanBuilder will convert said int to the enum's logical string value. + // - And will apply the value as a string (`StringBindVariable`) in the query. + // What this allows is for enum values to have different ordering in the before/after table schema, + // so that for example you could modify an enum column: + // - from `('red', 'green', 'blue')` to `('red', 'blue')` + // - from `('red', 'green', 'blue')` to `('blue', 'red', 'green')` + case mappedColumn.Type == vrepl.EnumColumnType: + v.enumToTextMap[sourceColumn.Name] = sourceColumn.EnumValues + case mappedColumn.Charset != "": + v.enumToTextMap[sourceColumn.Name] = sourceColumn.EnumValues + v.targetSharedColumns.SetEnumToTextConversion(mappedColumn.Name, sourceColumn.EnumValues) + } } + if sourceColumn.IsIntegralType() && mappedColumn.Type == vrepl.EnumColumnType { v.intToEnumMap[sourceColumn.Name] = true } diff --git a/go/vt/vttablet/onlineddl/vrepl/columns.go b/go/vt/vttablet/onlineddl/vrepl/columns.go index 2937b1b2b2c..f2bb8f6d3f2 100644 --- a/go/vt/vttablet/onlineddl/vrepl/columns.go +++ b/go/vt/vttablet/onlineddl/vrepl/columns.go @@ -129,7 +129,7 @@ func isExpandedColumn(sourceColumn *Column, targetColumn *Column) (bool, string) return true, "source is unsigned, target is signed" } if sourceColumn.NumericPrecision <= targetColumn.NumericPrecision && !sourceColumn.IsUnsigned && targetColumn.IsUnsigned { - // e.g. INT SIGNED => INT UNSIGNED, INT SIGNED = BIGINT UNSIGNED + // e.g. INT SIGNED => INT UNSIGNED, INT SIGNED => BIGINT UNSIGNED return true, "target unsigned value exceeds source unsigned value" } if targetColumn.IsFloatingPoint() && !sourceColumn.IsFloatingPoint() { diff --git a/go/vt/vttablet/onlineddl/vrepl/columns_test.go b/go/vt/vttablet/onlineddl/vrepl/columns_test.go index b4d3ac9af58..201ffe55201 100644 --- a/go/vt/vttablet/onlineddl/vrepl/columns_test.go +++ b/go/vt/vttablet/onlineddl/vrepl/columns_test.go @@ -133,3 +133,248 @@ func TestGetSharedColumns(t *testing.T) { }) } } + +func TestGetExpandedColumnNames(t *testing.T) { + var ( + columnsA = &ColumnList{ + columns: []Column{ + { + Name: "c1", + IsNullable: true, + }, + { + Name: "c2", + IsNullable: true, + }, + { + Name: "c3", + IsNullable: false, + }, + }, + Ordinals: ColumnsMap{}, + } + columnsB = &ColumnList{ + columns: []Column{ + { + Name: "c1", + IsNullable: true, + }, + { + Name: "c2", + IsNullable: false, + }, + { + Name: "c3", + IsNullable: true, + }, + }, + Ordinals: ColumnsMap{}, + } + ) + tcases := []struct { + name string + sourceCol Column + targetCol Column + expanded bool + }{ + { + "both nullable", + Column{ + IsNullable: true, + }, + Column{ + IsNullable: true, + }, + false, + }, + { + "nullable to non nullable", + Column{ + IsNullable: true, + }, + Column{ + IsNullable: false, + }, + false, + }, + { + "non nullable to nullable", + Column{ + IsNullable: false, + }, + Column{ + IsNullable: true, + }, + true, + }, + { + "signed to unsigned", + Column{ + Type: IntegerColumnType, + NumericPrecision: 4, + IsUnsigned: false, + }, + Column{ + Type: IntegerColumnType, + NumericPrecision: 4, + IsUnsigned: true, + }, + true, + }, + { + "unsigned to signed", + Column{ + Type: IntegerColumnType, + NumericPrecision: 4, + IsUnsigned: true, + }, + Column{ + Type: IntegerColumnType, + NumericPrecision: 4, + IsUnsigned: false, + }, + true, + }, + { + "signed to smaller unsigned", + Column{ + Type: IntegerColumnType, + NumericPrecision: 8, + IsUnsigned: false, + }, + Column{ + Type: IntegerColumnType, + NumericPrecision: 4, + IsUnsigned: true, + }, + false, + }, + { + "same char length", + Column{ + CharacterMaximumLength: 20, + }, + Column{ + CharacterMaximumLength: 20, + }, + false, + }, + { + "reduced char length", + Column{ + CharacterMaximumLength: 20, + }, + Column{ + CharacterMaximumLength: 19, + }, + false, + }, + { + "increased char length", + Column{ + CharacterMaximumLength: 20, + }, + Column{ + CharacterMaximumLength: 21, + }, + true, + }, + { + "expand temporal", + Column{ + DataType: "time", + }, + Column{ + DataType: "timestamp", + }, + true, + }, + { + "expand temporal", + Column{ + DataType: "date", + }, + Column{ + DataType: "timestamp", + }, + true, + }, + { + "expand temporal", + Column{ + DataType: "date", + }, + Column{ + DataType: "datetime", + }, + true, + }, + { + "non expand temporal", + Column{ + DataType: "datetime", + }, + Column{ + DataType: "timestamp", + }, + false, + }, + { + "expand temporal", + Column{ + DataType: "timestamp", + }, + Column{ + DataType: "datetime", + }, + true, + }, + { + "expand enum", + Column{ + Type: EnumColumnType, + EnumValues: "'a', 'b'", + }, + Column{ + Type: EnumColumnType, + EnumValues: "'a', 'x'", + }, + true, + }, + { + "expand enum", + Column{ + Type: EnumColumnType, + EnumValues: "'a', 'b'", + }, + Column{ + Type: EnumColumnType, + EnumValues: "'a', 'b', 'c'", + }, + true, + }, + { + "reduce enum", + Column{ + Type: EnumColumnType, + EnumValues: "'a', 'b', 'c'", + }, + Column{ + Type: EnumColumnType, + EnumValues: "'a', 'b'", + }, + false, + }, + } + + expectedExpandedColumnNames := []string{"c3"} + expandedColumnNames, _ := GetExpandedColumnNames(columnsA, columnsB) + assert.Equal(t, expectedExpandedColumnNames, expandedColumnNames) + + for _, tcase := range tcases { + t.Run(tcase.name, func(t *testing.T) { + expanded, _ := isExpandedColumn(&tcase.sourceCol, &tcase.targetCol) + assert.Equal(t, tcase.expanded, expanded) + }) + } +} diff --git a/go/vt/vttablet/onlineddl/vrepl/foreign_key.go b/go/vt/vttablet/onlineddl/vrepl/foreign_key.go new file mode 100644 index 00000000000..79e2df614f4 --- /dev/null +++ b/go/vt/vttablet/onlineddl/vrepl/foreign_key.go @@ -0,0 +1,58 @@ +/* + Copyright 2016 GitHub Inc. + See https://github.com/github/gh-ost/blob/master/LICENSE +*/ +/* +Copyright 2021 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 vrepl + +import ( + "vitess.io/vitess/go/vt/schemadiff" + "vitess.io/vitess/go/vt/sqlparser" + "vitess.io/vitess/go/vt/vtenv" +) + +// RemovedForeignKeyNames returns the names of removed foreign keys, ignoring mere name changes +func RemovedForeignKeyNames( + venv *vtenv.Environment, + originalCreateTable string, + vreplCreateTable string, +) (names []string, err error) { + if originalCreateTable == "" || vreplCreateTable == "" { + return nil, nil + } + env := schemadiff.NewEnv(venv, venv.CollationEnv().DefaultConnectionCharset()) + diffHints := schemadiff.DiffHints{ + ConstraintNamesStrategy: schemadiff.ConstraintNamesIgnoreAll, + } + diff, err := schemadiff.DiffCreateTablesQueries(env, originalCreateTable, vreplCreateTable, &diffHints) + if err != nil { + return nil, err + } + + validateWalk := func(node sqlparser.SQLNode) (kontinue bool, err error) { + switch node := node.(type) { + case *sqlparser.DropKey: + if node.Type == sqlparser.ForeignKeyType { + names = append(names, node.Name.String()) + } + } + return true, nil + } + _ = sqlparser.Walk(validateWalk, diff.Statement()) // We never return an error + return names, nil +} diff --git a/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go b/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go index 39ffdef04ae..b5deb309d45 100644 --- a/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go +++ b/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go @@ -332,8 +332,13 @@ func (tp *TablePlan) bindFieldVal(field *querypb.Field, val *sqltypes.Value) (*q if enumValues, ok := tp.EnumValuesMap[field.Name]; ok && !val.IsNull() { // The fact that this field has a EnumValuesMap entry, means we must // use the enum's text value as opposed to the enum's numerical value. - // Once known use case is with Online DDL, when a column is converted from - // ENUM to a VARCHAR/TEXT. + // This may be needed in Online DDL, when the enum column could be modified: + // - Either from ENUM to a text type (VARCHAR/TEXT) + // - Or from ENUM to another ENUM with different value ordering, + // e.g. from `('red', 'green', 'blue')` to `('red', 'blue')`. + // By applying the textual value of an enum we eliminate the ordering concern. + // In non-Online DDL this shouldn't be a concern because the schema is static, + // and so passing the enum's numerical value is sufficient. enumValue, enumValueOK := enumValues[val.ToString()] if !enumValueOK { return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "Invalid enum value: %v for field %s", val, field.Name) From a9dcd0ed24b3a5e90e94305f0348586939c8cad0 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 25 Feb 2024 18:17:06 +0200 Subject: [PATCH 2/4] resolved conflict Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/types.go | 8 -------- go/vt/vttablet/onlineddl/executor.go | 7 ------- go/vt/vttablet/onlineddl/vrepl/foreign_key.go | 5 +---- 3 files changed, 1 insertion(+), 19 deletions(-) diff --git a/go/vt/schemadiff/types.go b/go/vt/schemadiff/types.go index 9429eb4001e..86e5a8d06bf 100644 --- a/go/vt/schemadiff/types.go +++ b/go/vt/schemadiff/types.go @@ -108,14 +108,6 @@ const ( AlterTableAlgorithmStrategyCopy ) -<<<<<<< HEAD -======= -const ( - EnumReorderStrategyAllow int = iota - EnumReorderStrategyReject -) - ->>>>>>> 47e137539b (VReplication/OnlineDDL: reordering enum values (#15103)) // DiffHints is an assortment of rules for diffing entities type DiffHints struct { StrictIndexOrdering bool diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index 8a4e70921fe..66e81aef949 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -2625,14 +2625,7 @@ func (e *Executor) evaluateDeclarativeDiff(ctx context.Context, onlineDDL *schem if newShowCreateTable == "" { return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "unexpected: cannot find table or view even as it was just created: %v", onlineDDL.Table) } -<<<<<<< HEAD hints := &schemadiff.DiffHints{AutoIncrementStrategy: schemadiff.AutoIncrementApplyHigher} -======= - senv := schemadiff.NewEnv(e.env.Environment(), e.env.Environment().CollationEnv().DefaultConnectionCharset()) - hints := &schemadiff.DiffHints{ - AutoIncrementStrategy: schemadiff.AutoIncrementApplyHigher, - } ->>>>>>> 47e137539b (VReplication/OnlineDDL: reordering enum values (#15103)) switch ddlStmt.(type) { case *sqlparser.CreateTable: diff, err = schemadiff.DiffCreateTablesQueries(existingShowCreateTable, newShowCreateTable, hints) diff --git a/go/vt/vttablet/onlineddl/vrepl/foreign_key.go b/go/vt/vttablet/onlineddl/vrepl/foreign_key.go index 79e2df614f4..9f191176d6b 100644 --- a/go/vt/vttablet/onlineddl/vrepl/foreign_key.go +++ b/go/vt/vttablet/onlineddl/vrepl/foreign_key.go @@ -23,23 +23,20 @@ package vrepl import ( "vitess.io/vitess/go/vt/schemadiff" "vitess.io/vitess/go/vt/sqlparser" - "vitess.io/vitess/go/vt/vtenv" ) // RemovedForeignKeyNames returns the names of removed foreign keys, ignoring mere name changes func RemovedForeignKeyNames( - venv *vtenv.Environment, originalCreateTable string, vreplCreateTable string, ) (names []string, err error) { if originalCreateTable == "" || vreplCreateTable == "" { return nil, nil } - env := schemadiff.NewEnv(venv, venv.CollationEnv().DefaultConnectionCharset()) diffHints := schemadiff.DiffHints{ ConstraintNamesStrategy: schemadiff.ConstraintNamesIgnoreAll, } - diff, err := schemadiff.DiffCreateTablesQueries(env, originalCreateTable, vreplCreateTable, &diffHints) + diff, err := schemadiff.DiffCreateTablesQueries(originalCreateTable, vreplCreateTable, &diffHints) if err != nil { return nil, err } From 0edde554c4735acdae6d37ff9b4f99c2ab2479a2 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 25 Feb 2024 18:18:25 +0200 Subject: [PATCH 3/4] resolved conflict Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/vttablet/onlineddl/vrepl/foreign_key.go | 55 ------------------- 1 file changed, 55 deletions(-) delete mode 100644 go/vt/vttablet/onlineddl/vrepl/foreign_key.go diff --git a/go/vt/vttablet/onlineddl/vrepl/foreign_key.go b/go/vt/vttablet/onlineddl/vrepl/foreign_key.go deleted file mode 100644 index 9f191176d6b..00000000000 --- a/go/vt/vttablet/onlineddl/vrepl/foreign_key.go +++ /dev/null @@ -1,55 +0,0 @@ -/* - Copyright 2016 GitHub Inc. - See https://github.com/github/gh-ost/blob/master/LICENSE -*/ -/* -Copyright 2021 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 vrepl - -import ( - "vitess.io/vitess/go/vt/schemadiff" - "vitess.io/vitess/go/vt/sqlparser" -) - -// RemovedForeignKeyNames returns the names of removed foreign keys, ignoring mere name changes -func RemovedForeignKeyNames( - originalCreateTable string, - vreplCreateTable string, -) (names []string, err error) { - if originalCreateTable == "" || vreplCreateTable == "" { - return nil, nil - } - diffHints := schemadiff.DiffHints{ - ConstraintNamesStrategy: schemadiff.ConstraintNamesIgnoreAll, - } - diff, err := schemadiff.DiffCreateTablesQueries(originalCreateTable, vreplCreateTable, &diffHints) - if err != nil { - return nil, err - } - - validateWalk := func(node sqlparser.SQLNode) (kontinue bool, err error) { - switch node := node.(type) { - case *sqlparser.DropKey: - if node.Type == sqlparser.ForeignKeyType { - names = append(names, node.Name.String()) - } - } - return true, nil - } - _ = sqlparser.Walk(validateWalk, diff.Statement()) // We never return an error - return names, nil -} From c5609e03e3b5c26537debb2d64bf1303991727d8 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 25 Feb 2024 18:20:41 +0200 Subject: [PATCH 4/4] empty commit to kick CI Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>