From 14c7dce3da1ea5e316537b212fb229cecbabecf0 Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Sun, 29 Jan 2023 16:27:54 +0800 Subject: [PATCH] ddl: persist index info after changing backfill state (#40229) (#40305) close pingcap/tidb#40217 --- ddl/concurrentddltest/BUILD.bazel | 26 ----- ddl/concurrentddltest/main_test.go | 45 -------- ddl/concurrentddltest/switch_test.go | 149 --------------------------- ddl/index.go | 5 +- ddl/multi_schema_change_test.go | 11 ++ executor/batch_checker.go | 2 +- 6 files changed, 16 insertions(+), 222 deletions(-) delete mode 100644 ddl/concurrentddltest/BUILD.bazel delete mode 100644 ddl/concurrentddltest/main_test.go delete mode 100644 ddl/concurrentddltest/switch_test.go diff --git a/ddl/concurrentddltest/BUILD.bazel b/ddl/concurrentddltest/BUILD.bazel deleted file mode 100644 index d5acc141896c5..0000000000000 --- a/ddl/concurrentddltest/BUILD.bazel +++ /dev/null @@ -1,26 +0,0 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_test") - -go_test( - name = "concurrentddltest_test", - timeout = "short", - srcs = [ - "main_test.go", - "switch_test.go", - ], - flaky = True, - race = "on", - shard_count = 2, - deps = [ - "//config", - "//ddl", - "//kv", - "//meta", - "//sessionctx/variable", - "//testkit", - "//testkit/testsetup", - "//util", - "@com_github_stretchr_testify//require", - "@org_uber_go_atomic//:atomic", - "@org_uber_go_goleak//:goleak", - ], -) diff --git a/ddl/concurrentddltest/main_test.go b/ddl/concurrentddltest/main_test.go deleted file mode 100644 index 4ab7e96eab2ae..0000000000000 --- a/ddl/concurrentddltest/main_test.go +++ /dev/null @@ -1,45 +0,0 @@ -// Copyright 2022 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, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package concurrentddltest - -import ( - "testing" - "time" - - "github.com/pingcap/tidb/config" - "github.com/pingcap/tidb/ddl" - "github.com/pingcap/tidb/testkit/testsetup" - "go.uber.org/goleak" -) - -func TestMain(m *testing.M) { - testsetup.SetupForCommonTest() - - config.UpdateGlobal(func(conf *config.Config) { - conf.TiKVClient.AsyncCommit.SafeWindow = 0 - conf.TiKVClient.AsyncCommit.AllowedClockDrift = 0 - }) - - ddl.SetWaitTimeWhenErrorOccurred(time.Microsecond) - - opts := []goleak.Option{ - goleak.IgnoreTopFunction("github.com/golang/glog.(*loggingT).flushDaemon"), - goleak.IgnoreTopFunction("github.com/lestrrat-go/httprc.runFetchWorker"), - goleak.IgnoreTopFunction("go.etcd.io/etcd/client/pkg/v3/logutil.(*MergeLogger).outputLoop"), - goleak.IgnoreTopFunction("go.opencensus.io/stats/view.(*worker).start"), - } - - goleak.VerifyTestMain(m, opts...) -} diff --git a/ddl/concurrentddltest/switch_test.go b/ddl/concurrentddltest/switch_test.go deleted file mode 100644 index 6cd26811008e6..0000000000000 --- a/ddl/concurrentddltest/switch_test.go +++ /dev/null @@ -1,149 +0,0 @@ -// Copyright 2022 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, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package concurrentddltest - -import ( - "context" - "fmt" - "math/rand" - "testing" - "time" - - "github.com/pingcap/tidb/kv" - "github.com/pingcap/tidb/meta" - "github.com/pingcap/tidb/sessionctx/variable" - "github.com/pingcap/tidb/testkit" - "github.com/pingcap/tidb/util" - "github.com/stretchr/testify/require" - "go.uber.org/atomic" -) - -func TestConcurrentDDLSwitch(t *testing.T) { - store := testkit.CreateMockStore(t) - - type table struct { - columnIdx int - indexIdx int - } - - var tables []*table - tblCount := 20 - for i := 0; i < tblCount; i++ { - tables = append(tables, &table{1, 0}) - } - - tk := testkit.NewTestKit(t, store) - tk.MustExec("use test") - tk.MustExec("set global tidb_enable_metadata_lock=0") - tk.MustExec("set @@global.tidb_ddl_reorg_worker_cnt=1") - tk.MustExec("set @@global.tidb_ddl_reorg_batch_size=32") - - for i := range tables { - tk.MustExec(fmt.Sprintf("create table t%d (col0 int)", i)) - for j := 0; j < 1000; j++ { - tk.MustExec(fmt.Sprintf("insert into t%d values (%d)", i, j)) - } - } - - ddls := make([]string, 0, tblCount) - ddlCount := 100 - for i := 0; i < ddlCount; i++ { - tblIdx := rand.Intn(tblCount) - if rand.Intn(2) == 0 { - ddls = append(ddls, fmt.Sprintf("alter table t%d add index idx%d (col0)", tblIdx, tables[tblIdx].indexIdx)) - tables[tblIdx].indexIdx++ - } else { - ddls = append(ddls, fmt.Sprintf("alter table t%d add column col%d int", tblIdx, tables[tblIdx].columnIdx)) - tables[tblIdx].columnIdx++ - } - } - - c := atomic.NewInt32(0) - ch := make(chan struct{}) - go func() { - var wg util.WaitGroupWrapper - for i := range ddls { - wg.Add(1) - go func(idx int) { - tk := testkit.NewTestKit(t, store) - tk.MustExec("use test") - tk.MustExec(ddls[idx]) - c.Add(1) - wg.Done() - }(i) - } - wg.Wait() - ch <- struct{}{} - }() - - // sleep 2s to make sure the ddl jobs is into table. - time.Sleep(2 * time.Second) - ticker := time.NewTicker(time.Second) - count := 0 - done := false - for !done { - select { - case <-ch: - done = true - case <-ticker.C: - var b bool - var err error - err = kv.RunInNewTxn(kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL), store, false, func(ctx context.Context, txn kv.Transaction) error { - b, err = meta.NewMeta(txn).IsConcurrentDDL() - return err - }) - require.NoError(t, err) - rs, err := testkit.NewTestKit(t, store).Exec(fmt.Sprintf("set @@global.tidb_enable_concurrent_ddl=%t", !b)) - if rs != nil { - require.NoError(t, rs.Close()) - } - if err == nil { - count++ - if b { - tk := testkit.NewTestKit(t, store) - tk.Session().GetSessionVars().MemQuotaQuery = -1 - tk.MustQuery("select count(*) from mysql.tidb_ddl_job").Check(testkit.Rows("0")) - tk.MustQuery("select count(*) from mysql.tidb_ddl_reorg").Check(testkit.Rows("0")) - } - } - } - } - - require.Equal(t, int32(ddlCount), c.Load()) - require.Greater(t, count, 0) - - tk = testkit.NewTestKit(t, store) - tk.Session().GetSessionVars().MemQuotaQuery = -1 - tk.MustExec("use test") - for i, tbl := range tables { - tk.MustQuery(fmt.Sprintf("select count(*) from information_schema.columns where TABLE_SCHEMA = 'test' and TABLE_NAME = 't%d'", i)).Check(testkit.Rows(fmt.Sprintf("%d", tbl.columnIdx))) - tk.MustExec(fmt.Sprintf("admin check table t%d", i)) - for j := 0; j < tbl.indexIdx; j++ { - tk.MustExec(fmt.Sprintf("admin check index t%d idx%d", i, j)) - } - } -} - -func TestConcurrentDDLSwitchWithMDL(t *testing.T) { - if !variable.EnableConcurrentDDL.Load() { - t.Skip("skip test if concurrent DDL is disabled") - } - store := testkit.CreateMockStore(t) - tk := testkit.NewTestKit(t, store) - tk.MustGetErrMsg("set global tidb_enable_concurrent_ddl=off", "can not disable concurrent ddl when metadata lock is enabled") - tk.MustExec("set global tidb_enable_metadata_lock=0") - tk.MustExec("set global tidb_enable_concurrent_ddl=off") - tk.MustExec("create table test.t(a int)") -} diff --git a/ddl/index.go b/ddl/index.go index eba86473acba6..478338a78adc5 100644 --- a/ddl/index.go +++ b/ddl/index.go @@ -774,6 +774,9 @@ func doReorgWorkForCreateIndexMultiSchema(w *worker, d *ddlCtx, t *meta.Meta, jo done, ver, err = doReorgWorkForCreateIndex(w, d, t, job, tbl, indexInfo) if done { job.MarkNonRevertible() + if err == nil { + ver, err = updateVersionAndTableInfo(d, t, job, tbl.Meta(), true) + } } // We need another round to wait for all the others sub-jobs to finish. return false, ver, err @@ -857,7 +860,7 @@ func doReorgWorkForCreateIndex(w *worker, d *ddlCtx, t *meta.Meta, job *model.Jo return false, ver, err } indexInfo.BackfillState = model.BackfillStateInapplicable // Prevent double-write on this index. - return true, ver, nil + return true, ver, err default: return false, 0, dbterror.ErrInvalidDDLState.GenWithStackByArgs("backfill", indexInfo.BackfillState) } diff --git a/ddl/multi_schema_change_test.go b/ddl/multi_schema_change_test.go index 0f8aeca87802c..bf4aef776d291 100644 --- a/ddl/multi_schema_change_test.go +++ b/ddl/multi_schema_change_test.go @@ -1208,6 +1208,17 @@ func TestMultiSchemaChangeSchemaVersion(t *testing.T) { dom.DDL().SetHook(originHook) } +func TestMultiSchemaChangeAddIndexChangeColumn(t *testing.T) { + store := testkit.CreateMockStore(t) + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test;") + tk.MustExec("CREATE TABLE t (a SMALLINT DEFAULT '30219', b TIME NULL DEFAULT '02:45:06', PRIMARY KEY (a));") + tk.MustExec("ALTER TABLE t ADD unique INDEX idx4 (b), change column a e MEDIUMINT DEFAULT '5280454' FIRST;") + tk.MustExec("insert ignore into t (e) values (5586359),(501788),(-5961048),(220083),(-4917129),(-7267211),(7750448);") + tk.MustQuery("select * from t;").Check(testkit.Rows("5586359 02:45:06")) + tk.MustExec("admin check table t;") +} + func TestMultiSchemaChangeMixedWithUpdate(t *testing.T) { store, dom := testkit.CreateMockStoreAndDomain(t) tk := testkit.NewTestKit(t, store) diff --git a/executor/batch_checker.go b/executor/batch_checker.go index 79a6748b2d5c3..70466cbd22cd0 100644 --- a/executor/batch_checker.go +++ b/executor/batch_checker.go @@ -181,7 +181,7 @@ func getKeysNeedCheckOneRow(ctx sessionctx.Context, t table.Table, row []types.D continue } // If index is used ingest ways, then we should check key from temp index. - if v.Meta().BackfillState != model.BackfillStateInapplicable { + if v.Meta().State != model.StatePublic && v.Meta().BackfillState != model.BackfillStateInapplicable { _, key, _ = tables.GenTempIdxKeyByState(v.Meta(), key) } colValStr, err1 := formatDataForDupError(colVals)