From 5f0b0ee380e75e47ce62f415b75cce58ab459d8e Mon Sep 17 00:00:00 2001 From: Lynn Date: Tue, 2 Jan 2018 17:03:58 +0800 Subject: [PATCH 1/2] ddl: fix rawArgs is overwritten (#5531) * ddl: fix rawArgs is overwritten --- ddl/column.go | 4 +++ ddl/ddl_worker.go | 21 ++++++++----- ddl/fail_test.go | 67 ++++++++++++++++++++++++++++++++++++++++++ inspectkv/inspectkv.go | 2 +- meta/meta.go | 11 ++++--- meta/meta_test.go | 2 +- 6 files changed, 93 insertions(+), 14 deletions(-) create mode 100644 ddl/fail_test.go diff --git a/ddl/column.go b/ddl/column.go index 63ab59e22c07e..c1ee1597d8cf8 100644 --- a/ddl/column.go +++ b/ddl/column.go @@ -98,6 +98,10 @@ func (d *ddl) onAddColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { if err != nil { return ver, errors.Trace(err) } + // gofail: var errorBeforeDecodeArgs bool + // if errorBeforeDecodeArgs { + // return ver, errors.New("occur an error before decode args") + // } col := &model.ColumnInfo{} pos := &ast.ColumnPosition{} diff --git a/ddl/ddl_worker.go b/ddl/ddl_worker.go index 3ac18b655d2ab..bd62573991db5 100644 --- a/ddl/ddl_worker.go +++ b/ddl/ddl_worker.go @@ -123,10 +123,16 @@ func (d *ddl) handleUpdateJobError(t *meta.Meta, job *model.Job, err error) erro // updateDDLJob updates the DDL job information. // Every time we enter another state except final state, we must call this function. -func (d *ddl) updateDDLJob(t *meta.Meta, job *model.Job, updateTS uint64) error { +func (d *ddl) updateDDLJob(t *meta.Meta, job *model.Job, updateTS uint64, meetErr bool) error { job.LastUpdateTS = int64(updateTS) - err := t.UpdateDDLJob(0, job) - return errors.Trace(err) + updateRawArgs := true + // If there is an error when running job and the RawArgs hasn't been decoded by DecodeArgs, + // so we shouldn't replace RawArgs with the marshaling Args. + if meetErr && (job.RawArgs != nil && job.Args == nil) { + log.Infof("[ddl] update DDL Job %s shouldn't update raw args", job) + updateRawArgs = false + } + return errors.Trace(t.UpdateDDLJob(0, job, updateRawArgs)) } // finishDDLJob deletes the finished DDL job in the ddl queue and puts it to history queue. @@ -211,12 +217,12 @@ func (d *ddl) handleDDLJobQueue() error { // If running job meets error, we will save this error in job Error // and retry later if the job is not cancelled. - schemaVer = d.runDDLJob(t, job) + schemaVer, err = d.runDDLJob(t, job) if job.IsCancelled() { err = d.finishDDLJob(t, job) return errors.Trace(err) } - err = d.updateDDLJob(t, job, txn.StartTS()) + err = d.updateDDLJob(t, job, txn.StartTS(), err != nil) return errors.Trace(d.handleUpdateJobError(t, job, err)) }) if err != nil { @@ -249,8 +255,8 @@ func chooseLeaseTime(t, max time.Duration) time.Duration { return t } -// runDDLJob runs a DDL job. It returns the current schema version in this transaction. -func (d *ddl) runDDLJob(t *meta.Meta, job *model.Job) (ver int64) { +// runDDLJob runs a DDL job. It returns the current schema version in this transaction and the error. +func (d *ddl) runDDLJob(t *meta.Meta, job *model.Job) (ver int64, err error) { log.Infof("[ddl] run DDL job %s", job) if job.IsFinished() { return @@ -273,7 +279,6 @@ func (d *ddl) runDDLJob(t *meta.Meta, job *model.Job) (ver int64) { job.State = model.JobRunning } - var err error switch job.Type { case model.ActionCreateSchema: ver, err = d.onCreateSchema(t, job) diff --git a/ddl/fail_test.go b/ddl/fail_test.go new file mode 100644 index 0000000000000..0f275677c3ccb --- /dev/null +++ b/ddl/fail_test.go @@ -0,0 +1,67 @@ +// Copyright 2018 PingCAP, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// See the License for the specific language governing permissions and +// limitations under the License. + +package ddl + +import ( + gofail "github.com/coreos/gofail/runtime" + . "github.com/pingcap/check" + "github.com/pingcap/tidb/ast" + "github.com/pingcap/tidb/model" + "github.com/pingcap/tidb/types" + "github.com/pingcap/tidb/util/testleak" + goctx "golang.org/x/net/context" +) + +func (s *testColumnChangeSuite) TestFailBeforeDecodeArgs(c *C) { + defer testleak.AfterTest(c)() + d := testNewDDL(goctx.Background(), nil, s.store, nil, nil, testLease) + defer d.Stop() + // create table t_fail (c1 int, c2 int); + tblInfo := testTableInfo(c, d, "t_fail", 2) + ctx := testNewContext(d) + err := ctx.NewTxn() + c.Assert(err, IsNil) + testCreateTable(c, ctx, d, s.dbInfo, tblInfo) + // insert t_fail values (1, 2); + originTable := testGetTable(c, d, s.dbInfo.ID, tblInfo.ID) + row := types.MakeDatums(1, 2) + _, err = originTable.AddRecord(ctx, row, false) + c.Assert(err, IsNil) + err = ctx.Txn().Commit(goctx.Background()) + c.Assert(err, IsNil) + + tc := &TestDDLCallback{} + first := true + stateCnt := 0 + tc.onJobRunBefore = func(job *model.Job) { + // It can be other schema states except failed schema state. + // This schema state can only appear once. + if job.SchemaState == model.StateWriteOnly { + stateCnt++ + } else if job.SchemaState == model.StateWriteReorganization { + if first { + gofail.Enable("github.com/pingcap/tidb/ddl/errorBeforeDecodeArgs", `return(true)`) + first = false + } else { + gofail.Disable("github.com/pingcap/tidb/ddl/errorBeforeDecodeArgs") + } + } + } + d.SetHook(tc) + defaultValue := int64(3) + job := testCreateColumn(c, ctx, d, s.dbInfo, tblInfo, "c3", &ast.ColumnPosition{Tp: ast.ColumnPositionNone}, defaultValue) + // Make sure the schema state only appears once. + c.Assert(stateCnt, Equals, 1) + testCheckJobDone(c, d, job, true) +} diff --git a/inspectkv/inspectkv.go b/inspectkv/inspectkv.go index c0acf18e8018d..4690cdb198a89 100644 --- a/inspectkv/inspectkv.go +++ b/inspectkv/inspectkv.go @@ -101,7 +101,7 @@ func CancelJobs(txn kv.Transaction, ids []int64) ([]error, error) { errs[i] = errors.Trace(err) continue } - err = t.UpdateDDLJob(int64(j), job) + err = t.UpdateDDLJob(int64(j), job, true) if err != nil { errs[i] = errors.Trace(err) } diff --git a/meta/meta.go b/meta/meta.go index 6850a7db36400..0caf3c6077c8b 100644 --- a/meta/meta.go +++ b/meta/meta.go @@ -486,8 +486,10 @@ func (m *Meta) GetDDLJob(index int64) (*model.Job, error) { return job, errors.Trace(err) } -func (m *Meta) updateDDLJob(index int64, job *model.Job, key []byte) error { - b, err := job.Encode(true) +// updateDDLJob updates the DDL job with index and key. +// updateRawArgs is used to determine whether to update the raw args when encode the job. +func (m *Meta) updateDDLJob(index int64, job *model.Job, key []byte, updateRawArgs bool) error { + b, err := job.Encode(updateRawArgs) if err != nil { return errors.Trace(err) } @@ -495,8 +497,9 @@ func (m *Meta) updateDDLJob(index int64, job *model.Job, key []byte) error { } // UpdateDDLJob updates the DDL job with index. -func (m *Meta) UpdateDDLJob(index int64, job *model.Job) error { - return m.updateDDLJob(index, job, mDDLJobListKey) +// updateRawArgs is used to determine whether to update the raw args when encode the job. +func (m *Meta) UpdateDDLJob(index int64, job *model.Job, updateRawArgs bool) error { + return m.updateDDLJob(index, job, mDDLJobListKey, updateRawArgs) } // DDLJobQueueLen returns the DDL job queue length. diff --git a/meta/meta_test.go b/meta/meta_test.go index f92df45885638..68c3130ab7d7a 100644 --- a/meta/meta_test.go +++ b/meta/meta_test.go @@ -280,7 +280,7 @@ func (s *testSuite) TestDDL(c *C) { c.Assert(err, IsNil) c.Assert(v, IsNil) job.ID = 2 - err = t.UpdateDDLJob(0, job) + err = t.UpdateDDLJob(0, job, true) c.Assert(err, IsNil) err = t.UpdateDDLReorgHandle(job, 1) From 24fbaf32c0f4e82165b0c9ec271775beda70159d Mon Sep 17 00:00:00 2001 From: zimulala Date: Fri, 5 Jan 2018 15:55:34 +0800 Subject: [PATCH 2/2] ddl: remove gofail test --- ddl/column.go | 4 --- ddl/fail_test.go | 67 ------------------------------------------------ 2 files changed, 71 deletions(-) delete mode 100644 ddl/fail_test.go diff --git a/ddl/column.go b/ddl/column.go index c1ee1597d8cf8..63ab59e22c07e 100644 --- a/ddl/column.go +++ b/ddl/column.go @@ -98,10 +98,6 @@ func (d *ddl) onAddColumn(t *meta.Meta, job *model.Job) (ver int64, _ error) { if err != nil { return ver, errors.Trace(err) } - // gofail: var errorBeforeDecodeArgs bool - // if errorBeforeDecodeArgs { - // return ver, errors.New("occur an error before decode args") - // } col := &model.ColumnInfo{} pos := &ast.ColumnPosition{} diff --git a/ddl/fail_test.go b/ddl/fail_test.go deleted file mode 100644 index 0f275677c3ccb..0000000000000 --- a/ddl/fail_test.go +++ /dev/null @@ -1,67 +0,0 @@ -// Copyright 2018 PingCAP, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// See the License for the specific language governing permissions and -// limitations under the License. - -package ddl - -import ( - gofail "github.com/coreos/gofail/runtime" - . "github.com/pingcap/check" - "github.com/pingcap/tidb/ast" - "github.com/pingcap/tidb/model" - "github.com/pingcap/tidb/types" - "github.com/pingcap/tidb/util/testleak" - goctx "golang.org/x/net/context" -) - -func (s *testColumnChangeSuite) TestFailBeforeDecodeArgs(c *C) { - defer testleak.AfterTest(c)() - d := testNewDDL(goctx.Background(), nil, s.store, nil, nil, testLease) - defer d.Stop() - // create table t_fail (c1 int, c2 int); - tblInfo := testTableInfo(c, d, "t_fail", 2) - ctx := testNewContext(d) - err := ctx.NewTxn() - c.Assert(err, IsNil) - testCreateTable(c, ctx, d, s.dbInfo, tblInfo) - // insert t_fail values (1, 2); - originTable := testGetTable(c, d, s.dbInfo.ID, tblInfo.ID) - row := types.MakeDatums(1, 2) - _, err = originTable.AddRecord(ctx, row, false) - c.Assert(err, IsNil) - err = ctx.Txn().Commit(goctx.Background()) - c.Assert(err, IsNil) - - tc := &TestDDLCallback{} - first := true - stateCnt := 0 - tc.onJobRunBefore = func(job *model.Job) { - // It can be other schema states except failed schema state. - // This schema state can only appear once. - if job.SchemaState == model.StateWriteOnly { - stateCnt++ - } else if job.SchemaState == model.StateWriteReorganization { - if first { - gofail.Enable("github.com/pingcap/tidb/ddl/errorBeforeDecodeArgs", `return(true)`) - first = false - } else { - gofail.Disable("github.com/pingcap/tidb/ddl/errorBeforeDecodeArgs") - } - } - } - d.SetHook(tc) - defaultValue := int64(3) - job := testCreateColumn(c, ctx, d, s.dbInfo, tblInfo, "c3", &ast.ColumnPosition{Tp: ast.ColumnPositionNone}, defaultValue) - // Make sure the schema state only appears once. - c.Assert(stateCnt, Equals, 1) - testCheckJobDone(c, d, job, true) -}