diff --git a/ddl/column.go b/ddl/column.go index 60dc9dc1e057f..7578d81ec0ed5 100644 --- a/ddl/column.go +++ b/ddl/column.go @@ -688,7 +688,13 @@ func needChangeColumnData(oldCol, newCol *model.ColumnInfo) bool { toUnsigned := mysql.HasUnsignedFlag(newCol.Flag) originUnsigned := mysql.HasUnsignedFlag(oldCol.Flag) needTruncationOrToggleSign := func() bool { - return newCol.Flen > 0 && newCol.Flen < oldCol.Flen || toUnsigned != originUnsigned + return (newCol.Flen > 0 && newCol.Flen < oldCol.Flen) || (toUnsigned != originUnsigned) + } + // Ignore the potential max display length represented by integer's flen, use default flen instead. + oldColFlen, _ := mysql.GetDefaultFieldLengthAndDecimal(oldCol.Tp) + newColFlen, _ := mysql.GetDefaultFieldLengthAndDecimal(newCol.Tp) + needTruncationOrToggleSignForInteger := func() bool { + return (newColFlen > 0 && newColFlen < oldColFlen) || (toUnsigned != originUnsigned) } // Deal with the same type. @@ -700,6 +706,8 @@ func needChangeColumnData(oldCol, newCol *model.ColumnInfo) bool { return oldCol.Flen != newCol.Flen || oldCol.Decimal != newCol.Decimal || toUnsigned != originUnsigned case mysql.TypeEnum, mysql.TypeSet: return isElemsChangedToModifyColumn(oldCol.Elems, newCol.Elems) + case mysql.TypeTiny, mysql.TypeShort, mysql.TypeInt24, mysql.TypeLong, mysql.TypeLonglong: + return toUnsigned != originUnsigned } return needTruncationOrToggleSign() @@ -715,7 +723,7 @@ func needChangeColumnData(oldCol, newCol *model.ColumnInfo) bool { case mysql.TypeTiny, mysql.TypeShort, mysql.TypeInt24, mysql.TypeLong, mysql.TypeLonglong: switch newCol.Tp { case mysql.TypeTiny, mysql.TypeShort, mysql.TypeInt24, mysql.TypeLong, mysql.TypeLonglong: - return needTruncationOrToggleSign() + return needTruncationOrToggleSignForInteger() } case mysql.TypeFloat, mysql.TypeDouble: switch newCol.Tp { diff --git a/ddl/column_test.go b/ddl/column_test.go index e080a7e3e6bd1..6dc9125208175 100644 --- a/ddl/column_test.go +++ b/ddl/column_test.go @@ -1152,7 +1152,7 @@ func (s *testColumnSuite) TestModifyColumn(c *C) { err error }{ {"int", "bigint", nil}, - {"int", "int unsigned", errUnsupportedModifyColumn.GenWithStackByArgs("length 10 is less than origin 11, and tidb_enable_change_column_type is false")}, + {"int", "int unsigned", errUnsupportedModifyColumn.GenWithStackByArgs("can't change unsigned integer to signed or vice versa, and tidb_enable_change_column_type is false")}, {"varchar(10)", "text", nil}, {"varbinary(10)", "blob", nil}, {"text", "blob", errUnsupportedModifyCharset.GenWithStackByArgs("charset from utf8mb4 to binary")}, diff --git a/ddl/column_type_change_test.go b/ddl/column_type_change_test.go index 4f3207d032b96..88a7333cda763 100644 --- a/ddl/column_type_change_test.go +++ b/ddl/column_type_change_test.go @@ -939,3 +939,51 @@ func (s *testColumnTypeChangeSuite) TestColumnTypeChangeFromNumericToOthers(c *C tk.MustExec("alter table t modify b json") tk.MustQuery("select * from t").Check(testkit.Rows("-258.12345 333.33 2000000.20000002 323232323.32323235 -111.11111450195312 -222222222222.22223 \"\\u0015\"")) } + +// Test issue #20529. +func (s *testColumnTypeChangeSuite) TestColumnTypeChangeIgnoreDisplayLength(c *C) { + tk := testkit.NewTestKit(c, s.store) + tk.MustExec("use test") + tk.Se.GetSessionVars().EnableChangeColumnType = true + defer func() { + tk.Se.GetSessionVars().EnableChangeColumnType = false + }() + + originalHook := s.dom.DDL().GetHook() + defer s.dom.DDL().(ddl.DDLForTest).SetHook(originalHook) + + var assertResult bool + assertHasAlterWriteReorg := func(tbl table.Table) { + // Restore the assert result to false. + assertResult = false + hook := &ddl.TestDDLCallback{} + hook.OnJobRunBeforeExported = func(job *model.Job) { + if tbl.Meta().ID != job.TableID { + return + } + if job.SchemaState == model.StateWriteReorganization { + assertResult = true + } + } + s.dom.DDL().(ddl.DDLForTest).SetHook(hook) + } + + // Change int to tinyint. + // Although display length is increased, the default flen is decreased, reorg is needed. + tk.MustExec("drop table if exists t") + tk.MustExec("create table t(a int(1))") + tbl := testGetTableByName(c, tk.Se, "test", "t") + assertHasAlterWriteReorg(tbl) + tk.MustExec("alter table t modify column a tinyint(3)") + c.Assert(assertResult, Equals, true) + + // Change tinyint to tinyint + // Although display length is decreased, default flen is the same, reorg is not needed. + tk.MustExec("drop table if exists t") + tk.MustExec("create table t(a tinyint(3))") + tbl = testGetTableByName(c, tk.Se, "test", "t") + assertHasAlterWriteReorg(tbl) + tk.MustExec("alter table t modify column a tinyint(1)") + c.Assert(assertResult, Equals, false) + tk.MustExec("drop table if exists t") +} diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index f7acacd7d4ff6..9a577ca9ad158 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -3385,6 +3385,10 @@ func checkModifyCharsetAndCollation(toCharset, toCollate, origCharset, origColla // CheckModifyTypeCompatible checks whether changes column type to another is compatible considering // field length and precision. func CheckModifyTypeCompatible(origin *types.FieldType, to *types.FieldType) (allowedChangeColumnValueMsg string, err error) { + var ( + toFlen = to.Flen + originFlen = origin.Flen + ) unsupportedMsg := fmt.Sprintf("type %v not match origin %v", to.CompactStr(), origin.CompactStr()) var skipSignCheck bool var skipLenCheck bool @@ -3405,6 +3409,10 @@ func CheckModifyTypeCompatible(origin *types.FieldType, to *types.FieldType) (al case mysql.TypeTiny, mysql.TypeShort, mysql.TypeInt24, mysql.TypeLong, mysql.TypeLonglong: switch to.Tp { case mysql.TypeTiny, mysql.TypeShort, mysql.TypeInt24, mysql.TypeLong, mysql.TypeLonglong: + // For integers, we should ignore the potential display length represented by flen, using + // the default flen of the type. + originFlen, _ = mysql.GetDefaultFieldLengthAndDecimal(origin.Tp) + toFlen, _ = mysql.GetDefaultFieldLengthAndDecimal(to.Tp) // Changing integer to integer, whether reorg is necessary is depend on the flen/decimal/signed. skipSignCheck = true skipLenCheck = true @@ -3527,8 +3535,8 @@ func CheckModifyTypeCompatible(origin *types.FieldType, to *types.FieldType) (al } } - if to.Flen > 0 && to.Flen < origin.Flen { - msg := fmt.Sprintf("length %d is less than origin %d", to.Flen, origin.Flen) + if toFlen > 0 && toFlen < originFlen { + msg := fmt.Sprintf("length %d is less than origin %d", toFlen, originFlen) if skipLenCheck { return msg, errUnsupportedModifyColumn.GenWithStackByArgs(msg) } diff --git a/ddl/ddl_worker_test.go b/ddl/ddl_worker_test.go index 6efa8df25e2df..301bbb43c65b1 100644 --- a/ddl/ddl_worker_test.go +++ b/ddl/ddl_worker_test.go @@ -456,7 +456,7 @@ func buildCancelJobTests(firstID int64) []testCancelJob { {act: model.ActionShardRowID, jobIDs: []int64{firstID + 17}, cancelRetErrs: noErrs, cancelState: model.StateNone}, {act: model.ActionModifyColumn, jobIDs: []int64{firstID + 18}, cancelRetErrs: noErrs, cancelState: model.StateNone}, - {act: model.ActionModifyColumn, jobIDs: []int64{firstID + 19}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 19)}, cancelState: model.StateDeleteOnly}, + {act: model.ActionModifyColumn, jobIDs: []int64{firstID + 19}, cancelRetErrs: noErrs, cancelState: model.StateDeleteOnly}, {act: model.ActionAddForeignKey, jobIDs: []int64{firstID + 20}, cancelRetErrs: noErrs, cancelState: model.StateNone}, {act: model.ActionAddForeignKey, jobIDs: []int64{firstID + 21}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 21)}, cancelState: model.StatePublic}, @@ -773,7 +773,7 @@ func (s *testDDLSerialSuite) TestCancelJob(c *C) { // modify none-state column col.DefaultValue = "1" updateTest(&tests[15]) - modifyColumnArgs := []interface{}{col, col.Name, &ast.ColumnPosition{}, byte(0)} + modifyColumnArgs := []interface{}{col, col.Name, &ast.ColumnPosition{}, byte(0), uint64(0)} doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, test.act, modifyColumnArgs, &test.cancelState) c.Check(checkErr, IsNil) changedTable = testGetTable(c, d, dbInfo.ID, tblInfo.ID) @@ -784,13 +784,14 @@ func (s *testDDLSerialSuite) TestCancelJob(c *C) { col.FieldType.Tp = mysql.TypeTiny col.FieldType.Flen = col.FieldType.Flen - 1 updateTest(&tests[16]) - modifyColumnArgs = []interface{}{col, col.Name, &ast.ColumnPosition{}, byte(0)} - doDDLJobSuccess(ctx, d, c, dbInfo.ID, tblInfo.ID, test.act, modifyColumnArgs) + modifyColumnArgs = []interface{}{col, col.Name, &ast.ColumnPosition{}, byte(0), uint64(0)} + cancelState = model.StateNone + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, test.act, modifyColumnArgs, &cancelState) c.Check(checkErr, IsNil) changedTable = testGetTable(c, d, dbInfo.ID, tblInfo.ID) changedCol = model.FindColumnInfo(changedTable.Meta().Columns, col.Name.L) - c.Assert(changedCol.FieldType.Tp, Equals, mysql.TypeTiny) - c.Assert(changedCol.FieldType.Flen, Equals, col.FieldType.Flen) + c.Assert(changedCol.FieldType.Tp, Equals, mysql.TypeLonglong) + c.Assert(changedCol.FieldType.Flen, Equals, col.FieldType.Flen+1) col.FieldType.Flen++ // Test add foreign key failed cause by canceled.