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 30814dfc26c..a4edb09ec9b 100644 --- a/go/vt/schemadiff/types.go +++ b/go/vt/schemadiff/types.go @@ -120,8 +120,8 @@ const ( ) const ( - EnumReorderStrategyReject int = iota - EnumReorderStrategyAllow + EnumReorderStrategyAllow int = iota + EnumReorderStrategyReject ) // DiffHints is an assortment of rules for diffing entities diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index dcebc0b9bd4..de641d1d65a 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -2798,7 +2798,6 @@ func (e *Executor) evaluateDeclarativeDiff(ctx context.Context, onlineDDL *schem senv := schemadiff.NewEnv(e.env.Environment(), e.env.Environment().CollationEnv().DefaultConnectionCharset()) hints := &schemadiff.DiffHints{ AutoIncrementStrategy: schemadiff.AutoIncrementApplyHigher, - EnumReorderStrategy: schemadiff.EnumReorderStrategyAllow, } switch ddlStmt.(type) { case *sqlparser.CreateTable: diff --git a/go/vt/vttablet/onlineddl/vrepl.go b/go/vt/vttablet/onlineddl/vrepl.go index ef38fb7d012..847e40e3fbc 100644 --- a/go/vt/vttablet/onlineddl/vrepl.go +++ b/go/vt/vttablet/onlineddl/vrepl.go @@ -491,11 +491,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 index 8671badadc0..79e2df614f4 100644 --- a/go/vt/vttablet/onlineddl/vrepl/foreign_key.go +++ b/go/vt/vttablet/onlineddl/vrepl/foreign_key.go @@ -38,7 +38,6 @@ func RemovedForeignKeyNames( env := schemadiff.NewEnv(venv, venv.CollationEnv().DefaultConnectionCharset()) diffHints := schemadiff.DiffHints{ ConstraintNamesStrategy: schemadiff.ConstraintNamesIgnoreAll, - EnumReorderStrategy: schemadiff.EnumReorderStrategyAllow, } diff, err := schemadiff.DiffCreateTablesQueries(env, originalCreateTable, vreplCreateTable, &diffHints) if err != nil { diff --git a/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go b/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go index a328249d0e0..424daad4871 100644 --- a/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go +++ b/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go @@ -338,8 +338,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)