From b29d6cf5b2706864dfaa339095dc9defbe212d0a Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Tue, 25 Apr 2023 19:39:52 +0800 Subject: [PATCH] ddl: fix an error message for 'alter table' change column with generated columns dependency (#43350) close pingcap/tidb#24321, close pingcap/tidb#38988 --- ddl/column_change_test.go | 15 ++++++++++++ ddl/column_modify_test.go | 14 +++++++----- ddl/ddl_api.go | 48 +++++++++++++++++++++++++++------------ 3 files changed, 57 insertions(+), 20 deletions(-) diff --git a/ddl/column_change_test.go b/ddl/column_change_test.go index 76a3b377a5abe..b6e3346e8cd59 100644 --- a/ddl/column_change_test.go +++ b/ddl/column_change_test.go @@ -24,6 +24,7 @@ import ( "github.com/pingcap/errors" "github.com/pingcap/tidb/ddl" "github.com/pingcap/tidb/ddl/internal/callback" + "github.com/pingcap/tidb/errno" "github.com/pingcap/tidb/kv" "github.com/pingcap/tidb/meta" "github.com/pingcap/tidb/parser/model" @@ -471,3 +472,17 @@ func TestIssue40135(t *testing.T) { require.ErrorContains(t, checkErr, "[ddl:3855]Column 'a' has a partitioning function dependency and cannot be dropped or renamed") } + +func TestIssue38988And24321(t *testing.T) { + store := testkit.CreateMockStore(t) + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + // For issue https://github.com/pingcap/tidb/issues/38988 + tk.MustExec("create table t (a int, b int as (a+3));") + tk.MustGetErrCode("alter table t change a c int not null;", errno.ErrDependentByGeneratedColumn) + + // For issue https://github.com/pingcap/tidb/issues/24321 + // Note, the result is not the same with MySQL, since the limitation of the current modify column implementation. + tk.MustExec("create table t2(id int, a int, b int generated always as (abs(a)) virtual);") + tk.MustGetErrCode("alter table t2 modify column a bigint;", errno.ErrUnsupportedOnGeneratedColumn) +} diff --git a/ddl/column_modify_test.go b/ddl/column_modify_test.go index 574de0ee8f08a..1e31d3540cca9 100644 --- a/ddl/column_modify_test.go +++ b/ddl/column_modify_test.go @@ -538,7 +538,7 @@ func TestGeneratedColumnDDL(t *testing.T) { }{ // Drop/rename columns dependent by other column. {`alter table test_gv_ddl drop column a`, errno.ErrDependentByGeneratedColumn}, - {`alter table test_gv_ddl change column a anew int`, errno.ErrBadField}, + {`alter table test_gv_ddl change column a anew int`, errno.ErrDependentByGeneratedColumn}, // Modify/change stored status of generated columns. {`alter table test_gv_ddl modify column b bigint`, errno.ErrUnsupportedOnGeneratedColumn}, @@ -546,7 +546,7 @@ func TestGeneratedColumnDDL(t *testing.T) { // Modify/change generated columns breaking prior. {`alter table test_gv_ddl modify column b int as (c+100)`, errno.ErrGeneratedColumnNonPrior}, - {`alter table test_gv_ddl change column b bnew int as (c+100)`, errno.ErrGeneratedColumnNonPrior}, + {`alter table test_gv_ddl change column b bnew int as (c+100)`, errno.ErrDependentByGeneratedColumn}, // Refer not exist columns in generation expression. {`create table test_gv_ddl_bad (a int, b int as (c+8))`, errno.ErrBadField}, @@ -582,13 +582,15 @@ func TestGeneratedColumnDDL(t *testing.T) { result = tk.MustQuery(`DESC test_gv_ddl`) result.Check(testkit.Rows(`a int(11) YES `, `b int(11) YES VIRTUAL GENERATED`, `c int(11) YES STORED GENERATED`)) - tk.MustExec(`alter table test_gv_ddl change column b b bigint as (a+100) virtual`) - result = tk.MustQuery(`DESC test_gv_ddl`) - result.Check(testkit.Rows(`a int(11) YES `, `b bigint(20) YES VIRTUAL GENERATED`, `c int(11) YES STORED GENERATED`)) + // According to https://github.com/pingcap/tidb/issues/24321, this test case is not supported. + // Although in MySQL this is a legal one. + // tk.MustExec(`alter table test_gv_ddl change column b b bigint as (a+100) virtual`) + // result = tk.MustQuery(`DESC test_gv_ddl`) + // result.Check(testkit.Rows(`a int(11) YES `, `b bigint(20) YES VIRTUAL GENERATED`, `c int(11) YES STORED GENERATED`)) tk.MustExec(`alter table test_gv_ddl change column c cnew bigint`) result = tk.MustQuery(`DESC test_gv_ddl`) - result.Check(testkit.Rows(`a int(11) YES `, `b bigint(20) YES VIRTUAL GENERATED`, `cnew bigint(20) YES `)) + result.Check(testkit.Rows(`a int(11) YES `, `b int(11) YES VIRTUAL GENERATED`, `cnew bigint(20) YES `)) // Test generated column `\\`. tk.MustExec("drop table if exists t") diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index f9d5039b41351..1a997ce623cc6 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -4907,6 +4907,24 @@ func (d *ddl) getModifiableColumnJob(ctx context.Context, sctx sessionctx.Contex return GetModifiableColumnJob(ctx, sctx, is, ident, originalColName, schema, t, spec) } +func checkModifyColumnWithGeneratedColumnsConstraint(allCols []*table.Column, oldColName model.CIStr) error { + for _, col := range allCols { + if col.GeneratedExpr == nil { + continue + } + dependedColNames := FindColumnNamesInExpr(col.GeneratedExpr) + for _, name := range dependedColNames { + if name.Name.L == oldColName.L { + if col.Hidden { + return dbterror.ErrDependentByFunctionalIndex.GenWithStackByArgs(oldColName.O) + } + return dbterror.ErrDependentByGeneratedColumn.GenWithStackByArgs(oldColName.O) + } + } + } + return nil +} + // GetModifiableColumnJob returns a DDL job of model.ActionModifyColumn. func GetModifiableColumnJob( ctx context.Context, @@ -4929,12 +4947,20 @@ func GetModifiableColumnJob( if newColName.L == model.ExtraHandleName.L { return nil, dbterror.ErrWrongColumnName.GenWithStackByArgs(newColName.L) } + errG := checkModifyColumnWithGeneratedColumnsConstraint(t.Cols(), originalColName) + // If we want to rename the column name, we need to check whether it already exists. if newColName.L != originalColName.L { c := table.FindCol(t.Cols(), newColName.L) if c != nil { return nil, infoschema.ErrColumnExists.GenWithStackByArgs(newColName) } + + // And also check the generated columns dependency, if some generated columns + // depend on this column, we can't rename the column name. + if errG != nil { + return nil, errors.Trace(errG) + } } // Constraints in the new column means adding new constraints. Errors should thrown, @@ -5144,6 +5170,11 @@ func GetModifiableColumnJob( if err = checkModifyGeneratedColumn(sctx, schema.Name, t, col, newCol, specNewColumn, spec.Position); err != nil { return nil, errors.Trace(err) } + if errG != nil { + // According to issue https://github.com/pingcap/tidb/issues/24321, + // changing the type of a column involving generating a column is prohibited. + return nil, dbterror.ErrUnsupportedOnGeneratedColumn.GenWithStackByArgs(errG.Error()) + } if t.Meta().TTLInfo != nil { // the column referenced by TTL should be a time type @@ -5409,21 +5440,10 @@ func (d *ddl) RenameColumn(ctx sessionctx.Context, ident ast.Ident, spec *ast.Al } // Check generated expression. - for _, col := range allCols { - if col.GeneratedExpr == nil { - continue - } - dependedColNames := FindColumnNamesInExpr(col.GeneratedExpr) - for _, name := range dependedColNames { - if name.Name.L == oldColName.L { - if col.Hidden { - return dbterror.ErrDependentByFunctionalIndex.GenWithStackByArgs(oldColName.O) - } - return dbterror.ErrDependentByGeneratedColumn.GenWithStackByArgs(oldColName.O) - } - } + err = checkModifyColumnWithGeneratedColumnsConstraint(allCols, oldColName) + if err != nil { + return errors.Trace(err) } - err = checkDropColumnWithPartitionConstraint(tbl, oldColName) if err != nil { return errors.Trace(err)