From cde3a7ba7757c7909b77fe3230e6a8e9fcd9cf6e Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Tue, 16 Apr 2024 15:07:37 +0800 Subject: [PATCH 1/5] executor: handle the corner case that temp index is not exist but the normal index is exist (#51862) (#52499) close pingcap/tidb#51784 --- ddl/ddl_test.go | 41 +++++++++++++++++++++ executor/insert_common.go | 76 +++++++++++++++++++++++++-------------- 2 files changed, 90 insertions(+), 27 deletions(-) diff --git a/ddl/ddl_test.go b/ddl/ddl_test.go index 9760608674c6f..e895875dcc0f2 100644 --- a/ddl/ddl_test.go +++ b/ddl/ddl_test.go @@ -16,6 +16,8 @@ package ddl import ( "context" + "github.com/pingcap/tidb/testkit" + "github.com/stretchr/testify/assert" "testing" "time" @@ -383,3 +385,42 @@ func TestError(t *testing.T) { require.Equal(t, uint16(err.Code()), code) } } + +func TestInsertIgnore(t *testing.T) { + store, dom := testkit.CreateMockStoreAndDomain(t) + + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + tk.MustExec("create table t(a smallint(6) DEFAULT '-13202', b varchar(221) NOT NULL DEFAULT 'duplicatevalue', " + + "c tinyint(1) NOT NULL DEFAULT '0', PRIMARY KEY (c, b));") + + tk1 := testkit.NewTestKit(t, store) + tk1.MustExec("use test") + + d := dom.DDL() + originalCallback := d.GetHook() + defer d.SetHook(originalCallback) + callback := &TestDDLCallback{} + + onJobUpdatedExportedFunc := func(job *model.Job) { + switch job.SchemaState { + case model.StateDeleteOnly: + _, err := tk1.Exec("INSERT INTO t VALUES (-18585,'aaa',1), (-18585,'0',1), (-18585,'1',1), (-18585,'duplicatevalue',1);") + assert.NoError(t, err) + case model.StateWriteReorganization: + tbl, err := dom.InfoSchema().TableByName(model.NewCIStr("test"), model.NewCIStr("t")) + assert.NoError(t, err) + idx := tbl.Meta().FindIndexByName("idx") + if idx.BackfillState == model.BackfillStateReadyToMerge { + _, err := tk1.Exec("insert ignore into `t` values ( 234,'duplicatevalue',-2028 );") + assert.NoError(t, err) + return + } + } + } + callback.OnJobUpdatedExported.Store(&onJobUpdatedExportedFunc) + d.SetHook(callback) + + tk.MustExec("alter table t add unique index idx(b);") + tk.MustExec("admin check table t;") +} diff --git a/executor/insert_common.go b/executor/insert_common.go index b3683169ce32f..53b729641adbc 100644 --- a/executor/insert_common.go +++ b/executor/insert_common.go @@ -1097,6 +1097,28 @@ func (e *InsertValues) collectRuntimeStatsEnabled() bool { return false } +func (e *InsertValues) handleDuplicateKey(ctx context.Context, txn kv.Transaction, uk *keyValueWithDupInfo, replace bool, r toBeCheckedRow) (bool, error) { + if !replace { + e.ctx.GetSessionVars().StmtCtx.AppendWarning(uk.dupErr) + if txnCtx := e.ctx.GetSessionVars().TxnCtx; txnCtx.IsPessimistic && e.ctx.GetSessionVars().LockUnchangedKeys { + txnCtx.AddUnchangedKeyForLock(uk.newKey) + } + return true, nil + } + _, handle, err := tables.FetchDuplicatedHandle(ctx, uk.newKey, true, txn, e.Table.Meta().ID, uk.commonHandle) + if err != nil { + return false, err + } + if handle == nil { + return false, nil + } + _, err = e.removeRow(ctx, txn, handle, r, true) + if err != nil { + return false, err + } + return false, nil +} + // batchCheckAndInsert checks rows with duplicate errors. // All duplicate rows will be ignored and appended as duplicate warnings. func (e *InsertValues) batchCheckAndInsert(ctx context.Context, rows [][]types.Datum, @@ -1177,44 +1199,44 @@ func (e *InsertValues) batchCheckAndInsert(ctx context.Context, rows [][]types.D return err } } - skip := false + rowInserted := false for _, uk := range r.uniqueKeys { _, err := txn.Get(ctx, uk.newKey) + if err != nil && !kv.IsErrNotFound(err) { + return err + } if err == nil { - if replace { - _, handle, err := tables.FetchDuplicatedHandle( - ctx, - uk.newKey, - true, - txn, - e.Table.Meta().ID, - uk.commonHandle, - ) - if err != nil { - return err - } - if handle == nil { - continue - } - _, err = e.removeRow(ctx, txn, handle, r, true) + rowInserted, err = e.handleDuplicateKey(ctx, txn, uk, replace, r) + if err != nil { + return err + } + if rowInserted { + break + } + continue + } + if tablecodec.IsTempIndexKey(uk.newKey) { + tablecodec.TempIndexKey2IndexKey(uk.newKey) + _, err = txn.Get(ctx, uk.newKey) + if err != nil && !kv.IsErrNotFound(err) { + return err + } + if err == nil { + rowInserted, err = e.handleDuplicateKey(ctx, txn, uk, replace, r) if err != nil { return err } - } else { - // If duplicate keys were found in BatchGet, mark row = nil. - e.ctx.GetSessionVars().StmtCtx.AppendWarning(uk.dupErr) - if txnCtx := e.ctx.GetSessionVars().TxnCtx; txnCtx.IsPessimistic { - // lock duplicated unique key on insert-ignore - txnCtx.AddUnchangedRowKey(uk.newKey) + if rowInserted { + break } - skip = true - break } - } else if !kv.IsErrNotFound(err) { - return err } } + if rowInserted { + continue + } + // If row was checked with no duplicate keys, // it should be add to values map for the further row check. // There may be duplicate keys inside the insert statement. From 7d2b388233d3e9fd7f47abd6c30673de300ad52d Mon Sep 17 00:00:00 2001 From: wjhuang2016 Date: Sat, 14 Sep 2024 16:26:18 +0800 Subject: [PATCH 2/5] fix Signed-off-by: wjhuang2016 --- ddl/ddl_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ddl/ddl_test.go b/ddl/ddl_test.go index e895875dcc0f2..50b195c06d31c 100644 --- a/ddl/ddl_test.go +++ b/ddl/ddl_test.go @@ -16,8 +16,6 @@ package ddl import ( "context" - "github.com/pingcap/tidb/testkit" - "github.com/stretchr/testify/assert" "testing" "time" @@ -34,9 +32,11 @@ import ( "github.com/pingcap/tidb/sessionctx" "github.com/pingcap/tidb/store/mockstore" "github.com/pingcap/tidb/table" + "github.com/pingcap/tidb/testkit" "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/util/dbterror" "github.com/pingcap/tidb/util/mock" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) From 08f2ad4b90b9ecba91d185da8ae296d57078f5c7 Mon Sep 17 00:00:00 2001 From: wjhuang2016 Date: Sat, 14 Sep 2024 16:49:46 +0800 Subject: [PATCH 3/5] fix Signed-off-by: wjhuang2016 --- executor/insert_common.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/executor/insert_common.go b/executor/insert_common.go index 53b729641adbc..70a2c02944b7f 100644 --- a/executor/insert_common.go +++ b/executor/insert_common.go @@ -1100,8 +1100,9 @@ func (e *InsertValues) collectRuntimeStatsEnabled() bool { func (e *InsertValues) handleDuplicateKey(ctx context.Context, txn kv.Transaction, uk *keyValueWithDupInfo, replace bool, r toBeCheckedRow) (bool, error) { if !replace { e.ctx.GetSessionVars().StmtCtx.AppendWarning(uk.dupErr) - if txnCtx := e.ctx.GetSessionVars().TxnCtx; txnCtx.IsPessimistic && e.ctx.GetSessionVars().LockUnchangedKeys { - txnCtx.AddUnchangedKeyForLock(uk.newKey) + if txnCtx := e.ctx.GetSessionVars().TxnCtx; txnCtx.IsPessimistic { + // lock duplicated row key on insert-ignore + txnCtx.AddUnchangedRowKey(r.handleKey.newKey) } return true, nil } @@ -1240,12 +1241,10 @@ func (e *InsertValues) batchCheckAndInsert(ctx context.Context, rows [][]types.D // If row was checked with no duplicate keys, // it should be add to values map for the further row check. // There may be duplicate keys inside the insert statement. - if !skip { - e.ctx.GetSessionVars().StmtCtx.AddCopiedRows(1) - err = addRecord(ctx, rows[i]) - if err != nil { - return err - } + e.ctx.GetSessionVars().StmtCtx.AddCopiedRows(1) + err = addRecord(ctx, rows[i]) + if err != nil { + return err } } if e.stats != nil { From d2421f094f002d1e9f7f198675b6e6d4f20c2d16 Mon Sep 17 00:00:00 2001 From: wjhuang2016 Date: Sat, 14 Sep 2024 16:58:09 +0800 Subject: [PATCH 4/5] fix Signed-off-by: wjhuang2016 --- ddl/db_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ ddl/ddl_test.go | 41 ----------------------------------------- 2 files changed, 40 insertions(+), 41 deletions(-) diff --git a/ddl/db_test.go b/ddl/db_test.go index d52c1983fd6a5..7e75c7509baf1 100644 --- a/ddl/db_test.go +++ b/ddl/db_test.go @@ -52,6 +52,7 @@ import ( "github.com/pingcap/tidb/util/dbterror" "github.com/pingcap/tidb/util/mock" "github.com/pingcap/tidb/util/sqlexec" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/tikv/client-go/v2/oracle" "github.com/tikv/client-go/v2/tikv" @@ -1817,3 +1818,42 @@ func TestMDLTruncateTable(t *testing.T) { require.True(t, timetk2.After(timeMain)) require.True(t, timetk3.After(timeMain)) } + +func TestInsertIgnore(t *testing.T) { + store, dom := testkit.CreateMockStoreAndDomain(t) + + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + tk.MustExec("create table t(a smallint(6) DEFAULT '-13202', b varchar(221) NOT NULL DEFAULT 'duplicatevalue', " + + "c tinyint(1) NOT NULL DEFAULT '0', PRIMARY KEY (c, b));") + + tk1 := testkit.NewTestKit(t, store) + tk1.MustExec("use test") + + d := dom.DDL() + originalCallback := d.GetHook() + defer d.SetHook(originalCallback) + callback := &ddl.TestDDLCallback{} + + onJobUpdatedExportedFunc := func(job *model.Job) { + switch job.SchemaState { + case model.StateDeleteOnly: + _, err := tk1.Exec("INSERT INTO t VALUES (-18585,'aaa',1), (-18585,'0',1), (-18585,'1',1), (-18585,'duplicatevalue',1);") + assert.NoError(t, err) + case model.StateWriteReorganization: + tbl, err := dom.InfoSchema().TableByName(model.NewCIStr("test"), model.NewCIStr("t")) + assert.NoError(t, err) + idx := tbl.Meta().FindIndexByName("idx") + if idx.BackfillState == model.BackfillStateReadyToMerge { + _, err := tk1.Exec("insert ignore into `t` values ( 234,'duplicatevalue',-2028 );") + assert.NoError(t, err) + return + } + } + } + callback.OnJobUpdatedExported.Store(&onJobUpdatedExportedFunc) + d.SetHook(callback) + + tk.MustExec("alter table t add unique index idx(b);") + tk.MustExec("admin check table t;") +} diff --git a/ddl/ddl_test.go b/ddl/ddl_test.go index 50b195c06d31c..9760608674c6f 100644 --- a/ddl/ddl_test.go +++ b/ddl/ddl_test.go @@ -32,11 +32,9 @@ import ( "github.com/pingcap/tidb/sessionctx" "github.com/pingcap/tidb/store/mockstore" "github.com/pingcap/tidb/table" - "github.com/pingcap/tidb/testkit" "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/util/dbterror" "github.com/pingcap/tidb/util/mock" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -385,42 +383,3 @@ func TestError(t *testing.T) { require.Equal(t, uint16(err.Code()), code) } } - -func TestInsertIgnore(t *testing.T) { - store, dom := testkit.CreateMockStoreAndDomain(t) - - tk := testkit.NewTestKit(t, store) - tk.MustExec("use test") - tk.MustExec("create table t(a smallint(6) DEFAULT '-13202', b varchar(221) NOT NULL DEFAULT 'duplicatevalue', " + - "c tinyint(1) NOT NULL DEFAULT '0', PRIMARY KEY (c, b));") - - tk1 := testkit.NewTestKit(t, store) - tk1.MustExec("use test") - - d := dom.DDL() - originalCallback := d.GetHook() - defer d.SetHook(originalCallback) - callback := &TestDDLCallback{} - - onJobUpdatedExportedFunc := func(job *model.Job) { - switch job.SchemaState { - case model.StateDeleteOnly: - _, err := tk1.Exec("INSERT INTO t VALUES (-18585,'aaa',1), (-18585,'0',1), (-18585,'1',1), (-18585,'duplicatevalue',1);") - assert.NoError(t, err) - case model.StateWriteReorganization: - tbl, err := dom.InfoSchema().TableByName(model.NewCIStr("test"), model.NewCIStr("t")) - assert.NoError(t, err) - idx := tbl.Meta().FindIndexByName("idx") - if idx.BackfillState == model.BackfillStateReadyToMerge { - _, err := tk1.Exec("insert ignore into `t` values ( 234,'duplicatevalue',-2028 );") - assert.NoError(t, err) - return - } - } - } - callback.OnJobUpdatedExported.Store(&onJobUpdatedExportedFunc) - d.SetHook(callback) - - tk.MustExec("alter table t add unique index idx(b);") - tk.MustExec("admin check table t;") -} From 2140a5124362c44ee51722566c9a21edc5c88ae6 Mon Sep 17 00:00:00 2001 From: wjhuang2016 Date: Sat, 14 Sep 2024 17:32:54 +0800 Subject: [PATCH 5/5] fix Signed-off-by: wjhuang2016 --- executor/insert_common.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/executor/insert_common.go b/executor/insert_common.go index 70a2c02944b7f..18501893e84e4 100644 --- a/executor/insert_common.go +++ b/executor/insert_common.go @@ -1102,7 +1102,7 @@ func (e *InsertValues) handleDuplicateKey(ctx context.Context, txn kv.Transactio e.ctx.GetSessionVars().StmtCtx.AppendWarning(uk.dupErr) if txnCtx := e.ctx.GetSessionVars().TxnCtx; txnCtx.IsPessimistic { // lock duplicated row key on insert-ignore - txnCtx.AddUnchangedRowKey(r.handleKey.newKey) + txnCtx.AddUnchangedRowKey(uk.newKey) } return true, nil }