From 2fee84924b92cff57be4f13bae7667b60be7305a Mon Sep 17 00:00:00 2001 From: tangenta Date: Fri, 20 Sep 2024 16:44:30 +0800 Subject: [PATCH 1/2] This is an automated cherry-pick of #56162 Signed-off-by: ti-chi-bot --- ddl/index_merge_tmp.go | 34 ++++++++++++++++++---------- ddl/index_merge_tmp_test.go | 23 +++++++++++++++++++ pkg/ddl/tests/indexmerge/BUILD.bazel | 33 +++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 12 deletions(-) create mode 100644 pkg/ddl/tests/indexmerge/BUILD.bazel diff --git a/ddl/index_merge_tmp.go b/ddl/index_merge_tmp.go index 0150f0fb42b4c..1d72002b7fb27 100644 --- a/ddl/index_merge_tmp.go +++ b/ddl/index_merge_tmp.go @@ -51,41 +51,51 @@ func (w *mergeIndexWorker) batchCheckTemporaryUniqueKey(txn kv.Transaction, idxR } for i, key := range w.originIdxKeys { - if val, found := batchVals[string(key)]; found { + keyStr := string(key) + if val, found := batchVals[keyStr]; found { // Found a value in the original index key. - err := checkTempIndexKey(txn, idxRecords[i], val, w.table) + matchDeleted, err := checkTempIndexKey(txn, idxRecords[i], val, w.table) if err != nil { return errors.Trace(err) } + if matchDeleted { + // Delete from batchVals to prevent false-positive duplicate detection. + delete(batchVals, keyStr) + } } else if idxRecords[i].distinct { // The keys in w.batchCheckKeys also maybe duplicate, // so we need to backfill the not found key into `batchVals` map. - batchVals[string(key)] = idxRecords[i].vals + batchVals[keyStr] = idxRecords[i].vals } } return nil } -func checkTempIndexKey(txn kv.Transaction, tmpRec *temporaryIndexRecord, originIdxVal []byte, tblInfo table.Table) error { +// checkTempIndexKey determines whether there is a duplicated index key entry according to value of temp index. +// For non-delete temp record, if the index values mismatch, it is duplicated. +// For delete temp record, we decode the handle from the origin index value and temp index value. +// - if the handles matche, we can delete the index key. +// - otherwise, we further check if the row exists in the table. +func checkTempIndexKey(txn kv.Transaction, tmpRec *temporaryIndexRecord, originIdxVal []byte, tblInfo table.Table) (matchDelete bool, err error) { if !tmpRec.delete { if tmpRec.distinct && !bytes.Equal(originIdxVal, tmpRec.vals) { - return kv.ErrKeyExists + return false, kv.ErrKeyExists } // The key has been found in the original index, skip merging it. tmpRec.skip = true - return nil + return false, nil } // Delete operation. distinct := tablecodec.IndexKVIsUnique(originIdxVal) if !distinct { // For non-distinct key, it is consist of a null value and the handle. // Same as the non-unique indexes, replay the delete operation on non-distinct keys. - return nil + return false, nil } // For distinct index key values, prevent deleting an unexpected index KV in original index. hdInVal, err := tablecodec.DecodeHandleInUniqueIndexValue(originIdxVal, tblInfo.Meta().IsCommonHandle) if err != nil { - return errors.Trace(err) + return false, errors.Trace(err) } if !tmpRec.handle.Equal(hdInVal) { // The inequality means multiple modifications happened in the same key. @@ -96,16 +106,16 @@ func checkTempIndexKey(txn kv.Transaction, tmpRec *temporaryIndexRecord, originI if kv.IsErrNotFound(err) { // The row is deleted, so we can merge the delete operation to the origin index. tmpRec.skip = false - return nil + return false, nil } // Unexpected errors. - return errors.Trace(err) + return false, errors.Trace(err) } // Don't delete the index key if the row exists. tmpRec.skip = true - return nil + return false, nil } - return nil + return true, nil } // temporaryIndexRecord is the record information of an index. diff --git a/ddl/index_merge_tmp_test.go b/ddl/index_merge_tmp_test.go index 8dd5283d6c3de..a0928c1750b4b 100644 --- a/ddl/index_merge_tmp_test.go +++ b/ddl/index_merge_tmp_test.go @@ -911,3 +911,26 @@ func TestAddIndexUpdateUntouchedValues(t *testing.T) { tk.MustExec("admin check table t;") tk.MustQuery("select * from t;").Check(testkit.Rows("1 1 2", "2 1 2")) } + +func TestAddUniqueIndexFalsePositiveDuplicate(t *testing.T) { + store := testkit.CreateMockStore(t) + + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + tk.MustExec(`create table t(a bigint DEFAULT '-13202', + b varchar(221) NOT NULL DEFAULT 'dup', + unique key exist_idx(b), + PRIMARY KEY (a));`) + tk.MustExec("INSERT INTO t VALUES (1,'1'), (2,'dup');") + + tk1 := testkit.NewTestKit(t, store) + tk1.MustExec("use test") + ddl.MockDMLExecution = func() { + _, err := tk1.Exec("replace into `t` values (3, 'dup');") + assert.NoError(t, err) + } + testfailpoint.Enable(t, "github.com/pingcap/tidb/pkg/ddl/mockDMLExecution", "1*return(true)->return(false)") + + tk.MustExec("alter table t add unique index idx(b);") + tk.MustExec("admin check table t;") +} diff --git a/pkg/ddl/tests/indexmerge/BUILD.bazel b/pkg/ddl/tests/indexmerge/BUILD.bazel new file mode 100644 index 0000000000000..ad5186396e2ca --- /dev/null +++ b/pkg/ddl/tests/indexmerge/BUILD.bazel @@ -0,0 +1,33 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_test") + +go_test( + name = "indexmerge_test", + timeout = "short", + srcs = [ + "main_test.go", + "merge_test.go", + ], + flaky = True, + race = "on", + shard_count = 21, + deps = [ + "//pkg/config", + "//pkg/ddl", + "//pkg/ddl/ingest", + "//pkg/ddl/testutil", + "//pkg/domain", + "//pkg/errno", + "//pkg/kv", + "//pkg/meta/autoid", + "//pkg/meta/model", + "//pkg/tablecodec", + "//pkg/testkit", + "//pkg/testkit/testfailpoint", + "//pkg/testkit/testsetup", + "@com_github_pingcap_failpoint//:failpoint", + "@com_github_stretchr_testify//assert", + "@com_github_stretchr_testify//require", + "@com_github_tikv_client_go_v2//tikv", + "@org_uber_go_goleak//:goleak", + ], +) From 6523e867da0bfa16d7fe74f41b06463b6af698bf Mon Sep 17 00:00:00 2001 From: tangenta Date: Fri, 7 Feb 2025 17:04:42 +0800 Subject: [PATCH 2/2] resolve conflict --- ddl/index_merge_tmp_test.go | 4 +++- pkg/ddl/tests/indexmerge/BUILD.bazel | 33 ---------------------------- pkg/sessionctx/BUILD.bazel | 0 3 files changed, 3 insertions(+), 34 deletions(-) delete mode 100644 pkg/ddl/tests/indexmerge/BUILD.bazel delete mode 100644 pkg/sessionctx/BUILD.bazel diff --git a/ddl/index_merge_tmp_test.go b/ddl/index_merge_tmp_test.go index a0928c1750b4b..382b8636ef6c5 100644 --- a/ddl/index_merge_tmp_test.go +++ b/ddl/index_merge_tmp_test.go @@ -929,8 +929,10 @@ func TestAddUniqueIndexFalsePositiveDuplicate(t *testing.T) { _, err := tk1.Exec("replace into `t` values (3, 'dup');") assert.NoError(t, err) } - testfailpoint.Enable(t, "github.com/pingcap/tidb/pkg/ddl/mockDMLExecution", "1*return(true)->return(false)") + err := failpoint.Enable("github.com/pingcap/tidb/ddl/mockDMLExecution", "1*return(true)->return(false)") + require.NoError(t, err) tk.MustExec("alter table t add unique index idx(b);") tk.MustExec("admin check table t;") + failpoint.Disable("github.com/pingcap/tidb/ddl/mockDMLExecution") } diff --git a/pkg/ddl/tests/indexmerge/BUILD.bazel b/pkg/ddl/tests/indexmerge/BUILD.bazel deleted file mode 100644 index ad5186396e2ca..0000000000000 --- a/pkg/ddl/tests/indexmerge/BUILD.bazel +++ /dev/null @@ -1,33 +0,0 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_test") - -go_test( - name = "indexmerge_test", - timeout = "short", - srcs = [ - "main_test.go", - "merge_test.go", - ], - flaky = True, - race = "on", - shard_count = 21, - deps = [ - "//pkg/config", - "//pkg/ddl", - "//pkg/ddl/ingest", - "//pkg/ddl/testutil", - "//pkg/domain", - "//pkg/errno", - "//pkg/kv", - "//pkg/meta/autoid", - "//pkg/meta/model", - "//pkg/tablecodec", - "//pkg/testkit", - "//pkg/testkit/testfailpoint", - "//pkg/testkit/testsetup", - "@com_github_pingcap_failpoint//:failpoint", - "@com_github_stretchr_testify//assert", - "@com_github_stretchr_testify//require", - "@com_github_tikv_client_go_v2//tikv", - "@org_uber_go_goleak//:goleak", - ], -) diff --git a/pkg/sessionctx/BUILD.bazel b/pkg/sessionctx/BUILD.bazel deleted file mode 100644 index e69de29bb2d1d..0000000000000