From 5254f03356b0d0e39592dc423e02ad16de77df5d Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Mon, 20 May 2019 20:33:04 +0800 Subject: [PATCH 1/2] ddl: validate table info before doing ddl job to avoid load infoschema error (#10464) --- ddl/column.go | 6 +++--- ddl/db_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ ddl/ddl_api.go | 14 ++++++++++++++ ddl/index.go | 2 +- ddl/table.go | 22 +++++++++++++++++++++- 5 files changed, 79 insertions(+), 5 deletions(-) diff --git a/ddl/column.go b/ddl/column.go index 8535c9d7adc9e..643135a9a1067 100644 --- a/ddl/column.go +++ b/ddl/column.go @@ -182,7 +182,7 @@ func onAddColumn(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64, err error) // none -> delete only job.SchemaState = model.StateDeleteOnly columnInfo.State = model.StateDeleteOnly - ver, err = updateVersionAndTableInfo(t, job, tblInfo, originalState != columnInfo.State) + ver, err = updateVersionAndTableInfoWithCheck(t, job, tblInfo, originalState != columnInfo.State) case model.StateDeleteOnly: // delete only -> write only job.SchemaState = model.StateWriteOnly @@ -260,7 +260,7 @@ func onDropColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { return ver, errors.Trace(err) } } - ver, err = updateVersionAndTableInfo(t, job, tblInfo, originalState != colInfo.State) + ver, err = updateVersionAndTableInfoWithCheck(t, job, tblInfo, originalState != colInfo.State) case model.StateWriteOnly: // write only -> delete only job.SchemaState = model.StateDeleteOnly @@ -407,7 +407,7 @@ func doModifyColumn(t *meta.Meta, job *model.Job, newCol *model.ColumnInfo, oldN } } - ver, err = updateVersionAndTableInfo(t, job, tblInfo, true) + ver, err = updateVersionAndTableInfoWithCheck(t, job, tblInfo, true) if err != nil { job.State = model.JobStateCancelled return ver, errors.Trace(err) diff --git a/ddl/db_test.go b/ddl/db_test.go index 975f91205f4e4..dc177d69f1ce6 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -1696,6 +1696,14 @@ func (s *testDBSuite) TestDropColumn(c *C) { } } + // Test for drop partition table column. + s.tk.MustExec("drop table if exists t1") + s.tk.MustExec("set @@tidb_enable_table_partition = 1") + s.tk.MustExec("create table t1 (a bigint, b int, c int generated always as (b+1)) partition by range(a) (partition p0 values less than (10));") + _, err := s.tk.Exec("alter table t1 drop column a") + c.Assert(err, NotNil) + c.Assert(err.Error(), Equals, "[expression:1054]Unknown column 'a' in 'expression'") + s.tk.MustExec("drop database drop_col_db") } @@ -4684,3 +4692,35 @@ func (s *testDBSuite) TestAlterShardRowIDBits(c *C) { c.Assert(err, NotNil) c.Assert(err.Error(), Equals, "[autoid:1467]Failed to read auto-increment value from storage engine") } + +func (s *testDBSuite) TestDDLWithInvalidTableInfo(c *C) { + s.tk = testkit.NewTestKit(c, s.store) + tk := s.tk + + tk.MustExec("use test") + tk.MustExec("drop table if exists t") + defer tk.MustExec("drop table if exists t") + tk.MustExec("set @@tidb_enable_table_partition = 1") + // Test create with invalid expression. + _, err := s.tk.Exec(`CREATE TABLE t ( + c0 int(11) , + c1 int(11), + c2 decimal(16,4) GENERATED ALWAYS AS ((case when (c0 = 0) then 0 when (c0 > 0) then (c1 / c0) end)) + );`) + c.Assert(err, NotNil) + c.Assert(err.Error(), Equals, "line 1 column 26 near \" 0) THEN 0WHEN (`c0` > 0) THEN (`c1` / `c0`) END)\" (total length 75)") + + tk.MustExec("create table t (a bigint, b int, c int generated always as (b+1)) partition by range(a) (partition p0 values less than (10));") + // Test drop partition column. + _, err = tk.Exec("alter table t drop column a;") + c.Assert(err, NotNil) + c.Assert(err.Error(), Equals, "[expression:1054]Unknown column 'a' in 'expression'") + // Test modify column with invalid expression. + _, err = tk.Exec("alter table t modify column c int GENERATED ALWAYS AS ((case when (a = 0) then 0 when (a > 0) then (b / a) end));") + c.Assert(err, NotNil) + c.Assert(err.Error(), Equals, "[ddl:-1]line 1 column 25 near \" 0) THEN 0WHEN (`a` > 0) THEN (`b` / `a`) END)\" (total length 71)") + // Test add column with invalid expression. + _, err = tk.Exec("alter table t add column d int GENERATED ALWAYS AS ((case when (a = 0) then 0 when (a > 0) then (b / a) end));") + c.Assert(err, NotNil) + c.Assert(err.Error(), Equals, "[ddl:-1]line 1 column 25 near \" 0) THEN 0WHEN (`a` > 0) THEN (`b` / `a`) END)\" (total length 71)") +} diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 97729d492faeb..eab896e1f6d35 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -20,6 +20,7 @@ package ddl import ( "bytes" "fmt" + "github.com/pingcap/tidb/table/tables" "strings" "time" @@ -994,6 +995,12 @@ func (d *ddl) CreateTableWithLike(ctx sessionctx.Context, ident, referIdent ast. return errors.Trace(err) } +// checkTableInfoValid uses to check table info valid. This is used to validate table info. +func checkTableInfoValid(tblInfo *model.TableInfo) error { + _, err := tables.TableFromMeta(nil, tblInfo) + return err +} + func buildTableInfoWithLike(ident ast.Ident, referTblInfo *model.TableInfo) model.TableInfo { tblInfo := *referTblInfo // Check non-public column and adjust column offset. @@ -1122,6 +1129,13 @@ func (d *ddl) CreateTable(ctx sessionctx.Context, s *ast.CreateTableStmt) (err e tbInfo.Partition = pi } + tbInfo.State = model.StatePublic + err = checkTableInfoValid(tbInfo) + if err != nil { + return err + } + tbInfo.State = model.StateNone + job := &model.Job{ SchemaID: schema.ID, TableID: tbInfo.ID, diff --git a/ddl/index.go b/ddl/index.go index ef7000450b9bd..50e13e9393bc8 100644 --- a/ddl/index.go +++ b/ddl/index.go @@ -299,7 +299,7 @@ func (w *worker) onCreateIndex(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int // none -> delete only job.SchemaState = model.StateDeleteOnly indexInfo.State = model.StateDeleteOnly - ver, err = updateVersionAndTableInfo(t, job, tblInfo, originalState != indexInfo.State) + ver, err = updateVersionAndTableInfoWithCheck(t, job, tblInfo, originalState != indexInfo.State) case model.StateDeleteOnly: // delete only -> write only job.SchemaState = model.StateWriteOnly diff --git a/ddl/table.go b/ddl/table.go index bc394c5e007ed..66a6a0410f1d6 100644 --- a/ddl/table.go +++ b/ddl/table.go @@ -66,7 +66,7 @@ func onCreateTable(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64, _ error) // none -> public tbInfo.State = model.StatePublic tbInfo.UpdateTS = t.StartTS - err = t.CreateTable(schemaID, tbInfo) + err = createTableWithCheck(t, job, schemaID, tbInfo) if err != nil { return ver, errors.Trace(err) } @@ -83,6 +83,15 @@ func onCreateTable(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64, _ error) } } +func createTableWithCheck(t *meta.Meta, job *model.Job, schemaID int64, tbInfo *model.TableInfo) error { + err := checkTableInfoValid(tbInfo) + if err != nil { + job.State = model.JobStateCancelled + return errors.Trace(err) + } + return t.CreateTable(schemaID, tbInfo) +} + func onDropTable(t *meta.Meta, job *model.Job) (ver int64, _ error) { schemaID := job.SchemaID tableID := job.TableID @@ -521,6 +530,17 @@ func checkTableNotExists(t *meta.Meta, job *model.Job, schemaID int64, tableName return nil } +// updateVersionAndTableInfoWithCheck checks table info validate and updates the schema version and the table information +func updateVersionAndTableInfoWithCheck(t *meta.Meta, job *model.Job, tblInfo *model.TableInfo, shouldUpdateVer bool) ( + ver int64, err error) { + err = checkTableInfoValid(tblInfo) + if err != nil { + job.State = model.JobStateCancelled + return ver, errors.Trace(err) + } + return updateVersionAndTableInfo(t, job, tblInfo, shouldUpdateVer) +} + // updateVersionAndTableInfo updates the schema version and the table information. func updateVersionAndTableInfo(t *meta.Meta, job *model.Job, tblInfo *model.TableInfo, shouldUpdateVer bool) ( ver int64, err error) { From 81d61b1261b385abb34ef37f9b1b7b9188bfa9fb Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Mon, 20 May 2019 20:37:00 +0800 Subject: [PATCH 2/2] refine code --- 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 eab896e1f6d35..f3674e5ca28cd 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -20,7 +20,6 @@ package ddl import ( "bytes" "fmt" - "github.com/pingcap/tidb/table/tables" "strings" "time" @@ -36,6 +35,7 @@ import ( "github.com/pingcap/tidb/sessionctx" "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/table" + "github.com/pingcap/tidb/table/tables" "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/util/logutil" "github.com/pingcap/tidb/util/schemautil"