From 475df093e943d7c6269b5d8f9b6add240c16f3fb Mon Sep 17 00:00:00 2001 From: tangenta Date: Tue, 25 Jun 2019 11:12:26 +0800 Subject: [PATCH 1/6] disallow modifying virtual columns with index --- ddl/db_test.go | 21 +++++++++++++++++++++ ddl/ddl_api.go | 5 +++++ 2 files changed, 26 insertions(+) diff --git a/ddl/db_test.go b/ddl/db_test.go index 1d3cb29ad77c..cd4cd7b8a600 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -2811,6 +2811,27 @@ func (s *testDBSuite5) TestAddIndexForGeneratedColumn(c *C) { s.tk.MustExec("admin check table gcai_table") } +func (s *testDBSuite5) TestModifyIndexedGeneratedColumn(c *C) { + tk := testkit.NewTestKit(c, s.store) + tk.MustExec("use test") + + // Test create an exist database + _, err := tk.Exec("CREATE database test") + c.Assert(err, NotNil) + + 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, "[ddl:3106]'modifying an indexed column' is not supported for generated columns.") + + tk.MustExec("drop index idx on t1;") + tk.MustExec("alter table t1 modify b int as (a + 2);") + + s.mustExec(c, "drop table t1;") +} + func (s *testDBSuite4) TestIssue9100(c *C) { tk := testkit.NewTestKit(c, s.store) tk.MustExec("use test_db") diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 829f1297bf95..19da6a4f7e9d 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -2705,6 +2705,11 @@ func (d *ddl) ModifyColumn(ctx sessionctx.Context, ident ast.Ident, spec *ast.Al if err := checkAutoIncrementRef(specNewColumn.Name.Name.L, dependColNames, t.Meta()); err != nil { return errors.Trace(err) } + + // If there is an index on the generated column we are modifying, throw an error + if tables.FindIndexByColName(t, specNewColumn.Name.Name.L) != nil { + return errUnsupportedOnGeneratedColumn.GenWithStackByArgs("modifying an indexed column") + } } } From cd63ec082d4a957ed7923faff48355d193a1bf6e Mon Sep 17 00:00:00 2001 From: tangenta Date: Tue, 25 Jun 2019 11:25:45 +0800 Subject: [PATCH 2/6] update comment --- ddl/ddl_api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 19da6a4f7e9d..7cdd809d3332 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -2706,7 +2706,7 @@ func (d *ddl) ModifyColumn(ctx sessionctx.Context, ident ast.Ident, spec *ast.Al return errors.Trace(err) } - // If there is an index on the generated column we are modifying, throw an error + // If there is an index on the generated column which we are trying to modify, return an error. if tables.FindIndexByColName(t, specNewColumn.Name.Name.L) != nil { return errUnsupportedOnGeneratedColumn.GenWithStackByArgs("modifying an indexed column") } From 004a19714d95ed525b66c16f9f22833f59888426 Mon Sep 17 00:00:00 2001 From: tangenta Date: Tue, 25 Jun 2019 14:38:14 +0800 Subject: [PATCH 3/6] address comment --- ddl/db_test.go | 24 +++++++++++++++--------- ddl/ddl_api.go | 11 +++++++++-- table/tables/tables.go | 2 +- 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/ddl/db_test.go b/ddl/db_test.go index cd4cd7b8a600..120e5038220a 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -2813,23 +2813,29 @@ func (s *testDBSuite5) TestAddIndexForGeneratedColumn(c *C) { func (s *testDBSuite5) TestModifyIndexedGeneratedColumn(c *C) { tk := testkit.NewTestKit(c, s.store) + tk.MustExec("create database if not exists test") tk.MustExec("use test") - - // Test create an exist database - _, err := tk.Exec("CREATE database test") - c.Assert(err, NotNil) + modIdxColErrMsg := "[ddl:3106]'modifying an indexed column' is not supported for generated columns." 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)") + _, err := tk.Exec("alter table t1 modify column b int as (a+2)") c.Assert(err, NotNil) - c.Assert(err.Error(), Equals, "[ddl:3106]'modifying an indexed column' is not supported for generated columns.") - + 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.MustExec("alter table t1 modify b int as (a+2);") + tk.MustQuery("select * from t1").Check(testkit.Rows("1 3")) - s.mustExec(c, "drop table t1;") + 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")) } func (s *testDBSuite4) TestIssue9100(c *C) { diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 7cdd809d3332..bac5ca1faf60 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -2707,8 +2707,15 @@ func (d *ddl) ModifyColumn(ctx sessionctx.Context, ident ast.Ident, spec *ast.Al } // If there is an index on the generated column which we are trying to modify, return an error. - if tables.FindIndexByColName(t, specNewColumn.Name.Name.L) != nil { - return errUnsupportedOnGeneratedColumn.GenWithStackByArgs("modifying an indexed column") + for _, idx := range t.Indices() { + if idx.Meta().State != model.StatePublic { + continue + } + for _, col := range idx.Meta().Columns { + if col.Name.L == specNewColumn.Name.Name.L { + return errUnsupportedOnGeneratedColumn.GenWithStackByArgs("modifying an indexed column") + } + } } } } diff --git a/table/tables/tables.go b/table/tables/tables.go index c02772ce4d95..c9d09841a073 100644 --- a/table/tables/tables.go +++ b/table/tables/tables.go @@ -1046,7 +1046,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. From d0d71bf478fb2904c7a365918c0a2a90d6dcf551 Mon Sep 17 00:00:00 2001 From: tangenta Date: Tue, 25 Jun 2019 15:06:11 +0800 Subject: [PATCH 4/6] check all indices --- ddl/ddl_api.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index bac5ca1faf60..bd12f2cd74af 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -2708,9 +2708,6 @@ func (d *ddl) ModifyColumn(ctx sessionctx.Context, ident ast.Ident, spec *ast.Al // If there is an index on the generated column which we are trying to modify, return an error. for _, idx := range t.Indices() { - if idx.Meta().State != model.StatePublic { - continue - } for _, col := range idx.Meta().Columns { if col.Name.L == specNewColumn.Name.Name.L { return errUnsupportedOnGeneratedColumn.GenWithStackByArgs("modifying an indexed column") From 199fa3149e8d9c6db0665e5db154da971b1478dc Mon Sep 17 00:00:00 2001 From: tangenta Date: Fri, 28 Jun 2019 16:03:38 +0800 Subject: [PATCH 5/6] address comment --- ddl/db_test.go | 53 +++++++++++++++++++++++++++++++++------ ddl/ddl_api.go | 25 +------------------ ddl/generated_column.go | 55 ++++++++++++++++++++++++++++++++--------- 3 files changed, 90 insertions(+), 43 deletions(-) diff --git a/ddl/db_test.go b/ddl/db_test.go index 120e5038220a..3292ced9cad0 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -2126,13 +2126,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`) @@ -2811,31 +2815,64 @@ func (s *testDBSuite5) TestAddIndexForGeneratedColumn(c *C) { s.tk.MustExec("admin check table gcai_table") } -func (s *testDBSuite5) TestModifyIndexedGeneratedColumn(c *C) { +func (s *testDBSuite5) TestModifyGeneratedColumn(c *C) { tk := testkit.NewTestKit(c, s.store) - tk.MustExec("create database if not exists test") + 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)") + 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") + 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 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) { diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index bd12f2cd74af..176de8c9733f 100644 --- a/ddl/ddl_api.go +++ b/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,29 +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) - } - - // If there is an index on the generated column which we are trying to modify, return an error. - for _, idx := range t.Indices() { - for _, col := range idx.Meta().Columns { - if col.Name.L == specNewColumn.Name.Name.L { - return errUnsupportedOnGeneratedColumn.GenWithStackByArgs("modifying an indexed column") - } - } - } - } - } - 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 e0caa6ae6b07..98e67639ce5f 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,30 @@ 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 newCol.GeneratedStored && oldCol.GeneratedExprString != newCol.GeneratedExprString { + 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 { From bd4af136f604cc4d113ecbeb965b49d474ee8507 Mon Sep 17 00:00:00 2001 From: tangenta Date: Fri, 28 Jun 2019 16:31:43 +0800 Subject: [PATCH 6/6] address comment --- ddl/db_test.go | 8 ++++++++ ddl/generated_column.go | 6 +++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/ddl/db_test.go b/ddl/db_test.go index a3e2072c729b..ccdcfbdf7f9d 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -2860,6 +2860,14 @@ func (s *testDBSuite5) TestModifyGeneratedColumn(c *C) { 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);") diff --git a/ddl/generated_column.go b/ddl/generated_column.go index 98e67639ce5f..e9ee788ff033 100644 --- a/ddl/generated_column.go +++ b/ddl/generated_column.go @@ -217,7 +217,11 @@ func checkGeneratedWithAutoInc(tableInfo *model.TableInfo, newColumnDef *ast.Col } func checkIndexOrStored(tbl table.Table, oldCol, newCol *table.Column) error { - if newCol.GeneratedStored && oldCol.GeneratedExprString != newCol.GeneratedExprString { + if oldCol.GeneratedExprString == newCol.GeneratedExprString { + return nil + } + + if newCol.GeneratedStored { return errUnsupportedOnGeneratedColumn.GenWithStackByArgs("modifying a stored column") }