From e1a6b1d63399ce06b00132bc55052fb2c4e8a7fd Mon Sep 17 00:00:00 2001 From: wjHuang Date: Thu, 16 May 2024 16:49:13 +0800 Subject: [PATCH] *: check delete unique key's handle to handle corner case (#52975) close pingcap/tidb#52914 --- pkg/ddl/index.go | 8 +++ pkg/ddl/ingest/backend.go | 8 +++ pkg/table/tables/index.go | 25 +++++++-- tests/realtikvtest/addindextest/BUILD.bazel | 5 ++ .../addindextest/add_index_test.go | 54 +++++++++++++++++++ 5 files changed, 97 insertions(+), 3 deletions(-) diff --git a/pkg/ddl/index.go b/pkg/ddl/index.go index 6c753c00f9922..94f6297ca96fa 100644 --- a/pkg/ddl/index.go +++ b/pkg/ddl/index.go @@ -875,6 +875,11 @@ func doReorgWorkForCreateIndex(w *worker, d *ddlCtx, t *meta.Meta, job *model.Jo ver, err = updateVersionAndTableInfo(d, t, job, tbl.Meta(), true) return false, ver, errors.Trace(err) case model.BackfillStateReadyToMerge: + failpoint.Inject("mockDMLExecutionStateBeforeMerge", func(_ failpoint.Value) { + if MockDMLExecutionStateBeforeMerge != nil { + MockDMLExecutionStateBeforeMerge() + } + }) logutil.DDLLogger().Info("index backfill state ready to merge", zap.Int64("job ID", job.ID), zap.String("table", tbl.Meta().Name.O), @@ -1924,6 +1929,9 @@ var MockDMLExecutionStateMerging func() // MockDMLExecutionStateBeforeImport is only used for test. var MockDMLExecutionStateBeforeImport func() +// MockDMLExecutionStateBeforeMerge is only used for test. +var MockDMLExecutionStateBeforeMerge func() + func (w *worker) addPhysicalTableIndex(t table.PhysicalTable, reorgInfo *reorgInfo) error { if reorgInfo.mergingTmpIdx { logutil.DDLLogger().Info("start to merge temp index", zap.Stringer("job", reorgInfo.Job), zap.Stringer("reorgInfo", reorgInfo)) diff --git a/pkg/ddl/ingest/backend.go b/pkg/ddl/ingest/backend.go index de9c11f504ff4..977b00b139562 100644 --- a/pkg/ddl/ingest/backend.go +++ b/pkg/ddl/ingest/backend.go @@ -41,6 +41,9 @@ import ( "go.uber.org/zap" ) +// MockDMLExecutionStateBeforeImport is a failpoint to mock the DML execution state before import. +var MockDMLExecutionStateBeforeImport func() + // BackendCtx is the backend context for one add index reorg task. type BackendCtx interface { // Register create a new engineInfo for each index ID and register it to the @@ -237,6 +240,11 @@ func (bc *litBackendCtx) Flush(mode FlushMode) (flushed, imported bool, errIdxID } }() } + failpoint.Inject("mockDMLExecutionStateBeforeImport", func(_ failpoint.Value) { + if MockDMLExecutionStateBeforeImport != nil { + MockDMLExecutionStateBeforeImport() + } + }) for indexID, ei := range bc.engines { if err = bc.unsafeImportAndReset(ei); err != nil { diff --git a/pkg/table/tables/index.go b/pkg/table/tables/index.go index f359f23d52cf7..2929bbdb9e9ff 100644 --- a/pkg/table/tables/index.go +++ b/pkg/table/tables/index.go @@ -427,9 +427,28 @@ func (c *index) Delete(ctx table.MutateContext, txn kv.Transaction, indexedValue if distinct { if len(key) > 0 { - err = txn.GetMemBuffer().DeleteWithFlags(key, kv.SetNeedLocked) - if err != nil { - return err + okToDelete := true + if c.idxInfo.BackfillState != model.BackfillStateInapplicable { + // #52914: the delete key is covered by the new ingested key, which shouldn't be deleted. + originVal, err := getKeyInTxn(context.TODO(), txn, key) + if err != nil { + return err + } + if len(originVal) > 0 { + oh, err := tablecodec.DecodeHandleInUniqueIndexValue(originVal, c.tblInfo.IsCommonHandle) + if err != nil { + return err + } + if !h.Equal(oh) { + okToDelete = false + } + } + } + if okToDelete { + err = txn.GetMemBuffer().DeleteWithFlags(key, kv.SetNeedLocked) + if err != nil { + return err + } } } if len(tempKey) > 0 { diff --git a/tests/realtikvtest/addindextest/BUILD.bazel b/tests/realtikvtest/addindextest/BUILD.bazel index 34666d072b471..a2b7a11e4bf54 100644 --- a/tests/realtikvtest/addindextest/BUILD.bazel +++ b/tests/realtikvtest/addindextest/BUILD.bazel @@ -13,10 +13,15 @@ go_test( ], deps = [ "//pkg/config", + "//pkg/ddl", "//pkg/ddl/ingest", + "//pkg/ddl/util/callback", + "//pkg/parser/model", "//pkg/testkit", "//tests/realtikvtest", "//tests/realtikvtest/addindextestutil", + "@com_github_pingcap_failpoint//:failpoint", + "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@com_github_tikv_client_go_v2//tikv", ], diff --git a/tests/realtikvtest/addindextest/add_index_test.go b/tests/realtikvtest/addindextest/add_index_test.go index b58df0961377f..8a5750274ff10 100644 --- a/tests/realtikvtest/addindextest/add_index_test.go +++ b/tests/realtikvtest/addindextest/add_index_test.go @@ -20,11 +20,16 @@ import ( "path/filepath" "testing" + "github.com/pingcap/failpoint" "github.com/pingcap/tidb/pkg/config" + "github.com/pingcap/tidb/pkg/ddl" "github.com/pingcap/tidb/pkg/ddl/ingest" + "github.com/pingcap/tidb/pkg/ddl/util/callback" + "github.com/pingcap/tidb/pkg/parser/model" "github.com/pingcap/tidb/pkg/testkit" "github.com/pingcap/tidb/tests/realtikvtest" "github.com/pingcap/tidb/tests/realtikvtest/addindextestutil" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/tikv/client-go/v2/tikv" ) @@ -200,3 +205,52 @@ func TestLitBackendCtxMgr(t *testing.T) { _, ok = mgr.Load(jobID) require.False(t, ok) } + +func TestAddUniqueDuplicateIndexes(t *testing.T) { + store, dom := realtikvtest.CreateMockStoreAndDomainAndSetup(t) + + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + tk.MustExec("create table t(a int DEFAULT '-13202', b varchar(221) NOT NULL DEFAULT 'duplicatevalue', " + + "c int NOT NULL DEFAULT '0');") + + tk1 := testkit.NewTestKit(t, store) + tk1.MustExec("use test") + + d := dom.DDL() + originalCallback := d.GetHook() + defer d.SetHook(originalCallback) + callback := &callback.TestDDLCallback{} + + tk1.Exec("INSERT INTO t VALUES (-18585,'duplicatevalue',0);") + + onJobUpdatedExportedFunc := func(job *model.Job) { + switch job.SchemaState { + case model.StateDeleteOnly: + _, err := tk1.Exec("delete from t where c = 0;") + assert.NoError(t, err) + _, err = tk1.Exec("insert INTO t VALUES (-18585,'duplicatevalue',1);") + assert.NoError(t, err) + } + } + callback.OnJobUpdatedExported.Store(&onJobUpdatedExportedFunc) + d.SetHook(callback) + + tk3 := testkit.NewTestKit(t, store) + tk3.MustExec("use test") + ingest.MockDMLExecutionStateBeforeImport = func() { + tk3.MustExec("replace INTO t VALUES (-18585,'duplicatevalue',4);") + tk3.MustQuery("select * from t;").Check(testkit.Rows("-18585 duplicatevalue 1", "-18585 duplicatevalue 4")) + } + ddl.MockDMLExecutionStateBeforeMerge = func() { + tk3.MustQuery("select * from t;").Check(testkit.Rows("-18585 duplicatevalue 1", "-18585 duplicatevalue 4")) + tk3.MustExec("replace into t values (-18585,'duplicatevalue',0);") + } + + require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/pkg/ddl/ingest/mockDMLExecutionStateBeforeImport", "1*return")) + require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/pkg/ddl/mockDMLExecutionStateBeforeMerge", "return(true)")) + tk.MustExec("alter table t add unique index idx(b);") + tk.MustExec("admin check table t;") + require.NoError(t, failpoint.Disable("github.com/pingcap/tidb/pkg/ddl/ingest/mockDMLExecutionStateBeforeImport")) + require.NoError(t, failpoint.Disable("github.com/pingcap/tidb/pkg/ddl/mockDMLExecutionStateBeforeMerge")) +}