ddl, expression: disallow add stored generated columns through ALTER TABLE (#10758)

ddl/column.go

@@ -519,8 +519,8 @@ func allocateColumnID(tblInfo *model.TableInfo) int64 {
 	return tblInfo.MaxColumnID
 }
 
-func checkAddColumnTooManyColumns(oldCols int) error {
-	if uint32(oldCols) > atomic.LoadUint32(&TableColumnCountLimit) {
+func checkAddColumnTooManyColumns(colNum int) error {
+	if uint32(colNum) > atomic.LoadUint32(&TableColumnCountLimit) {
 		return errTooManyFields
 	}
 	return nil

ddl/db_test.go

@@ -2025,24 +2025,19 @@ func (s *testDBSuite3) TestGeneratedColumnDDL(c *C) {
 	s.tk = testkit.NewTestKit(c, s.store)
 	s.tk.MustExec("use test")
 
-	// Check create table with virtual generated column.
-	s.tk.MustExec(`CREATE TABLE test_gv_ddl(a int, b int as (a+8) virtual)`)
+	// Check create table with virtual and stored generated columns.
+	s.tk.MustExec(`CREATE TABLE test_gv_ddl(a int, b int as (a+8) virtual, c int as (b + 2) stored)`)
 
-	// Check desc table with virtual generated column.
+	// Check desc table with virtual and stored generated columns.
 	result := s.tk.MustQuery(`DESC test_gv_ddl`)
-	result.Check(testkit.Rows(`a int(11) YES  <nil> `, `b int(11) YES  <nil> VIRTUAL GENERATED`))
+	result.Check(testkit.Rows(`a int(11) YES  <nil> `, `b int(11) YES  <nil> VIRTUAL GENERATED`, `c int(11) YES  <nil> STORED GENERATED`))
 
-	// Check show create table with virtual generated column.
+	// Check show create table with virtual and stored generated columns.
 	result = s.tk.MustQuery(`show create table test_gv_ddl`)
 	result.Check(testkit.Rows(
-		"test_gv_ddl CREATE TABLE `test_gv_ddl` (\n  `a` int(11) DEFAULT NULL,\n  `b` int(11) GENERATED ALWAYS AS (`a` + 8) VIRTUAL\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin",
+		"test_gv_ddl CREATE TABLE `test_gv_ddl` (\n  `a` int(11) DEFAULT NULL,\n  `b` int(11) GENERATED ALWAYS AS (`a` + 8) VIRTUAL,\n  `c` int(11) GENERATED ALWAYS AS (`b` + 2) STORED\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin",
 	))
 
-	// Check alter table add a stored generated column.
-	s.tk.MustExec(`alter table test_gv_ddl add column c int as (b+2) stored`)
-	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 int(11) YES  <nil> STORED GENERATED`))
-
 	// Check generated expression with blanks.
 	s.tk.MustExec("create table table_with_gen_col_blanks (a int, b char(20) as (cast( \r\n\t a \r\n\tas  char)))")
 	result = s.tk.MustQuery(`show create table table_with_gen_col_blanks`)
@@ -2055,28 +2050,32 @@ func (s *testDBSuite3) TestGeneratedColumnDDL(c *C) {
 		stmt string
 		err  int
 	}{
-		// drop/rename columns dependent by other column.
+		// Drop/rename columns dependent by other column.
 		{`alter table test_gv_ddl drop column a`, mysql.ErrDependentByGeneratedColumn},
 		{`alter table test_gv_ddl change column a anew int`, mysql.ErrBadField},
 
-		// modify/change stored status of generated columns.
+		// Modify/change stored status of generated columns.
 		{`alter table test_gv_ddl modify column b bigint`, mysql.ErrUnsupportedOnGeneratedColumn},
 		{`alter table test_gv_ddl change column c cnew bigint as (a+100)`, mysql.ErrUnsupportedOnGeneratedColumn},
 
-		// modify/change generated columns breaking prior.
+		// Modify/change generated columns breaking prior.
 		{`alter table test_gv_ddl modify column b int as (c+100)`, mysql.ErrGeneratedColumnNonPrior},
 		{`alter table test_gv_ddl change column b bnew int as (c+100)`, mysql.ErrGeneratedColumnNonPrior},
 
-		// refer not exist columns in generation expression.
+		// Refer not exist columns in generation expression.
 		{`create table test_gv_ddl_bad (a int, b int as (c+8))`, mysql.ErrBadField},
 
-		// refer generated columns non prior.
+		// Refer generated columns non prior.
 		{`create table test_gv_ddl_bad (a int, b int as (c+1), c int as (a+1))`, mysql.ErrGeneratedColumnNonPrior},
 
-		// virtual generated columns cannot be primary key.
+		// Virtual generated columns cannot be primary key.
 		{`create table test_gv_ddl_bad (a int, b int, c int as (a+b) primary key)`, mysql.ErrUnsupportedOnGeneratedColumn},
 		{`create table test_gv_ddl_bad (a int, b int, c int as (a+b), primary key(c))`, mysql.ErrUnsupportedOnGeneratedColumn},
 		{`create table test_gv_ddl_bad (a int, b int, c int as (a+b), primary key(a, c))`, mysql.ErrUnsupportedOnGeneratedColumn},
+
+		// Add stored generated column through alter table.
+		{`alter table test_gv_ddl add column d int as (b+2) stored`, mysql.ErrUnsupportedOnGeneratedColumn},
+		{`alter table test_gv_ddl modify column b int as (a + 8) stored`, mysql.ErrUnsupportedOnGeneratedColumn},
 	}
 	for _, tt := range genExprTests {
 		assertErrorCode(c, s.tk, tt.stmt, tt.err)

ddl/ddl.go

@@ -109,7 +109,6 @@ var (
 	errInvalidJobVersion        = terror.ClassDDL.New(codeInvalidJobVersion, "DDL job with version %d greater than current %d")
 	errFileNotFound             = terror.ClassDDL.New(codeFileNotFound, "Can't find file: './%s/%s.frm'")
 	errErrorOnRename            = terror.ClassDDL.New(codeErrorOnRename, "Error on rename of './%s/%s' to './%s/%s'")
-	errBadField                 = terror.ClassDDL.New(codeBadField, "Unknown column '%s' in '%s'")
 	errInvalidUseOfNull         = terror.ClassDDL.New(codeInvalidUseOfNull, "Invalid use of NULL value")
 	errTooManyFields            = terror.ClassDDL.New(codeTooManyFields, "Too many columns")
 	errInvalidSplitRegionRanges = terror.ClassDDL.New(codeInvalidRanges, "Failed to split region ranges")
@@ -161,6 +160,8 @@ var (
 
 	// ErrColumnBadNull returns for a bad null value.
 	ErrColumnBadNull = terror.ClassDDL.New(codeBadNull, "column cann't be null")
+	// ErrBadField forbids to refer to unknown column.
+	ErrBadField = terror.ClassDDL.New(codeBadField, "Unknown column '%s' in '%s'")
 	// ErrCantRemoveAllFields returns for deleting all columns.
 	ErrCantRemoveAllFields = terror.ClassDDL.New(codeCantRemoveAllFields, "can't delete all columns with ALTER TABLE")
 	// ErrCantDropFieldOrKey returns for dropping a non-existent field or key.

ddl/ddl_api.go

@@ -1940,7 +1940,7 @@ func (d *ddl) getSchemaAndTableByIdent(ctx sessionctx.Context, tableIdent ast.Id
 	return schema, t, nil
 }
 
-func checkColumnConstraint(col *ast.ColumnDef, ti ast.Ident) error {
+func checkUnsupportedColumnConstraint(col *ast.ColumnDef, ti ast.Ident) error {
 	for _, constraint := range col.Options {
 		switch constraint.Tp {
 		case ast.ColumnOptionAutoIncrement:
@@ -1958,8 +1958,8 @@ func checkColumnConstraint(col *ast.ColumnDef, ti ast.Ident) error {
 // AddColumn will add a new column to the table.
 func (d *ddl) AddColumn(ctx sessionctx.Context, ti ast.Ident, spec *ast.AlterTableSpec) error {
 	specNewColumn := spec.NewColumns[0]
-	// Check whether the added column constraints are supported.
-	err := checkColumnConstraint(specNewColumn, ti)
+
+	err := checkUnsupportedColumnConstraint(specNewColumn, ti)
 	if err != nil {
 		return errors.Trace(err)
 	}
@@ -1991,15 +1991,17 @@ func (d *ddl) AddColumn(ctx sessionctx.Context, ti ast.Ident, spec *ast.AlterTab
 			if err := checkIllegalFn4GeneratedColumn(specNewColumn.Name.Name.L, option.Expr); err != nil {
 				return errors.Trace(err)
 			}
-			referableColNames := make(map[string]struct{}, len(t.Cols()))
-			for _, col := range t.Cols() {
-				referableColNames[col.Name.L] = struct{}{}
+
+			if option.Stored {
+				return errUnsupportedOnGeneratedColumn.GenWithStackByArgs("Adding generated stored column through ALTER TABLE")
 			}
+
 			_, dependColNames := findDependedColumnNames(specNewColumn)
 			if err = checkAutoIncrementRef(specNewColumn.Name.Name.L, dependColNames, t.Meta()); err != nil {
 				return errors.Trace(err)
 			}
-			if err = columnNamesCover(referableColNames, dependColNames); err != nil {
+
+			if err = checkDependedColExist(dependColNames, t.Cols()); err != nil {
 				return errors.Trace(err)
 			}
 		}
@@ -2605,7 +2607,7 @@ func (d *ddl) AlterColumn(ctx sessionctx.Context, ident ast.Ident, spec *ast.Alt
 	// Check whether alter column has existed.
 	col := table.FindCol(t.Cols(), colName.L)
 	if col == nil {
-		return errBadField.GenWithStackByArgs(colName, ident.Name)
+		return ErrBadField.GenWithStackByArgs(colName, ident.Name)
 	}
 
 	// Clean the NoDefaultValueFlag value.
@@ -2953,7 +2955,7 @@ func (d *ddl) CreateIndex(ctx sessionctx.Context, ti ast.Ident, unique bool, ind
 	}
 
 	tblInfo := t.Meta()
-	// Check before put the job is put to the queue.
+	// Check before the job is put to the queue.
 	// This check is redudant, but useful. If DDL check fail before the job is put
 	// to job queue, the fail path logic is super fast.
 	// After DDL job is put to the queue, and if the check fail, TiDB will run the DDL cancel logic.

ddl/generated_column.go

@@ -41,21 +41,24 @@ func verifyColumnGeneration(colName2Generation map[string]columnGenerationInDDL,
 					return errors.Trace(err)
 				}
 			} else {
-				err := errBadField.GenWithStackByArgs(depCol, "generated column function")
+				err := ErrBadField.GenWithStackByArgs(depCol, "generated column function")
 				return errors.Trace(err)
 			}
 		}
 	}
 	return nil
 }
 
-// columnNamesCover checks whether dependColNames is covered by normalColNames or not.
-// it's only for alter table add column because before alter, we can make sure that all
-// columns in table are verified already.
-func columnNamesCover(normalColNames map[string]struct{}, dependColNames map[string]struct{}) error {
-	for name := range dependColNames {
-		if _, ok := normalColNames[name]; !ok {
-			return errBadField.GenWithStackByArgs(name, "generated column function")
+// checkDependedColExist ensure all depended columns exist.
+//
+// NOTE: this will MODIFY parameter `dependCols`.
+func checkDependedColExist(dependCols map[string]struct{}, cols []*table.Column) error {
+	for _, col := range cols {
+		delete(dependCols, col.Name.L)
+	}
+	if len(dependCols) != 0 {
+		for arbitraryCol := range dependCols {
+			return ErrBadField.GenWithStackByArgs(arbitraryCol, "generated column function")
 		}
 	}
 	return nil

executor/ddl_test.go

@@ -778,11 +778,14 @@ func (s *testSuite3) TestGeneratedColumnRelatedDDL(c *C) {
 	_, err = tk.Exec("alter table t1 add column d bigint generated always as (a + 1);")
 	c.Assert(err.Error(), Equals, ddl.ErrGeneratedColumnRefAutoInc.GenWithStackByArgs("d").Error())
 
-	tk.MustExec("alter table t1 add column d bigint generated always as (b + 1); ")
+	tk.MustExec("alter table t1 add column d bigint generated always as (b + 1);")
 
 	_, err = tk.Exec("alter table t1 modify column d bigint generated always as (a + 1);")
 	c.Assert(err.Error(), Equals, ddl.ErrGeneratedColumnRefAutoInc.GenWithStackByArgs("d").Error())
 
+	_, err = tk.Exec("alter table t1 add column e bigint as (z + 1);")
+	c.Assert(err.Error(), Equals, ddl.ErrBadField.GenWithStackByArgs("z", "generated column function").Error())
+
 	tk.MustExec("drop table t1;")
 }