ddl: disallow modifying the generated expression of stored or indexed…

… column (#10932)

ddl/db_test.go

@@ -2127,13 +2127,17 @@ func (s *testDBSuite3) TestGeneratedColumnDDL(c *C) {
 	}
 
 	// Check alter table modify/change generated column.
-	s.tk.MustExec(`alter table test_gv_ddl modify column c bigint as (b+200) stored`)
+	modStoredColErrMsg := "[ddl:3106]'modifying a stored column' is not supported for generated columns."
+	_, err := s.tk.Exec(`alter table test_gv_ddl modify column c bigint as (b+200) stored`)
+	c.Assert(err, NotNil)
+	c.Assert(err.Error(), Equals, modStoredColErrMsg)
+
 	result = s.tk.MustQuery(`DESC test_gv_ddl`)
-	result.Check(testkit.Rows(`a int(11) YES  <nil> `, `b int(11) YES  <nil> VIRTUAL GENERATED`, `c bigint(20) YES  <nil> STORED GENERATED`))
+	result.Check(testkit.Rows(`a int(11) YES  <nil> `, `b int(11) YES  <nil> VIRTUAL GENERATED`, `c int(11) YES  <nil> STORED GENERATED`))
 
 	s.tk.MustExec(`alter table test_gv_ddl change column b b bigint as (a+100) virtual`)
 	result = s.tk.MustQuery(`DESC test_gv_ddl`)
-	result.Check(testkit.Rows(`a int(11) YES  <nil> `, `b bigint(20) YES  <nil> VIRTUAL GENERATED`, `c bigint(20) YES  <nil> STORED GENERATED`))
+	result.Check(testkit.Rows(`a int(11) YES  <nil> `, `b bigint(20) YES  <nil> VIRTUAL GENERATED`, `c int(11) YES  <nil> STORED GENERATED`))
 
 	s.tk.MustExec(`alter table test_gv_ddl change column c cnew bigint`)
 	result = s.tk.MustQuery(`DESC test_gv_ddl`)
@@ -2838,6 +2842,74 @@ func (s *testDBSuite5) TestAddIndexForGeneratedColumn(c *C) {
 	s.tk.MustExec("admin check table gcai_table")
 }
 
+func (s *testDBSuite5) TestModifyGeneratedColumn(c *C) {
+	tk := testkit.NewTestKit(c, s.store)
+	tk.MustExec("create database if not exists test;")
+	tk.MustExec("use test")
+	modIdxColErrMsg := "[ddl:3106]'modifying an indexed column' is not supported for generated columns."
+	modStoredColErrMsg := "[ddl:3106]'modifying a stored column' is not supported for generated columns."
+
+	// Modify column with single-col-index.
+	tk.MustExec("drop table if exists t1;")
+	tk.MustExec("create table t1 (a int, b int as (a+1), index idx(b));")
+	tk.MustExec("insert into t1 set a=1;")
+	_, err := tk.Exec("alter table t1 modify column b int as (a+2);")
+	c.Assert(err, NotNil)
+	c.Assert(err.Error(), Equals, modIdxColErrMsg)
+	tk.MustExec("drop index idx on t1;")
+	tk.MustExec("alter table t1 modify b int as (a+2);")
+	tk.MustQuery("select * from t1").Check(testkit.Rows("1 3"))
+
+	// Modify column with multi-col-index.
+	tk.MustExec("drop table t1;")
+	tk.MustExec("create table t1 (a int, b int as (a+1), index idx(a, b));")
+	tk.MustExec("insert into t1 set a=1;")
+	_, err = tk.Exec("alter table t1 modify column b int as (a+2);")
+	c.Assert(err, NotNil)
+	c.Assert(err.Error(), Equals, modIdxColErrMsg)
+	tk.MustExec("drop index idx on t1;")
+	tk.MustExec("alter table t1 modify b int as (a+2);")
+	tk.MustQuery("select * from t1").Check(testkit.Rows("1 3"))
+
+	// Modify column with stored status to a different expression.
+	tk.MustExec("drop table t1;")
+	tk.MustExec("create table t1 (a int, b int as (a+1) stored);")
+	tk.MustExec("insert into t1 set a=1;")
+	_, err = tk.Exec("alter table t1 modify column b int as (a+2) stored;")
+	c.Assert(err, NotNil)
+	c.Assert(err.Error(), Equals, modStoredColErrMsg)
+
+	// Modify column with stored status to the same expression.
+	tk.MustExec("drop table t1;")
+	tk.MustExec("create table t1 (a int, b int as (a+1) stored);")
+	tk.MustExec("insert into t1 set a=1;")
+	tk.MustExec("alter table t1 modify column b bigint as (a+1) stored;")
+	tk.MustExec("alter table t1 modify column b bigint as (a + 1) stored;")
+	tk.MustQuery("select * from t1").Check(testkit.Rows("1 2"))
+
+	// Modify column with index to the same expression.
+	tk.MustExec("drop table t1;")
+	tk.MustExec("create table t1 (a int, b int as (a+1), index idx(b));")
+	tk.MustExec("insert into t1 set a=1;")
+	tk.MustExec("alter table t1 modify column b bigint as (a+1);")
+	tk.MustExec("alter table t1 modify column b bigint as (a + 1);")
+	tk.MustQuery("select * from t1").Check(testkit.Rows("1 2"))
+
+	// Modify column from non-generated to stored generated.
+	tk.MustExec("drop table t1;")
+	tk.MustExec("create table t1 (a int, b int);")
+	_, err = tk.Exec("alter table t1 modify column b bigint as (a+1) stored;")
+	c.Assert(err, NotNil)
+	c.Assert(err.Error(), Equals, modStoredColErrMsg)
+
+	// Modify column from stored generated to non-generated.
+	tk.MustExec("drop table t1;")
+	tk.MustExec("create table t1 (a int, b int as (a+1) stored);")
+	tk.MustExec("insert into t1 set a=1;")
+	tk.MustExec("alter table t1 modify column b int;")
+	tk.MustQuery("select * from t1").Check(testkit.Rows("1 2"))
+}
+
 func (s *testDBSuite4) TestIssue9100(c *C) {
 	tk := testkit.NewTestKit(c, s.store)
 	tk.MustExec("use test_db")

ddl/ddl_api.go

@@ -2632,7 +2632,7 @@ func (d *ddl) getModifiableColumnJob(ctx sessionctx.Context, ident ast.Ident, or
 	}
 
 	// As same with MySQL, we don't support modifying the stored status for generated columns.
-	if err = checkModifyGeneratedColumn(t.Cols(), col, newCol); err != nil {
+	if err = checkModifyGeneratedColumn(t, col, newCol, specNewColumn); err != nil {
 		return nil, errors.Trace(err)
 	}
 
@@ -2694,20 +2694,6 @@ func (d *ddl) ModifyColumn(ctx sessionctx.Context, ident ast.Ident, spec *ast.Al
 		return ErrWrongTableName.GenWithStackByArgs(specNewColumn.Name.Table.O)
 	}
 
-	// If the modified column is generated, check whether it refers to any auto-increment columns.
-	for _, option := range specNewColumn.Options {
-		if option.Tp == ast.ColumnOptionGenerated {
-			_, t, err := d.getSchemaAndTableByIdent(ctx, ident)
-			if err != nil {
-				return errors.Trace(err)
-			}
-			_, dependColNames := findDependedColumnNames(specNewColumn)
-			if err := checkAutoIncrementRef(specNewColumn.Name.Name.L, dependColNames, t.Meta()); err != nil {
-				return errors.Trace(err)
-			}
-		}
-	}
-
 	originalColName := specNewColumn.Name.Name
 	job, err := d.getModifiableColumnJob(ctx, ident, originalColName, spec)
 	if err != nil {

ddl/generated_column.go

@@ -36,7 +36,7 @@ func verifyColumnGeneration(colName2Generation map[string]columnGenerationInDDL,
 			if attr, ok := colName2Generation[depCol]; ok {
 				if attr.generated && attribute.position <= attr.position {
 					// A generated column definition can refer to other
-					// generated columns occurring earilier in the table.
+					// generated columns occurring earlier in the table.
 					err := errGeneratedColumnNonPrior.GenWithStackByArgs()
 					return errors.Trace(err)
 				}
@@ -109,19 +109,18 @@ func (c *generatedColumnChecker) Leave(inNode ast.Node) (node ast.Node, ok bool)
 //  1. the modification can't change stored status;
 //  2. if the new is generated, check its refer rules.
 //  3. check if the modified expr contains non-deterministic functions
-func checkModifyGeneratedColumn(originCols []*table.Column, oldCol, newCol *table.Column) error {
+//  4. check whether new column refers to any auto-increment columns.
+//  5. check if the new column is indexed or stored
+func checkModifyGeneratedColumn(tbl table.Table, oldCol, newCol *table.Column, newColDef *ast.ColumnDef) error {
 	// rule 1.
-	var stored = [2]bool{false, false}
-	var cols = [2]*table.Column{oldCol, newCol}
-	for i, col := range cols {
-		if !col.IsGenerated() || col.GeneratedStored {
-			stored[i] = true
-		}
-	}
-	if stored[0] != stored[1] {
+	oldColIsStored := !oldCol.IsGenerated() || oldCol.GeneratedStored
+	newColIsStored := !newCol.IsGenerated() || newCol.GeneratedStored
+	if oldColIsStored != newColIsStored {
 		return errUnsupportedOnGeneratedColumn.GenWithStackByArgs("Changing the STORED status")
 	}
+
 	// rule 2.
+	originCols := tbl.Cols()
 	var colName2Generation = make(map[string]columnGenerationInDDL, len(originCols))
 	for i, column := range originCols {
 		// We can compare the pointers simply.
@@ -158,11 +157,21 @@ func checkModifyGeneratedColumn(originCols []*table.Column, oldCol, newCol *tabl
 		}
 	}
 
-	// rule 3
 	if newCol.IsGenerated() {
+		// rule 3.
 		if err := checkIllegalFn4GeneratedColumn(newCol.Name.L, newCol.GeneratedExpr); err != nil {
 			return errors.Trace(err)
 		}
+
+		// rule 4.
+		if err := checkGeneratedWithAutoInc(tbl.Meta(), newColDef); err != nil {
+			return errors.Trace(err)
+		}
+
+		// rule 5.
+		if err := checkIndexOrStored(tbl, oldCol, newCol); err != nil {
+			return errors.Trace(err)
+		}
 	}
 	return nil
 }
@@ -198,6 +207,34 @@ func checkIllegalFn4GeneratedColumn(colName string, expr ast.ExprNode) error {
 	return nil
 }
 
+// Check whether newColumnDef refers to any auto-increment columns.
+func checkGeneratedWithAutoInc(tableInfo *model.TableInfo, newColumnDef *ast.ColumnDef) error {
+	_, dependColNames := findDependedColumnNames(newColumnDef)
+	if err := checkAutoIncrementRef(newColumnDef.Name.Name.L, dependColNames, tableInfo); err != nil {
+		return errors.Trace(err)
+	}
+	return nil
+}
+
+func checkIndexOrStored(tbl table.Table, oldCol, newCol *table.Column) error {
+	if oldCol.GeneratedExprString == newCol.GeneratedExprString {
+		return nil
+	}
+
+	if newCol.GeneratedStored {
+		return errUnsupportedOnGeneratedColumn.GenWithStackByArgs("modifying a stored column")
+	}
+
+	for _, idx := range tbl.Indices() {
+		for _, col := range idx.Meta().Columns {
+			if col.Name.L == newCol.Name.L {
+				return errUnsupportedOnGeneratedColumn.GenWithStackByArgs("modifying an indexed column")
+			}
+		}
+	}
+	return nil
+}
+
 // checkAutoIncrementRef checks if an generated column depends on an auto-increment column and raises an error if so.
 // See https://dev.mysql.com/doc/refman/5.7/en/create-table-generated-columns.html for details.
 func checkAutoIncrementRef(name string, dependencies map[string]struct{}, tbInfo *model.TableInfo) error {

table/tables/tables.go

@@ -1059,7 +1059,7 @@ var (
 	recordPrefixSep = []byte("_r")
 )
 
-// FindIndexByColName implements table.Table FindIndexByColName interface.
+// FindIndexByColName returns a public table index containing only one column named `name`.
 func FindIndexByColName(t table.Table, name string) table.Index {
 	for _, idx := range t.Indices() {
 		// only public index can be read.