From c59a108c28b73012e2258f287a6228583463e945 Mon Sep 17 00:00:00 2001 From: Tanner Date: Thu, 13 Jun 2019 21:14:40 +0800 Subject: [PATCH] ddl, expression: Disallow add stored generated columns through ALTER TABLE (#10758) --- ddl/column.go | 4 ++-- ddl/db_test.go | 33 ++++++++++++++++----------------- ddl/ddl.go | 3 ++- ddl/ddl_api.go | 20 +++++++++++--------- ddl/generated_column.go | 19 +++++++++++-------- executor/ddl_test.go | 5 ++++- 6 files changed, 46 insertions(+), 38 deletions(-) diff --git a/ddl/column.go b/ddl/column.go index 3a0df926f62f1..c467d3d85eb90 100644 --- a/ddl/column.go +++ b/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 diff --git a/ddl/db_test.go b/ddl/db_test.go index 21d85abf38371..468829f452b9e 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -2026,24 +2026,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 `, `b int(11) YES VIRTUAL GENERATED`)) + result.Check(testkit.Rows(`a int(11) YES `, `b int(11) YES VIRTUAL GENERATED`, `c int(11) YES 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 `, `b int(11) YES VIRTUAL GENERATED`, `c int(11) YES 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`) @@ -2056,28 +2051,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) diff --git a/ddl/ddl.go b/ddl/ddl.go index 494693602651e..4f45c53b3f7c6 100644 --- a/ddl/ddl.go +++ b/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. diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 6c02fedd483a0..291a5b316687a 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -1962,7 +1962,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: @@ -1980,8 +1980,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) } @@ -2013,15 +2013,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) } } @@ -2638,7 +2640,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. @@ -2987,7 +2989,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. diff --git a/ddl/generated_column.go b/ddl/generated_column.go index efee4f1744af4..e0caa6ae6b075 100644 --- a/ddl/generated_column.go +++ b/ddl/generated_column.go @@ -41,7 +41,7 @@ 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) } } @@ -49,13 +49,16 @@ func verifyColumnGeneration(colName2Generation map[string]columnGenerationInDDL, 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 diff --git a/executor/ddl_test.go b/executor/ddl_test.go index d67ecbca3dfb3..35c1cdd0fa9f0 100644 --- a/executor/ddl_test.go +++ b/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;") }