From 8dff08a88f5fb54e13632d396962d0e227c4be91 Mon Sep 17 00:00:00 2001 From: Tanner Date: Thu, 4 Jul 2019 21:46:27 +0800 Subject: [PATCH] =?UTF-8?q?ddl:=20disallow=20modifying=20the=20generated?= =?UTF-8?q?=20expression=20of=20stored=20or=E2=80=A6=20(#11068)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ddl/db_test.go | 80 ++++++++++++++++++++++++++++++++++++++--- ddl/ddl_api.go | 16 +-------- ddl/generated_column.go | 59 ++++++++++++++++++++++++------ table/tables/tables.go | 2 +- 4 files changed, 126 insertions(+), 31 deletions(-) diff --git a/ddl/db_test.go b/ddl/db_test.go index df9b12597f02c..55ba11897f0f5 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -2121,13 +2121,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 `, `b int(11) YES VIRTUAL GENERATED`, `c bigint(20) YES STORED GENERATED`)) + result.Check(testkit.Rows(`a int(11) YES `, `b int(11) YES VIRTUAL GENERATED`, `c int(11) YES 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 `, `b bigint(20) YES VIRTUAL GENERATED`, `c bigint(20) YES STORED GENERATED`)) + result.Check(testkit.Rows(`a int(11) YES `, `b bigint(20) YES VIRTUAL GENERATED`, `c int(11) YES STORED GENERATED`)) s.tk.MustExec(`alter table test_gv_ddl change column c cnew bigint`) result = s.tk.MustQuery(`DESC test_gv_ddl`) @@ -2720,7 +2724,75 @@ func (s *testDBSuite5) TestAddIndexForGeneratedColumn(c *C) { s.tk.MustExec("admin check table gcai_table") } -func (s *testDBSuite6) TestIssue9100(c *C) { +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") tk.MustExec("create table employ (a int, b int) partition by range (b) (partition p0 values less than (1));") diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 9900536c9ad7e..996a7f69831fe 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -2551,7 +2551,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) } @@ -2604,20 +2604,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 { diff --git a/ddl/generated_column.go b/ddl/generated_column.go index e0caa6ae6b075..e9ee788ff033b 100644 --- a/ddl/generated_column.go +++ b/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 { diff --git a/table/tables/tables.go b/table/tables/tables.go index d7ed18fbe3f97..b8eb936aefe1e 100644 --- a/table/tables/tables.go +++ b/table/tables/tables.go @@ -1047,7 +1047,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.