ddl: fix an error message for 'alter table' change column with genera…

…ted columns dependency (#43350)

close #24321, close #38988

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)
+}

ddl/column_modify_test.go

@@ -538,15 +538,15 @@ 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},
  {`alter table test_gv_ddl change column c cnew bigint as (a+100)`, errno.ErrUnsupportedOnGeneratedColumn},
 
  // 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 <nil> `, `b int(11) YES <nil> VIRTUAL GENERATED`, `c int(11) YES <nil> 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 <nil> `, `b bigint(20) YES <nil> VIRTUAL GENERATED`, `c int(11) YES <nil> 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 <nil> `, `b bigint(20) YES <nil> VIRTUAL GENERATED`, `c int(11) YES <nil> 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 <nil> `, `b bigint(20) YES <nil> VIRTUAL GENERATED`, `cnew bigint(20) YES <nil> `))
+ result.Check(testkit.Rows(`a int(11) YES <nil> `, `b int(11) YES <nil> VIRTUAL GENERATED`, `cnew bigint(20) YES <nil> `))
 
  // Test generated column `\\`.
  tk.MustExec("drop table if exists t")

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)