From d19b23778d8a646f199fdab9aaa13a9345c6eb76 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Thu, 13 Dec 2018 16:15:55 +0800 Subject: [PATCH 01/19] fix cancel drop column ddl error --- ddl/db_test.go | 74 ++++++++++++++++++++++++++++++++++++++++++ ddl/ddl_worker_test.go | 5 +++ ddl/rollingback.go | 42 ++++++++++++++++++++++++ util/admin/admin.go | 6 ++++ 4 files changed, 127 insertions(+) diff --git a/ddl/db_test.go b/ddl/db_test.go index c794446e88673..11004f5566426 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -811,6 +811,80 @@ LOOP: s.tk.MustExec("drop table test_drop_index") } +// TestCancelDropColumn tests cancel ddl job which type is drop column. +func (s *testDBSuite) TestCancelDropColumn(c *C) { + s.tk = testkit.NewTestKit(c, s.store) + s.mustExec(c, "use test_db") + s.mustExec(c, "create database if not exists test_drop_column") + s.mustExec(c, "drop table if exists t") + s.mustExec(c, "create table t(c1 int, c2 int)") + defer s.mustExec(c, "drop table t;") + testCases := []struct { + jobState model.JobState + JobSchemaState model.SchemaState + }{ + {model.JobStateNone, model.StateNone}, + {model.JobStateRunning, model.StateWriteOnly}, + {model.JobStateRunning, model.StateDeleteOnly}, + {model.JobStateRunning, model.StateDeleteReorganization}, + } + var checkErr error + oldReorgWaitTimeout := ddl.ReorgWaitTimeout + ddl.ReorgWaitTimeout = 50 * time.Millisecond + defer func() { ddl.ReorgWaitTimeout = oldReorgWaitTimeout }() + hook := &ddl.TestDDLCallback{} + var jobID int64 + testCase := &testCases[0] + hook.OnJobRunBeforeExported = func(job *model.Job) { + if job.Type == model.ActionDropColumn && job.State == testCase.jobState && job.SchemaState == testCase.JobSchemaState { + jobIDs := []int64{job.ID} + jobID = job.ID + hookCtx := mock.NewContext() + hookCtx.Store = s.store + err := hookCtx.NewTxn(context.Background()) + if err != nil { + checkErr = errors.Trace(err) + return + } + errs, err1 := admin.CancelJobs(hookCtx.Txn(true), jobIDs) + if err1 != nil { + checkErr = errors.Trace(err1) + return + } + if errs[0] != nil { + checkErr = errors.Trace(errs[0]) + return + } + checkErr = hookCtx.Txn(true).Commit(context.Background()) + } + } + originalHook := s.dom.DDL().GetHook() + s.dom.DDL().(ddl.DDLForTest).SetHook(hook) + for i := range testCases { + testCase = &testCases[i] + s.mustExec(c, "alter table t add column c3 int") + rs, err := s.tk.Exec("alter table t drop column c3") + if rs != nil { + rs.Close() + } + var col1 *table.Column + t := s.testGetTable(c, "t") + for _, col := range t.Cols() { + if strings.EqualFold(col.Name.L, "c3") { + col1 = col + break + } + } + c.Assert(col1, IsNil) + c.Assert(err, IsNil) + c.Assert(checkErr, NotNil) + c.Assert(checkErr.Error(), Equals, admin.ErrCannotCancelDDLJob.GenWithStackByArgs(jobID).Error()) + } + s.dom.DDL().(ddl.DDLForTest).SetHook(originalHook) + s.mustExec(c, "alter table t add column c3 int") + s.mustExec(c, "alter table t drop column c3") +} + func checkDelRangeDone(c *C, ctx sessionctx.Context, idx table.Index) { startTime := time.Now() f := func() map[int64]struct{} { diff --git a/ddl/ddl_worker_test.go b/ddl/ddl_worker_test.go index 0fccd967d83b3..84baac87975ef 100644 --- a/ddl/ddl_worker_test.go +++ b/ddl/ddl_worker_test.go @@ -347,6 +347,11 @@ func buildCancelJobTests(firstID int64) []testCancelJob { {act: model.ActionAddColumn, jobIDs: []int64{firstID + 6}, cancelRetErrs: noErrs, cancelState: model.StateWriteOnly, ddlRetErr: err}, {act: model.ActionAddColumn, jobIDs: []int64{firstID + 7}, cancelRetErrs: noErrs, cancelState: model.StateWriteReorganization, ddlRetErr: err}, {act: model.ActionAddColumn, jobIDs: []int64{firstID + 8}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 8)}, cancelState: model.StatePublic, ddlRetErr: err}, + + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 9}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 9)}, cancelState: model.StateDeleteOnly, ddlRetErr: err}, + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 10}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 10)}, cancelState: model.StateWriteOnly, ddlRetErr: err}, + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 11}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 11)}, cancelState: model.StateWriteReorganization, ddlRetErr: err}, + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 12}, cancelRetErrs: noErrs, cancelState: model.StatePublic, ddlRetErr: err}, } return tests diff --git a/ddl/rollingback.go b/ddl/rollingback.go index db8da52003162..9b4ed2bb35ae6 100644 --- a/ddl/rollingback.go +++ b/ddl/rollingback.go @@ -118,6 +118,46 @@ func rollingbackAddColumn(t *meta.Meta, job *model.Job) (ver int64, err error) { return ver, errCancelledDDLJob } +func rollingbackDropColumn(t *meta.Meta, job *model.Job) (ver int64, err error) { + schemaID := job.SchemaID + tblInfo, err := getTableInfo(t, job, schemaID) + if err != nil { + return ver, errors.Trace(err) + } + var colName model.CIStr + err = job.DecodeArgs(&colName) + if err != nil { + job.State = model.JobStateCancelled + return ver, errors.Trace(err) + } + colInfo := model.FindColumnInfo(tblInfo.Columns, colName.L) + if colInfo == nil { + job.State = model.JobStateCancelled + ErrCantDropFieldOrKey.GenWithStack("column %s doesn't exist", colName) + } + switch colInfo.State { + // In the state of drop column `write only -> delete only -> reorganization`, + // We can not rollback now, so just continue to drop column. + case model.StateWriteOnly, model.StateDeleteOnly, model.StateDeleteReorganization, model.StateNone: + job.State = model.JobStateRunning + return ver, nil + case model.StatePublic: + job.State = model.JobStateRollbackDone + colInfo.State = model.StatePublic + default: + err = ErrInvalidTableState.GenWithStack("invalid table state %v", tblInfo.State) + } + originalState := colInfo.State + job.State = model.JobStateRollbackDone + job.Args = []interface{}{colInfo.Name} + ver, err = updateVersionAndTableInfo(t, job, tblInfo, originalState != colInfo.State) + if err != nil { + return ver, errors.Trace(err) + } + job.FinishTableJob(model.JobStateRollbackDone, model.StatePublic, ver, tblInfo) + return ver, errCancelledDDLJob +} + func rollingbackDropIndex(t *meta.Meta, job *model.Job) (ver int64, err error) { schemaID := job.SchemaID tblInfo, err := getTableInfo(t, job, schemaID) @@ -180,6 +220,8 @@ func convertJob2RollbackJob(w *worker, d *ddlCtx, t *meta.Meta, job *model.Job) switch job.Type { case model.ActionAddColumn: ver, err = rollingbackAddColumn(t, job) + case model.ActionDropColumn: + ver, err = rollingbackDropColumn(t, job) case model.ActionAddIndex: ver, err = rollingbackAddindex(w, d, t, job) case model.ActionDropIndex: diff --git a/util/admin/admin.go b/util/admin/admin.go index a09cfce31982c..f2092b8e5a4a9 100644 --- a/util/admin/admin.go +++ b/util/admin/admin.go @@ -91,6 +91,12 @@ func isJobRollbackable(job *model.Job, id int64) error { job.SchemaState == model.StateDeleteReorganization { return ErrCannotCancelDDLJob.GenWithStackByArgs(id) } + case model.ActionDropColumn: + if job.SchemaState == model.StateDeleteOnly || job.SchemaState == model.StateNone || + job.SchemaState == model.StateWriteOnly || + job.SchemaState == model.StateDeleteReorganization { + return ErrCannotCancelDDLJob.GenWithStackByArgs(id) + } } return nil } From d310b3fdba4c0bf8649999af7b0a4399fff46f23 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Tue, 18 Dec 2018 10:58:33 +0800 Subject: [PATCH 02/19] Fix CI --- util/admin/admin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/admin/admin.go b/util/admin/admin.go index f2092b8e5a4a9..6fac99872d95f 100644 --- a/util/admin/admin.go +++ b/util/admin/admin.go @@ -92,7 +92,7 @@ func isJobRollbackable(job *model.Job, id int64) error { return ErrCannotCancelDDLJob.GenWithStackByArgs(id) } case model.ActionDropColumn: - if job.SchemaState == model.StateDeleteOnly || job.SchemaState == model.StateNone || + if job.SchemaState == model.StateDeleteOnly || job.SchemaState == model.StateNone || job.SchemaState == model.StateWriteOnly || job.SchemaState == model.StateDeleteReorganization { return ErrCannotCancelDDLJob.GenWithStackByArgs(id) From e3bcb51d0bd901f79f803cbd53cd343cc127f8b6 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Tue, 18 Dec 2018 15:27:43 +0800 Subject: [PATCH 03/19] Address comment --- ddl/rollingback.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ddl/rollingback.go b/ddl/rollingback.go index 9b4ed2bb35ae6..9025d78a965d3 100644 --- a/ddl/rollingback.go +++ b/ddl/rollingback.go @@ -135,6 +135,7 @@ func rollingbackDropColumn(t *meta.Meta, job *model.Job) (ver int64, err error) job.State = model.JobStateCancelled ErrCantDropFieldOrKey.GenWithStack("column %s doesn't exist", colName) } + originalState := colInfo.State switch colInfo.State { // In the state of drop column `write only -> delete only -> reorganization`, // We can not rollback now, so just continue to drop column. @@ -147,8 +148,6 @@ func rollingbackDropColumn(t *meta.Meta, job *model.Job) (ver int64, err error) default: err = ErrInvalidTableState.GenWithStack("invalid table state %v", tblInfo.State) } - originalState := colInfo.State - job.State = model.JobStateRollbackDone job.Args = []interface{}{colInfo.Name} ver, err = updateVersionAndTableInfo(t, job, tblInfo, originalState != colInfo.State) if err != nil { From 7e42468870ce1ad0f0a430cc4194a15ebbc02faa Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Wed, 19 Dec 2018 16:53:08 +0800 Subject: [PATCH 04/19] Add TestCancelJob Test --- ddl/ddl_worker_test.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/ddl/ddl_worker_test.go b/ddl/ddl_worker_test.go index 84baac87975ef..be623a1c98a88 100644 --- a/ddl/ddl_worker_test.go +++ b/ddl/ddl_worker_test.go @@ -381,6 +381,18 @@ func (s *testDDLSuite) checkAddColumn(c *C, d *ddl, schemaID int64, tableID int6 c.Assert(found, Equals, success) } +func (s *testDDLSuite) checkDropColumn(c *C, d *ddl, schemaID int64, tableID int64, colName string, success bool) { + changedTable := testGetTable(c, d, schemaID, tableID) + found := true + for _, colInfo := range changedTable.Meta().Columns { + if colInfo.Name.O == colName { + found = false + break + } + } + c.Assert(found, Equals, success) +} + func (s *testDDLSuite) TestCancelJob(c *C) { store := testCreateStore(c, "test_cancel_job") defer store.Close() @@ -494,6 +506,24 @@ func (s *testDDLSuite) TestCancelJob(c *C) { testAddColumn(c, ctx, d, dbInfo, tblInfo, addColumnArgs) c.Check(errors.ErrorStack(checkErr), Equals, "") s.checkAddColumn(c, d, dbInfo.ID, tblInfo.ID, addingColName, true) + + // for drop column. + test = &tests[9] + dropColName := "c2" + dropColumnArgs := []interface{}{model.NewCIStr(dropColName)} + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) + c.Check(errors.ErrorStack(checkErr), Equals, "") + s.checkDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) + + test = &tests[10] + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) + c.Check(errors.ErrorStack(checkErr), Equals, "") + s.checkDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) + + test = &tests[11] + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) + c.Check(errors.ErrorStack(checkErr), Equals, "") + s.checkDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) } func (s *testDDLSuite) TestIgnorableSpec(c *C) { From 8c25da3b4bbff184f9aa10f3f3d0867bc8effb89 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Wed, 26 Dec 2018 17:29:08 +0800 Subject: [PATCH 05/19] resolve conflicts --- util/admin/admin.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/util/admin/admin.go b/util/admin/admin.go index f29747063fd2d..9b082a8b0c4fb 100644 --- a/util/admin/admin.go +++ b/util/admin/admin.go @@ -95,6 +95,8 @@ func isJobRollbackable(job *model.Job, id int64) error { if job.SchemaState == model.StateDeleteOnly || job.SchemaState == model.StateNone || job.SchemaState == model.StateWriteOnly || job.SchemaState == model.StateDeleteReorganization { + return ErrCannotCancelDDLJob.GenWithStackByArgs(id) + } case model.ActionDropSchema, model.ActionDropTable: // To simplify the rollback logic, cannot be canceled in the following states. if job.SchemaState == model.StateWriteOnly || From 53b005bb3b3c45b5241006e2ae5b66c3d02b86ae Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Wed, 26 Dec 2018 18:13:41 +0800 Subject: [PATCH 06/19] Address comment --- ddl/db_test.go | 37 ++++++++++++++++++++++++------------- ddl/rollingback.go | 7 ++----- util/admin/admin.go | 2 +- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/ddl/db_test.go b/ddl/db_test.go index e84d417c03f85..8a3ba80ba737b 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -895,11 +895,12 @@ func (s *testDBSuite) TestCancelDropColumn(c *C) { testCases := []struct { jobState model.JobState JobSchemaState model.SchemaState + cancelSucc bool }{ - {model.JobStateNone, model.StateNone}, - {model.JobStateRunning, model.StateWriteOnly}, - {model.JobStateRunning, model.StateDeleteOnly}, - {model.JobStateRunning, model.StateDeleteReorganization}, + {model.JobStateNone, model.StateNone, true}, + {model.JobStateRunning, model.StateWriteOnly, false}, + {model.JobStateRunning, model.StateDeleteOnly, false}, + {model.JobStateRunning, model.StateDeleteReorganization, false}, } var checkErr error oldReorgWaitTimeout := ddl.ReorgWaitTimeout @@ -911,7 +912,6 @@ func (s *testDBSuite) TestCancelDropColumn(c *C) { hook.OnJobRunBeforeExported = func(job *model.Job) { if job.Type == model.ActionDropColumn && job.State == testCase.jobState && job.SchemaState == testCase.JobSchemaState { jobIDs := []int64{job.ID} - jobID = job.ID hookCtx := mock.NewContext() hookCtx.Store = s.store err := hookCtx.NewTxn(context.Background()) @@ -919,16 +919,21 @@ func (s *testDBSuite) TestCancelDropColumn(c *C) { checkErr = errors.Trace(err) return } - errs, err1 := admin.CancelJobs(hookCtx.Txn(true), jobIDs) - if err1 != nil { - checkErr = errors.Trace(err1) + txn, err := hookCtx.Txn(true) + if err != nil { + checkErr = errors.Trace(err) + return + } + errs, err := admin.CancelJobs(txn, jobIDs) + if err != nil { + checkErr = errors.Trace(err) return } if errs[0] != nil { checkErr = errors.Trace(errs[0]) return } - checkErr = hookCtx.Txn(true).Commit(context.Background()) + checkErr = txn.Commit(context.Background()) } } originalHook := s.dom.DDL().GetHook() @@ -948,10 +953,16 @@ func (s *testDBSuite) TestCancelDropColumn(c *C) { break } } - c.Assert(col1, IsNil) - c.Assert(err, IsNil) - c.Assert(checkErr, NotNil) - c.Assert(checkErr.Error(), Equals, admin.ErrCannotCancelDDLJob.GenWithStackByArgs(jobID).Error()) + if testCase.cancelSucc { + c.Assert(checkErr, IsNil) + c.Assert(err, NotNil) + c.Assert(err.Error(), Equals, "[ddl:12]cancelled DDL job") + } else { + c.Assert(col1, IsNil) + c.Assert(err, IsNil) + c.Assert(checkErr, NotNil) + c.Assert(checkErr.Error(), Equals, admin.ErrCannotCancelDDLJob.GenWithStackByArgs(jobID).Error()) + } } s.dom.DDL().(ddl.DDLForTest).SetHook(originalHook) s.mustExec(c, "alter table t add column c3 int") diff --git a/ddl/rollingback.go b/ddl/rollingback.go index c9bb22e39ff8f..7fd5715633579 100644 --- a/ddl/rollingback.go +++ b/ddl/rollingback.go @@ -137,14 +137,11 @@ func rollingbackDropColumn(t *meta.Meta, job *model.Job) (ver int64, err error) } originalState := colInfo.State switch colInfo.State { - // In the state of drop column `write only -> delete only -> reorganization`, + // In the state of drop column `public -> write only -> delete only -> reorganization`, // We can not rollback now, so just continue to drop column. - case model.StateWriteOnly, model.StateDeleteOnly, model.StateDeleteReorganization, model.StateNone: + case model.StatePublic, model.StateWriteOnly, model.StateDeleteOnly, model.StateDeleteReorganization: job.State = model.JobStateRunning return ver, nil - case model.StatePublic: - job.State = model.JobStateRollbackDone - colInfo.State = model.StatePublic default: err = ErrInvalidTableState.GenWithStack("invalid table state %v", tblInfo.State) } diff --git a/util/admin/admin.go b/util/admin/admin.go index 9b082a8b0c4fb..fa0903dc9cbcb 100644 --- a/util/admin/admin.go +++ b/util/admin/admin.go @@ -92,7 +92,7 @@ func isJobRollbackable(job *model.Job, id int64) error { return ErrCannotCancelDDLJob.GenWithStackByArgs(id) } case model.ActionDropColumn: - if job.SchemaState == model.StateDeleteOnly || job.SchemaState == model.StateNone || + if job.SchemaState == model.StatePublic || job.SchemaState == model.StateDeleteOnly || job.SchemaState == model.StateWriteOnly || job.SchemaState == model.StateDeleteReorganization { return ErrCannotCancelDDLJob.GenWithStackByArgs(id) From f10d6da76a42c02c56626be97e7961590a586a60 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Wed, 26 Dec 2018 20:24:29 +0800 Subject: [PATCH 07/19] Address comment --- ddl/column.go | 9 +++++++++ ddl/ddl_worker_test.go | 8 ++++---- ddl/rollingback.go | 38 +------------------------------------- util/admin/admin.go | 4 +--- 4 files changed, 15 insertions(+), 44 deletions(-) diff --git a/ddl/column.go b/ddl/column.go index 6329b616158a0..7297cd109ac33 100644 --- a/ddl/column.go +++ b/ddl/column.go @@ -229,6 +229,15 @@ func onDropColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { job.State = model.JobStateCancelled return ver, ErrCantDropFieldOrKey.GenWithStack("column %s doesn't exist", colName) } + + if job.IsRollingback() && job.Type == model.ActionDropColumn { + if colInfo.State == model.StatePublic { + job.State = model.JobStateCancelled + return ver, errCancelledDDLJob + } + job.State = model.JobStateRunning + } + if err = isDroppableColumn(tblInfo, colName); err != nil { job.State = model.JobStateCancelled return ver, errors.Trace(err) diff --git a/ddl/ddl_worker_test.go b/ddl/ddl_worker_test.go index 1737676425b68..009c3c70176da 100644 --- a/ddl/ddl_worker_test.go +++ b/ddl/ddl_worker_test.go @@ -374,7 +374,7 @@ func buildCancelJobTests(firstID int64) []testCancelJob { {act: model.ActionDropColumn, jobIDs: []int64{firstID + 9}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 9)}, cancelState: model.StateDeleteOnly, ddlRetErr: err}, {act: model.ActionDropColumn, jobIDs: []int64{firstID + 10}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 10)}, cancelState: model.StateWriteOnly, ddlRetErr: err}, {act: model.ActionDropColumn, jobIDs: []int64{firstID + 11}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 11)}, cancelState: model.StateWriteReorganization, ddlRetErr: err}, - {act: model.ActionDropColumn, jobIDs: []int64{firstID + 12}, cancelRetErrs: noErrs, cancelState: model.StatePublic, ddlRetErr: err}, + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 12}, cancelRetErrs: noErrs, cancelState: model.StateNone, ddlRetErr: err}, // Test create table, watch out, table id will alloc a globalID. {act: model.ActionCreateTable, jobIDs: []int64{firstID + 10}, cancelRetErrs: noErrs, cancelState: model.StateNone, ddlRetErr: err}, @@ -411,14 +411,14 @@ func (s *testDDLSuite) checkAddColumn(c *C, d *ddl, schemaID int64, tableID int6 func (s *testDDLSuite) checkDropColumn(c *C, d *ddl, schemaID int64, tableID int64, colName string, success bool) { changedTable := testGetTable(c, d, schemaID, tableID) - found := true + notFound := true for _, colInfo := range changedTable.Meta().Columns { if colInfo.Name.O == colName { - found = false + notFound = false break } } - c.Assert(found, Equals, success) + c.Assert(notFound, Equals, success) } func (s *testDDLSuite) TestCancelJob(c *C) { diff --git a/ddl/rollingback.go b/ddl/rollingback.go index 7fd5715633579..230fcb772c26a 100644 --- a/ddl/rollingback.go +++ b/ddl/rollingback.go @@ -118,42 +118,6 @@ func rollingbackAddColumn(t *meta.Meta, job *model.Job) (ver int64, err error) { return ver, errCancelledDDLJob } -func rollingbackDropColumn(t *meta.Meta, job *model.Job) (ver int64, err error) { - schemaID := job.SchemaID - tblInfo, err := getTableInfo(t, job, schemaID) - if err != nil { - return ver, errors.Trace(err) - } - var colName model.CIStr - err = job.DecodeArgs(&colName) - if err != nil { - job.State = model.JobStateCancelled - return ver, errors.Trace(err) - } - colInfo := model.FindColumnInfo(tblInfo.Columns, colName.L) - if colInfo == nil { - job.State = model.JobStateCancelled - ErrCantDropFieldOrKey.GenWithStack("column %s doesn't exist", colName) - } - originalState := colInfo.State - switch colInfo.State { - // In the state of drop column `public -> write only -> delete only -> reorganization`, - // We can not rollback now, so just continue to drop column. - case model.StatePublic, model.StateWriteOnly, model.StateDeleteOnly, model.StateDeleteReorganization: - job.State = model.JobStateRunning - return ver, nil - default: - err = ErrInvalidTableState.GenWithStack("invalid table state %v", tblInfo.State) - } - job.Args = []interface{}{colInfo.Name} - ver, err = updateVersionAndTableInfo(t, job, tblInfo, originalState != colInfo.State) - if err != nil { - return ver, errors.Trace(err) - } - job.FinishTableJob(model.JobStateRollbackDone, model.StatePublic, ver, tblInfo) - return ver, errCancelledDDLJob -} - func rollingbackDropIndex(t *meta.Meta, job *model.Job) (ver int64, err error) { schemaID := job.SchemaID tblInfo, err := getTableInfo(t, job, schemaID) @@ -217,7 +181,7 @@ func convertJob2RollbackJob(w *worker, d *ddlCtx, t *meta.Meta, job *model.Job) case model.ActionAddColumn: ver, err = rollingbackAddColumn(t, job) case model.ActionDropColumn: - ver, err = rollingbackDropColumn(t, job) + job.State = model.JobStateRollingback case model.ActionAddIndex: ver, err = rollingbackAddindex(w, d, t, job) case model.ActionDropIndex: diff --git a/util/admin/admin.go b/util/admin/admin.go index fa0903dc9cbcb..52e3eae298280 100644 --- a/util/admin/admin.go +++ b/util/admin/admin.go @@ -92,9 +92,7 @@ func isJobRollbackable(job *model.Job, id int64) error { return ErrCannotCancelDDLJob.GenWithStackByArgs(id) } case model.ActionDropColumn: - if job.SchemaState == model.StatePublic || job.SchemaState == model.StateDeleteOnly || - job.SchemaState == model.StateWriteOnly || - job.SchemaState == model.StateDeleteReorganization { + if job.SchemaState != model.StateNone { return ErrCannotCancelDDLJob.GenWithStackByArgs(id) } case model.ActionDropSchema, model.ActionDropTable: From 3301e27ac120eaf44d593014d09796c3fbca5385 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Wed, 26 Dec 2018 20:45:31 +0800 Subject: [PATCH 08/19] Address comment --- ddl/column.go | 14 ++++++++------ ddl/ddl_worker_test.go | 2 -- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/ddl/column.go b/ddl/column.go index 7297cd109ac33..341bdae6b5d4c 100644 --- a/ddl/column.go +++ b/ddl/column.go @@ -230,19 +230,21 @@ func onDropColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { return ver, ErrCantDropFieldOrKey.GenWithStack("column %s doesn't exist", colName) } + if err = isDroppableColumn(tblInfo, colName); err != nil { + job.State = model.JobStateCancelled + return ver, errors.Trace(err) + } + if job.IsRollingback() && job.Type == model.ActionDropColumn { - if colInfo.State == model.StatePublic { + if colInfo.State == model.StatePublic && job.SchemaState == model.StateNone { job.State = model.JobStateCancelled return ver, errCancelledDDLJob } + // In the state of drop column `write only -> delete only -> reorganization`, + // We can not rollback now, so just continue to drop column. job.State = model.JobStateRunning } - if err = isDroppableColumn(tblInfo, colName); err != nil { - job.State = model.JobStateCancelled - return ver, errors.Trace(err) - } - originalState := colInfo.State switch colInfo.State { case model.StatePublic: diff --git a/ddl/ddl_worker_test.go b/ddl/ddl_worker_test.go index 009c3c70176da..04a8e16ff6aa5 100644 --- a/ddl/ddl_worker_test.go +++ b/ddl/ddl_worker_test.go @@ -577,8 +577,6 @@ func (s *testDDLSuite) TestCancelJob(c *C) { doDDLJobErrWithSchemaState(ctx, d, c, dbInfo1.ID, 0, model.ActionCreateSchema, []interface{}{dbInfo1}, &cancelState) c.Check(checkErr, IsNil) testCheckSchemaState(c, d, dbInfo1, model.StateNone) - - } func (s *testDDLSuite) TestIgnorableSpec(c *C) { From 0577091a524881ddbd20582e5e4cb5d8b89c9e27 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Wed, 26 Dec 2018 21:32:57 +0800 Subject: [PATCH 09/19] change rollingbackDropColumn fuction --- ddl/column.go | 22 +++++++++++++++------- ddl/db_test.go | 1 + 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/ddl/column.go b/ddl/column.go index 341bdae6b5d4c..cf526a3296d20 100644 --- a/ddl/column.go +++ b/ddl/column.go @@ -229,20 +229,16 @@ func onDropColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { job.State = model.JobStateCancelled return ver, ErrCantDropFieldOrKey.GenWithStack("column %s doesn't exist", colName) } - if err = isDroppableColumn(tblInfo, colName); err != nil { job.State = model.JobStateCancelled return ver, errors.Trace(err) } - if job.IsRollingback() && job.Type == model.ActionDropColumn { - if colInfo.State == model.StatePublic && job.SchemaState == model.StateNone { + if job.IsRollingback() { + if err = rollingbackDropColumn(job, colInfo); err != nil { job.State = model.JobStateCancelled - return ver, errCancelledDDLJob + return ver, errors.Trace(err) } - // In the state of drop column `write only -> delete only -> reorganization`, - // We can not rollback now, so just continue to drop column. - job.State = model.JobStateRunning } originalState := colInfo.State @@ -286,6 +282,18 @@ func onDropColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { return ver, errors.Trace(err) } +func rollingbackDropColumn(job *model.Job, colInfo *model.ColumnInfo) error{ + if job.Type == model.ActionDropColumn { + if colInfo.State == model.StatePublic { + return errCancelledDDLJob + } + // In the state of drop column `write only -> delete only -> reorganization`, + // We can not rollback now, so just continue to drop column. + job.State = model.JobStateRunning + } + return nil +} + func onSetDefaultValue(t *meta.Meta, job *model.Job) (ver int64, _ error) { newCol := &model.ColumnInfo{} err := job.DecodeArgs(newCol) diff --git a/ddl/db_test.go b/ddl/db_test.go index 8a3ba80ba737b..99a89fc919287 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -912,6 +912,7 @@ func (s *testDBSuite) TestCancelDropColumn(c *C) { hook.OnJobRunBeforeExported = func(job *model.Job) { if job.Type == model.ActionDropColumn && job.State == testCase.jobState && job.SchemaState == testCase.JobSchemaState { jobIDs := []int64{job.ID} + jobID = job.ID hookCtx := mock.NewContext() hookCtx.Store = s.store err := hookCtx.NewTxn(context.Background()) From a3e9ef367ced499fdd0414d02856b373c6b22029 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Wed, 26 Dec 2018 21:45:47 +0800 Subject: [PATCH 10/19] Address comment --- ddl/ddl_worker_test.go | 44 ++++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/ddl/ddl_worker_test.go b/ddl/ddl_worker_test.go index 04a8e16ff6aa5..bda78a9e69f99 100644 --- a/ddl/ddl_worker_test.go +++ b/ddl/ddl_worker_test.go @@ -370,16 +370,15 @@ func buildCancelJobTests(firstID int64) []testCancelJob { {act: model.ActionAddColumn, jobIDs: []int64{firstID + 7}, cancelRetErrs: noErrs, cancelState: model.StateWriteReorganization, ddlRetErr: err}, {act: model.ActionAddColumn, jobIDs: []int64{firstID + 8}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 8)}, cancelState: model.StatePublic, ddlRetErr: err}, - - {act: model.ActionDropColumn, jobIDs: []int64{firstID + 9}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 9)}, cancelState: model.StateDeleteOnly, ddlRetErr: err}, - {act: model.ActionDropColumn, jobIDs: []int64{firstID + 10}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 10)}, cancelState: model.StateWriteOnly, ddlRetErr: err}, - {act: model.ActionDropColumn, jobIDs: []int64{firstID + 11}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 11)}, cancelState: model.StateWriteReorganization, ddlRetErr: err}, - {act: model.ActionDropColumn, jobIDs: []int64{firstID + 12}, cancelRetErrs: noErrs, cancelState: model.StateNone, ddlRetErr: err}, - // Test create table, watch out, table id will alloc a globalID. {act: model.ActionCreateTable, jobIDs: []int64{firstID + 10}, cancelRetErrs: noErrs, cancelState: model.StateNone, ddlRetErr: err}, // Test create database, watch out, database id will alloc a globalID. {act: model.ActionCreateSchema, jobIDs: []int64{firstID + 12}, cancelRetErrs: noErrs, cancelState: model.StateNone, ddlRetErr: err}, + + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 13}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 13)}, cancelState: model.StateDeleteOnly, ddlRetErr: err}, + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 14}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 14)}, cancelState: model.StateWriteOnly, ddlRetErr: err}, + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 15}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 15)}, cancelState: model.StateWriteReorganization, ddlRetErr: err}, + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 16}, cancelRetErrs: noErrs, cancelState: model.StateNone, ddlRetErr: err}, } return tests @@ -545,38 +544,37 @@ func (s *testDDLSuite) TestCancelJob(c *C) { c.Check(errors.ErrorStack(checkErr), Equals, "") s.checkAddColumn(c, d, dbInfo.ID, tblInfo.ID, addingColName, true) + // for create table + tblInfo1 := testTableInfo(c, d, "t1", 2) + test = &tests[8] + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo1.ID, model.ActionCreateTable, []interface{}{tblInfo1}, &cancelState) + c.Check(checkErr, IsNil) + testCheckTableState(c, d, dbInfo, tblInfo1, model.StateNone) - // for drop column. + // for create database + dbInfo1 := testSchemaInfo(c, d, "test_cancel_job1") test = &tests[9] + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo1.ID, 0, model.ActionCreateSchema, []interface{}{dbInfo1}, &cancelState) + c.Check(checkErr, IsNil) + testCheckSchemaState(c, d, dbInfo1, model.StateNone) + + // for drop column. + test = &tests[13] dropColName := "c2" dropColumnArgs := []interface{}{model.NewCIStr(dropColName)} doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) c.Check(errors.ErrorStack(checkErr), Equals, "") s.checkDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) - test = &tests[10] + test = &tests[14] doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) c.Check(errors.ErrorStack(checkErr), Equals, "") s.checkDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) - test = &tests[11] + test = &tests[15] doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) c.Check(errors.ErrorStack(checkErr), Equals, "") s.checkDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) - - // for create table - tblInfo1 := testTableInfo(c, d, "t1", 2) - test = &tests[8] - doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo1.ID, model.ActionCreateTable, []interface{}{tblInfo1}, &cancelState) - c.Check(checkErr, IsNil) - testCheckTableState(c, d, dbInfo, tblInfo1, model.StateNone) - - // for create database - dbInfo1 := testSchemaInfo(c, d, "test_cancel_job1") - test = &tests[9] - doDDLJobErrWithSchemaState(ctx, d, c, dbInfo1.ID, 0, model.ActionCreateSchema, []interface{}{dbInfo1}, &cancelState) - c.Check(checkErr, IsNil) - testCheckSchemaState(c, d, dbInfo1, model.StateNone) } func (s *testDDLSuite) TestIgnorableSpec(c *C) { From b2a749bc73fd7806dcb06c1244304983afe0eadc Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Wed, 26 Dec 2018 22:39:55 +0800 Subject: [PATCH 11/19] change TestCancelDropColumn test --- ddl/db_test.go | 18 +++++++++--------- util/admin/admin.go | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/ddl/db_test.go b/ddl/db_test.go index 99a89fc919287..98753df426c4c 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -888,10 +888,9 @@ LOOP: func (s *testDBSuite) TestCancelDropColumn(c *C) { s.tk = testkit.NewTestKit(c, s.store) s.mustExec(c, "use test_db") - s.mustExec(c, "create database if not exists test_drop_column") - s.mustExec(c, "drop table if exists t") - s.mustExec(c, "create table t(c1 int, c2 int)") - defer s.mustExec(c, "drop table t;") + s.mustExec(c, "drop table if exists test_drop_column") + s.mustExec(c, "create table test_drop_column(c1 int, c2 int)") + defer s.mustExec(c, "drop table test_drop_column;") testCases := []struct { jobState model.JobState JobSchemaState model.SchemaState @@ -941,13 +940,13 @@ func (s *testDBSuite) TestCancelDropColumn(c *C) { s.dom.DDL().(ddl.DDLForTest).SetHook(hook) for i := range testCases { testCase = &testCases[i] - s.mustExec(c, "alter table t add column c3 int") - rs, err := s.tk.Exec("alter table t drop column c3") + s.mustExec(c, "alter table test_drop_column add column c3 int") + rs, err := s.tk.Exec("alter table test_drop_column drop column c3") if rs != nil { rs.Close() } var col1 *table.Column - t := s.testGetTable(c, "t") + t := s.testGetTable(c, "test_drop_column") for _, col := range t.Cols() { if strings.EqualFold(col.Name.L, "c3") { col1 = col @@ -957,6 +956,7 @@ func (s *testDBSuite) TestCancelDropColumn(c *C) { if testCase.cancelSucc { c.Assert(checkErr, IsNil) c.Assert(err, NotNil) + c.Assert(col1, NotNil) c.Assert(err.Error(), Equals, "[ddl:12]cancelled DDL job") } else { c.Assert(col1, IsNil) @@ -966,8 +966,8 @@ func (s *testDBSuite) TestCancelDropColumn(c *C) { } } s.dom.DDL().(ddl.DDLForTest).SetHook(originalHook) - s.mustExec(c, "alter table t add column c3 int") - s.mustExec(c, "alter table t drop column c3") + s.mustExec(c, "alter table test_drop_column add column c3 int") + s.mustExec(c, "alter table test_drop_column drop column c3") } func checkDelRangeDone(c *C, ctx sessionctx.Context, idx table.Index) { diff --git a/util/admin/admin.go b/util/admin/admin.go index 52e3eae298280..203c31307ab67 100644 --- a/util/admin/admin.go +++ b/util/admin/admin.go @@ -92,7 +92,7 @@ func isJobRollbackable(job *model.Job, id int64) error { return ErrCannotCancelDDLJob.GenWithStackByArgs(id) } case model.ActionDropColumn: - if job.SchemaState != model.StateNone { + if job.State != model.JobStateNone { return ErrCannotCancelDDLJob.GenWithStackByArgs(id) } case model.ActionDropSchema, model.ActionDropTable: From 703c5d390c994bb200a7ccb6584d3edfb0d57e83 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Thu, 27 Dec 2018 04:21:59 +0800 Subject: [PATCH 12/19] Address comment --- ddl/column.go | 4 ++-- ddl/db_test.go | 2 +- ddl/ddl_worker_test.go | 14 ++------------ 3 files changed, 5 insertions(+), 15 deletions(-) diff --git a/ddl/column.go b/ddl/column.go index cf526a3296d20..9d82e606de4cf 100644 --- a/ddl/column.go +++ b/ddl/column.go @@ -282,7 +282,7 @@ func onDropColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { return ver, errors.Trace(err) } -func rollingbackDropColumn(job *model.Job, colInfo *model.ColumnInfo) error{ +func rollingbackDropColumn(job *model.Job, colInfo *model.ColumnInfo) error { if job.Type == model.ActionDropColumn { if colInfo.State == model.StatePublic { return errCancelledDDLJob @@ -291,7 +291,7 @@ func rollingbackDropColumn(job *model.Job, colInfo *model.ColumnInfo) error{ // We can not rollback now, so just continue to drop column. job.State = model.JobStateRunning } - return nil + return errCancelledDDLJob } func onSetDefaultValue(t *meta.Meta, job *model.Job) (ver int64, _ error) { diff --git a/ddl/db_test.go b/ddl/db_test.go index 98753df426c4c..95d795380e993 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -894,7 +894,7 @@ func (s *testDBSuite) TestCancelDropColumn(c *C) { testCases := []struct { jobState model.JobState JobSchemaState model.SchemaState - cancelSucc bool + cancelSucc bool }{ {model.JobStateNone, model.StateNone, true}, {model.JobStateRunning, model.StateWriteOnly, false}, diff --git a/ddl/ddl_worker_test.go b/ddl/ddl_worker_test.go index bda78a9e69f99..c92a5934c74c9 100644 --- a/ddl/ddl_worker_test.go +++ b/ddl/ddl_worker_test.go @@ -408,7 +408,7 @@ func (s *testDDLSuite) checkAddColumn(c *C, d *ddl, schemaID int64, tableID int6 c.Assert(found, Equals, success) } -func (s *testDDLSuite) checkDropColumn(c *C, d *ddl, schemaID int64, tableID int64, colName string, success bool) { +func (s *testDDLSuite) checkCancelDropColumn(c *C, d *ddl, schemaID int64, tableID int64, colName string, success bool) { changedTable := testGetTable(c, d, schemaID, tableID) notFound := true for _, colInfo := range changedTable.Meta().Columns { @@ -564,17 +564,7 @@ func (s *testDDLSuite) TestCancelJob(c *C) { dropColumnArgs := []interface{}{model.NewCIStr(dropColName)} doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) c.Check(errors.ErrorStack(checkErr), Equals, "") - s.checkDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) - - test = &tests[14] - doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) - c.Check(errors.ErrorStack(checkErr), Equals, "") - s.checkDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) - - test = &tests[15] - doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) - c.Check(errors.ErrorStack(checkErr), Equals, "") - s.checkDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) + s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) } func (s *testDDLSuite) TestIgnorableSpec(c *C) { From 4a8aefa69808ad0ba0e505bf5367fce285b16cb9 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Thu, 27 Dec 2018 15:03:26 +0800 Subject: [PATCH 13/19] Fix CI --- ddl/column.go | 29 ++++++++++------------------- ddl/db_test.go | 28 +++++++++++++++------------- 2 files changed, 25 insertions(+), 32 deletions(-) diff --git a/ddl/column.go b/ddl/column.go index 9d82e606de4cf..869f069a22790 100644 --- a/ddl/column.go +++ b/ddl/column.go @@ -223,6 +223,16 @@ func onDropColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { job.State = model.JobStateCancelled return ver, errors.Trace(err) } + if job.IsRollingback() && job.Type == model.ActionDropColumn { + // StateNone means when the job is not running yet. + if job.SchemaState == model.StateNone { + job.State = model.JobStateCancelled + return ver, errors.Trace(errCancelledDDLJob) + } + // In the state of drop column `write only -> delete only -> reorganization`, + // We can not rollback now, so just continue to drop column. + job.State = model.JobStateRunning + } colInfo := model.FindColumnInfo(tblInfo.Columns, colName.L) if colInfo == nil { @@ -234,13 +244,6 @@ func onDropColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { return ver, errors.Trace(err) } - if job.IsRollingback() { - if err = rollingbackDropColumn(job, colInfo); err != nil { - job.State = model.JobStateCancelled - return ver, errors.Trace(err) - } - } - originalState := colInfo.State switch colInfo.State { case model.StatePublic: @@ -282,18 +285,6 @@ func onDropColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { return ver, errors.Trace(err) } -func rollingbackDropColumn(job *model.Job, colInfo *model.ColumnInfo) error { - if job.Type == model.ActionDropColumn { - if colInfo.State == model.StatePublic { - return errCancelledDDLJob - } - // In the state of drop column `write only -> delete only -> reorganization`, - // We can not rollback now, so just continue to drop column. - job.State = model.JobStateRunning - } - return errCancelledDDLJob -} - func onSetDefaultValue(t *meta.Meta, job *model.Job) (ver int64, _ error) { newCol := &model.ColumnInfo{} err := job.DecodeArgs(newCol) diff --git a/ddl/db_test.go b/ddl/db_test.go index 95d795380e993..5ceb47775021a 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -887,19 +887,20 @@ LOOP: // TestCancelDropColumn tests cancel ddl job which type is drop column. func (s *testDBSuite) TestCancelDropColumn(c *C) { s.tk = testkit.NewTestKit(c, s.store) - s.mustExec(c, "use test_db") + s.tk.MustExec("use " + s.schemaName) s.mustExec(c, "drop table if exists test_drop_column") s.mustExec(c, "create table test_drop_column(c1 int, c2 int)") defer s.mustExec(c, "drop table test_drop_column;") testCases := []struct { + needAddColumn bool jobState model.JobState JobSchemaState model.SchemaState cancelSucc bool }{ - {model.JobStateNone, model.StateNone, true}, - {model.JobStateRunning, model.StateWriteOnly, false}, - {model.JobStateRunning, model.StateDeleteOnly, false}, - {model.JobStateRunning, model.StateDeleteReorganization, false}, + {true, model.JobStateNone, model.StateNone, true}, + {false, model.JobStateRunning, model.StateWriteOnly, false}, + {true, model.JobStateRunning, model.StateDeleteOnly, false}, + {true, model.JobStateRunning, model.StateDeleteReorganization, false}, } var checkErr error oldReorgWaitTimeout := ddl.ReorgWaitTimeout @@ -914,7 +915,7 @@ func (s *testDBSuite) TestCancelDropColumn(c *C) { jobID = job.ID hookCtx := mock.NewContext() hookCtx.Store = s.store - err := hookCtx.NewTxn(context.Background()) + err := hookCtx.NewTxn(context.TODO()) if err != nil { checkErr = errors.Trace(err) return @@ -936,15 +937,16 @@ func (s *testDBSuite) TestCancelDropColumn(c *C) { checkErr = txn.Commit(context.Background()) } } + originalHook := s.dom.DDL().GetHook() s.dom.DDL().(ddl.DDLForTest).SetHook(hook) + var err1 error for i := range testCases { testCase = &testCases[i] - s.mustExec(c, "alter table test_drop_column add column c3 int") - rs, err := s.tk.Exec("alter table test_drop_column drop column c3") - if rs != nil { - rs.Close() + if testCase.needAddColumn { + s.mustExec(c, "alter table test_drop_column add column c3 int") } + _, err1 = s.tk.Exec("alter table test_drop_column drop column c3") var col1 *table.Column t := s.testGetTable(c, "test_drop_column") for _, col := range t.Cols() { @@ -955,12 +957,12 @@ func (s *testDBSuite) TestCancelDropColumn(c *C) { } if testCase.cancelSucc { c.Assert(checkErr, IsNil) - c.Assert(err, NotNil) c.Assert(col1, NotNil) - c.Assert(err.Error(), Equals, "[ddl:12]cancelled DDL job") + c.Assert(col1.Name.L, Equals, "c3") + c.Assert(err1.Error(), Equals, "[ddl:12]cancelled DDL job") } else { c.Assert(col1, IsNil) - c.Assert(err, IsNil) + c.Assert(err1, IsNil) c.Assert(checkErr, NotNil) c.Assert(checkErr.Error(), Equals, admin.ErrCannotCancelDDLJob.GenWithStackByArgs(jobID).Error()) } From 73737c0e634c303c31bc53158bcb4dc6e1386aef Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Thu, 27 Dec 2018 16:24:52 +0800 Subject: [PATCH 14/19] change rollingbackDropColumn function --- ddl/column.go | 11 ----------- ddl/rollingback.go | 34 +++++++++++++++++++++++++++++++++- util/admin/admin.go | 2 +- 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/ddl/column.go b/ddl/column.go index 869f069a22790..4363630565acb 100644 --- a/ddl/column.go +++ b/ddl/column.go @@ -223,17 +223,6 @@ func onDropColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { job.State = model.JobStateCancelled return ver, errors.Trace(err) } - if job.IsRollingback() && job.Type == model.ActionDropColumn { - // StateNone means when the job is not running yet. - if job.SchemaState == model.StateNone { - job.State = model.JobStateCancelled - return ver, errors.Trace(errCancelledDDLJob) - } - // In the state of drop column `write only -> delete only -> reorganization`, - // We can not rollback now, so just continue to drop column. - job.State = model.JobStateRunning - } - colInfo := model.FindColumnInfo(tblInfo.Columns, colName.L) if colInfo == nil { job.State = model.JobStateCancelled diff --git a/ddl/rollingback.go b/ddl/rollingback.go index 230fcb772c26a..455b430bea595 100644 --- a/ddl/rollingback.go +++ b/ddl/rollingback.go @@ -118,6 +118,38 @@ func rollingbackAddColumn(t *meta.Meta, job *model.Job) (ver int64, err error) { return ver, errCancelledDDLJob } +func rollingbackDropColumn(t *meta.Meta, job *model.Job) (ver int64, err error) { + tblInfo, err := getTableInfo(t, job, job.SchemaID) + if err != nil { + return ver, errors.Trace(err) + } + + var colName model.CIStr + err = job.DecodeArgs(&colName) + if err != nil { + job.State = model.JobStateCancelled + return ver, errors.Trace(err) + } + + colInfo := model.FindColumnInfo(tblInfo.Columns, colName.L) + if colInfo == nil { + job.State = model.JobStateCancelled + return ver, ErrCantDropFieldOrKey.GenWithStack("column %s doesn't exist", colName) + } + + // StatePublic means when the job is not running yet. + if colInfo.State == model.StatePublic { + job.State = model.JobStateCancelled + } else { + // In the state of drop column `write only -> delete only -> reorganization`, + // We can not rollback now, so just continue to drop column. + job.State = model.JobStateRunning + return ver, errors.Trace(nil) + } + job.FinishTableJob(model.JobStateRollbackDone, model.StatePublic, ver, tblInfo) + return ver, errors.Trace(errCancelledDDLJob) +} + func rollingbackDropIndex(t *meta.Meta, job *model.Job) (ver int64, err error) { schemaID := job.SchemaID tblInfo, err := getTableInfo(t, job, schemaID) @@ -181,7 +213,7 @@ func convertJob2RollbackJob(w *worker, d *ddlCtx, t *meta.Meta, job *model.Job) case model.ActionAddColumn: ver, err = rollingbackAddColumn(t, job) case model.ActionDropColumn: - job.State = model.JobStateRollingback + ver, err = rollingbackDropColumn(t, job) case model.ActionAddIndex: ver, err = rollingbackAddindex(w, d, t, job) case model.ActionDropIndex: diff --git a/util/admin/admin.go b/util/admin/admin.go index 203c31307ab67..52e3eae298280 100644 --- a/util/admin/admin.go +++ b/util/admin/admin.go @@ -92,7 +92,7 @@ func isJobRollbackable(job *model.Job, id int64) error { return ErrCannotCancelDDLJob.GenWithStackByArgs(id) } case model.ActionDropColumn: - if job.State != model.JobStateNone { + if job.SchemaState != model.StateNone { return ErrCannotCancelDDLJob.GenWithStackByArgs(id) } case model.ActionDropSchema, model.ActionDropTable: From e9ff8205e02df7f6154cc5039b8a7aa38e9d4030 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Thu, 27 Dec 2018 16:59:33 +0800 Subject: [PATCH 15/19] Address comment --- ddl/ddl_worker_test.go | 6 +++--- ddl/rollingback.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ddl/ddl_worker_test.go b/ddl/ddl_worker_test.go index c92a5934c74c9..20484f69288c2 100644 --- a/ddl/ddl_worker_test.go +++ b/ddl/ddl_worker_test.go @@ -376,9 +376,9 @@ func buildCancelJobTests(firstID int64) []testCancelJob { {act: model.ActionCreateSchema, jobIDs: []int64{firstID + 12}, cancelRetErrs: noErrs, cancelState: model.StateNone, ddlRetErr: err}, {act: model.ActionDropColumn, jobIDs: []int64{firstID + 13}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 13)}, cancelState: model.StateDeleteOnly, ddlRetErr: err}, - {act: model.ActionDropColumn, jobIDs: []int64{firstID + 14}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 14)}, cancelState: model.StateWriteOnly, ddlRetErr: err}, - {act: model.ActionDropColumn, jobIDs: []int64{firstID + 15}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 15)}, cancelState: model.StateWriteReorganization, ddlRetErr: err}, - {act: model.ActionDropColumn, jobIDs: []int64{firstID + 16}, cancelRetErrs: noErrs, cancelState: model.StateNone, ddlRetErr: err}, + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 15}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 15)}, cancelState: model.StateWriteOnly, ddlRetErr: err}, + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 16}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 16)}, cancelState: model.StateWriteReorganization, ddlRetErr: err}, + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 17}, cancelRetErrs: noErrs, cancelState: model.StateNone, ddlRetErr: err}, } return tests diff --git a/ddl/rollingback.go b/ddl/rollingback.go index 455b430bea595..2451a39d5ab85 100644 --- a/ddl/rollingback.go +++ b/ddl/rollingback.go @@ -212,10 +212,10 @@ func convertJob2RollbackJob(w *worker, d *ddlCtx, t *meta.Meta, job *model.Job) switch job.Type { case model.ActionAddColumn: ver, err = rollingbackAddColumn(t, job) - case model.ActionDropColumn: - ver, err = rollingbackDropColumn(t, job) case model.ActionAddIndex: ver, err = rollingbackAddindex(w, d, t, job) + case model.ActionDropColumn: + ver, err = rollingbackDropColumn(t, job) case model.ActionDropIndex: ver, err = rollingbackDropIndex(t, job) case model.ActionDropTable, model.ActionDropSchema: From 524d3e2d73b4e9c5d35f24b5546a342fc6fcd20e Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Thu, 27 Dec 2018 17:17:55 +0800 Subject: [PATCH 16/19] Add cancel job test --- ddl/ddl_worker_test.go | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/ddl/ddl_worker_test.go b/ddl/ddl_worker_test.go index 20484f69288c2..4c73aa12e6151 100644 --- a/ddl/ddl_worker_test.go +++ b/ddl/ddl_worker_test.go @@ -560,11 +560,33 @@ func (s *testDDLSuite) TestCancelJob(c *C) { // for drop column. test = &tests[13] - dropColName := "c2" + tblInfo2 := testTableInfo(c, d, "t_drop_column", 4) + dropColName := "c1" dropColumnArgs := []interface{}{model.NewCIStr(dropColName)} - doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo2.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) c.Check(errors.ErrorStack(checkErr), Equals, "") - s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) + s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo2.ID, dropColName, false) + + test = &tests[15] + dropColName = "c2" + dropColumnArgs = []interface{}{model.NewCIStr(dropColName)} + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo2.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) + c.Check(errors.ErrorStack(checkErr), Equals, "") + s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo2.ID, dropColName, false) + + test = &tests[16] + dropColName = "c3" + dropColumnArgs = []interface{}{model.NewCIStr(dropColName)} + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo2.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) + c.Check(errors.ErrorStack(checkErr), Equals, "") + s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo2.ID, dropColName, false) + + test = &tests[17] + dropColName = "c4" + dropColumnArgs = []interface{}{model.NewCIStr(dropColName)} + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo2.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) + c.Check(errors.ErrorStack(checkErr), Equals, "") + s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo2.ID, dropColName, true) } func (s *testDDLSuite) TestIgnorableSpec(c *C) { From 2667c99ac2b84580c9e3dbfe37bb47c89e188b88 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Thu, 27 Dec 2018 19:54:01 +0800 Subject: [PATCH 17/19] Fix CI --- ddl/ddl_worker_test.go | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/ddl/ddl_worker_test.go b/ddl/ddl_worker_test.go index 4c73aa12e6151..daa0df731416f 100644 --- a/ddl/ddl_worker_test.go +++ b/ddl/ddl_worker_test.go @@ -376,9 +376,8 @@ func buildCancelJobTests(firstID int64) []testCancelJob { {act: model.ActionCreateSchema, jobIDs: []int64{firstID + 12}, cancelRetErrs: noErrs, cancelState: model.StateNone, ddlRetErr: err}, {act: model.ActionDropColumn, jobIDs: []int64{firstID + 13}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 13)}, cancelState: model.StateDeleteOnly, ddlRetErr: err}, - {act: model.ActionDropColumn, jobIDs: []int64{firstID + 15}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 15)}, cancelState: model.StateWriteOnly, ddlRetErr: err}, - {act: model.ActionDropColumn, jobIDs: []int64{firstID + 16}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 16)}, cancelState: model.StateWriteReorganization, ddlRetErr: err}, - {act: model.ActionDropColumn, jobIDs: []int64{firstID + 17}, cancelRetErrs: noErrs, cancelState: model.StateNone, ddlRetErr: err}, + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 14}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 14)}, cancelState: model.StateWriteOnly, ddlRetErr: err}, + {act: model.ActionDropColumn, jobIDs: []int64{firstID + 15}, cancelRetErrs: []error{admin.ErrCancelFinishedDDLJob.GenWithStackByArgs(firstID + 15)}, cancelState: model.StateWriteReorganization, ddlRetErr: err}, } return tests @@ -559,34 +558,22 @@ func (s *testDDLSuite) TestCancelJob(c *C) { testCheckSchemaState(c, d, dbInfo1, model.StateNone) // for drop column. - test = &tests[13] - tblInfo2 := testTableInfo(c, d, "t_drop_column", 4) - dropColName := "c1" + test = &tests[10] + dropColName := "c2" dropColumnArgs := []interface{}{model.NewCIStr(dropColName)} - doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo2.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) c.Check(errors.ErrorStack(checkErr), Equals, "") - s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo2.ID, dropColName, false) + s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) - test = &tests[15] - dropColName = "c2" - dropColumnArgs = []interface{}{model.NewCIStr(dropColName)} - doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo2.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) + test = &tests[11] + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) c.Check(errors.ErrorStack(checkErr), Equals, "") - s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo2.ID, dropColName, false) + s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) - test = &tests[16] - dropColName = "c3" - dropColumnArgs = []interface{}{model.NewCIStr(dropColName)} - doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo2.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) + test = &tests[12] + doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) c.Check(errors.ErrorStack(checkErr), Equals, "") - s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo2.ID, dropColName, false) - - test = &tests[17] - dropColName = "c4" - dropColumnArgs = []interface{}{model.NewCIStr(dropColName)} - doDDLJobErrWithSchemaState(ctx, d, c, dbInfo.ID, tblInfo2.ID, model.ActionDropColumn, dropColumnArgs, &cancelState) - c.Check(errors.ErrorStack(checkErr), Equals, "") - s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo2.ID, dropColName, true) + s.checkCancelDropColumn(c, d, dbInfo.ID, tblInfo.ID, dropColName, false) } func (s *testDDLSuite) TestIgnorableSpec(c *C) { From 57d0e7e57fa24c1827893d31148f3538e1a15bb6 Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Thu, 27 Dec 2018 20:01:16 +0800 Subject: [PATCH 18/19] Go fmt --- ddl/column.go | 1 + 1 file changed, 1 insertion(+) diff --git a/ddl/column.go b/ddl/column.go index 4363630565acb..eefcaa97a0fd9 100644 --- a/ddl/column.go +++ b/ddl/column.go @@ -223,6 +223,7 @@ func onDropColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { job.State = model.JobStateCancelled return ver, errors.Trace(err) } + colInfo := model.FindColumnInfo(tblInfo.Columns, colName.L) if colInfo == nil { job.State = model.JobStateCancelled From e773c46dac355f6a61fde50f3c3b525f6f04872b Mon Sep 17 00:00:00 2001 From: ciscoxll Date: Thu, 27 Dec 2018 20:36:31 +0800 Subject: [PATCH 19/19] Fix CI --- ddl/column.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddl/column.go b/ddl/column.go index eefcaa97a0fd9..6329b616158a0 100644 --- a/ddl/column.go +++ b/ddl/column.go @@ -223,7 +223,7 @@ func onDropColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { job.State = model.JobStateCancelled return ver, errors.Trace(err) } - + colInfo := model.FindColumnInfo(tblInfo.Columns, colName.L) if colInfo == nil { job.State = model.JobStateCancelled