From 765559136868db34ad1070e6e01a5e6dd5a1ef78 Mon Sep 17 00:00:00 2001 From: tangenta Date: Thu, 19 Sep 2024 18:06:44 +0800 Subject: [PATCH 1/3] ddl: fix unexpected duplicate entry error when adding unique index --- pkg/ddl/index_merge_tmp.go | 35 +++++++++++++++++--------- pkg/ddl/tests/indexmerge/merge_test.go | 23 +++++++++++++++++ 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/pkg/ddl/index_merge_tmp.go b/pkg/ddl/index_merge_tmp.go index 89ca8159ace3d..6be104015e3db 100644 --- a/pkg/ddl/index_merge_tmp.go +++ b/pkg/ddl/index_merge_tmp.go @@ -49,44 +49,55 @@ func (w *mergeIndexWorker) batchCheckTemporaryUniqueKey( } 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 { if kv.ErrKeyExists.Equal(err) { return driver.ExtractKeyExistsErrFromIndex(key, val, w.table.Meta(), w.currentIndex.ID) } 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.DecodeHandleInIndexValue(originIdxVal) 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. @@ -97,16 +108,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/pkg/ddl/tests/indexmerge/merge_test.go b/pkg/ddl/tests/indexmerge/merge_test.go index d8f16032df284..6554585a51754 100644 --- a/pkg/ddl/tests/indexmerge/merge_test.go +++ b/pkg/ddl/tests/indexmerge/merge_test.go @@ -831,3 +831,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;") +} From ae4d347df02b368c4a91465e5d02a6481aa98dd9 Mon Sep 17 00:00:00 2001 From: tangenta Date: Thu, 19 Sep 2024 18:13:44 +0800 Subject: [PATCH 2/3] fix linter --- pkg/ddl/index_merge_tmp.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/ddl/index_merge_tmp.go b/pkg/ddl/index_merge_tmp.go index 6be104015e3db..801e137d22ab4 100644 --- a/pkg/ddl/index_merge_tmp.go +++ b/pkg/ddl/index_merge_tmp.go @@ -68,7 +68,6 @@ func (w *mergeIndexWorker) batchCheckTemporaryUniqueKey( // so we need to backfill the not found key into `batchVals` map. batchVals[keyStr] = idxRecords[i].vals } - } return nil } From 1f6bafbd02c78af56d536c60544c4ec9abc75bce Mon Sep 17 00:00:00 2001 From: tangenta Date: Thu, 19 Sep 2024 18:44:15 +0800 Subject: [PATCH 3/3] update bazel --- pkg/ddl/tests/indexmerge/BUILD.bazel | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/ddl/tests/indexmerge/BUILD.bazel b/pkg/ddl/tests/indexmerge/BUILD.bazel index 71f1344442fdf..ad5186396e2ca 100644 --- a/pkg/ddl/tests/indexmerge/BUILD.bazel +++ b/pkg/ddl/tests/indexmerge/BUILD.bazel @@ -9,7 +9,7 @@ go_test( ], flaky = True, race = "on", - shard_count = 20, + shard_count = 21, deps = [ "//pkg/config", "//pkg/ddl",