From d9ca5ce9a287fb9d093bfe202d7d106665106531 Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Fri, 11 Oct 2024 18:59:05 +0800 Subject: [PATCH 1/3] This is an automated cherry-pick of #56514 Signed-off-by: ti-chi-bot --- pkg/executor/insert.go | 54 ++++++++++++++++++++++++++++++ pkg/executor/insert_test.go | 66 +++++++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/pkg/executor/insert.go b/pkg/executor/insert.go index c10c7810f01e4..e02885e40a2dc 100644 --- a/pkg/executor/insert.go +++ b/pkg/executor/insert.go @@ -186,7 +186,11 @@ func (e *InsertValues) prefetchDataCache(ctx context.Context, txn kv.Transaction } // updateDupRow updates a duplicate row to a new row. +<<<<<<< HEAD func (e *InsertExec) updateDupRow(ctx context.Context, idxInBatch int, txn kv.Transaction, row toBeCheckedRow, handle kv.Handle, _ []*expression.Assignment) error { +======= +func (e *InsertExec) updateDupRow(ctx context.Context, idxInBatch int, txn kv.Transaction, row toBeCheckedRow, handle kv.Handle, _ []*expression.Assignment, dupKeyCheck table.DupKeyCheckMode, autoColIdx int) error { +>>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) oldRow, err := getOldRow(ctx, e.Ctx(), txn, row.t, handle, e.GenExprs) if err != nil { return err @@ -197,11 +201,18 @@ func (e *InsertExec) updateDupRow(ctx context.Context, idxInBatch int, txn kv.Tr extraCols = e.Ctx().GetSessionVars().CurrInsertBatchExtraCols[idxInBatch] } +<<<<<<< HEAD err = e.doDupRowUpdate(ctx, handle, oldRow, row.row, extraCols, e.OnDuplicate, idxInBatch) if e.Ctx().GetSessionVars().StmtCtx.DupKeyAsWarning && (kv.ErrKeyExists.Equal(err) || table.ErrCheckConstraintViolated.Equal(err)) { e.Ctx().GetSessionVars().StmtCtx.AppendWarning(err) return nil +======= + err = e.doDupRowUpdate(ctx, handle, oldRow, row.row, extraCols, e.OnDuplicate, idxInBatch, dupKeyCheck, autoColIdx) + if kv.ErrKeyExists.Equal(err) || table.ErrCheckConstraintViolated.Equal(err) { + ec := e.Ctx().GetSessionVars().StmtCtx.ErrCtx() + return ec.HandleErrorWithAlias(kv.ErrKeyExists, err, err) +>>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) } return err } @@ -236,6 +247,26 @@ func (e *InsertExec) batchUpdateDupRows(ctx context.Context, newRows [][]types.D e.stats.Prefetch += time.Since(prefetchStart) } +<<<<<<< HEAD +======= + // Use `optimizeDupKeyCheckForUpdate` to determine the update operation when the row meets the conflict in + // `INSERT ... ON DUPLICATE KEY UPDATE` statement. + // Though it is in an insert statement, `ON DUP KEY UPDATE` follows the dup-key check behavior of update. + // For example, it will ignore variable `tidb_constraint_check_in_place`, see the test case: + // https://github.com/pingcap/tidb/blob/3117d3fae50bbb5dabcde7b9589f92bfbbda5dc6/pkg/executor/test/writetest/write_test.go#L419-L426 + updateDupKeyCheck := optimizeDupKeyCheckForUpdate(txn, e.IgnoreErr) + // Do not use `updateDupKeyCheck` for `AddRecord` because it is not optimized for insert. + // It seems that we can just use `DupKeyCheckSkip` here because all constraints are checked. + // But we still use `optimizeDupKeyCheckForNormalInsert` to make the refactor same behavior with the original code. + // TODO: just use `DupKeyCheckSkip` here. + addRecordDupKeyCheck := optimizeDupKeyCheckForNormalInsert(e.Ctx().GetSessionVars(), txn) + + _, autoColIdx, found := findAutoIncrementColumn(e.Table) + if !found { + autoColIdx = -1 + } + +>>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) for i, r := range toBeCheckedRows { if r.handleKey != nil { handle, err := tablecodec.DecodeRowKey(r.handleKey.newKey) @@ -243,7 +274,11 @@ func (e *InsertExec) batchUpdateDupRows(ctx context.Context, newRows [][]types.D return err } +<<<<<<< HEAD err = e.updateDupRow(ctx, i, txn, r, handle, e.OnDuplicate) +======= + err = e.updateDupRow(ctx, i, txn, r, handle, e.OnDuplicate, updateDupKeyCheck, autoColIdx) +>>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) if err == nil { continue } @@ -260,7 +295,11 @@ func (e *InsertExec) batchUpdateDupRows(ctx context.Context, newRows [][]types.D if handle == nil { continue } +<<<<<<< HEAD err = e.updateDupRow(ctx, i, txn, r, handle, e.OnDuplicate) +======= + err = e.updateDupRow(ctx, i, txn, r, handle, e.OnDuplicate, updateDupKeyCheck, autoColIdx) +>>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) if err != nil { if kv.IsErrNotFound(err) { // Data index inconsistent? A unique key provide the handle information, but the @@ -381,7 +420,11 @@ func (e *InsertExec) initEvalBuffer4Dup() { // doDupRowUpdate updates the duplicate row. func (e *InsertExec) doDupRowUpdate(ctx context.Context, handle kv.Handle, oldRow []types.Datum, newRow []types.Datum, +<<<<<<< HEAD extraCols []types.Datum, cols []*expression.Assignment, idxInBatch int) error { +======= + extraCols []types.Datum, cols []*expression.Assignment, idxInBatch int, dupKeyMode table.DupKeyCheckMode, autoColIdx int) error { +>>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) assignFlag := make([]bool, len(e.Table.WritableCols())) // See http://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_values e.curInsertVals.SetDatums(newRow...) @@ -430,6 +473,17 @@ func (e *InsertExec) doDupRowUpdate(ctx context.Context, handle kv.Handle, oldRo if err != nil { return err } + + if autoColIdx >= 0 { + if e.Ctx().GetSessionVars().StmtCtx.AffectedRows() > 0 { + // If "INSERT ... ON DUPLICATE KEY UPDATE" duplicate and update a row, + // auto increment value should be set correctly for mysql_insert_id() + // See https://github.com/pingcap/tidb/issues/55965 + e.Ctx().GetSessionVars().StmtCtx.InsertID = newData[autoColIdx].GetUint64() + } else { + e.Ctx().GetSessionVars().StmtCtx.InsertID = 0 + } + } return nil } diff --git a/pkg/executor/insert_test.go b/pkg/executor/insert_test.go index 6cfa7fb258ad8..fd0bbb8149c82 100644 --- a/pkg/executor/insert_test.go +++ b/pkg/executor/insert_test.go @@ -1594,6 +1594,7 @@ func TestInsertLockUnchangedKeys(t *testing.T) { } } +<<<<<<< HEAD // see issue https://github.com/pingcap/tidb/issues/47787 func TestInsertBigScientificNotation(t *testing.T) { store := testkit.CreateMockStore(t) @@ -1627,4 +1628,69 @@ func TestUnsignedDecimalFloatInsertNegative(t *testing.T) { tk.MustExec("set @@sql_mode=''") tk.MustExec("insert into tf values('-100')") tk.MustQuery("select * from tf").Check(testkit.Rows("0")) +======= +func TestMySQLInsertID(t *testing.T) { + // mysql_insert_id() differs from LAST_INSERT_ID() + // See https://github.com/pingcap/tidb/issues/55965 + // mysql_insert_id() is got from tk.Session().LastInsertID() + store := testkit.CreateMockStore(t) + tk := testkit.NewTestKit(t, store) + tk.MustExec(`use test`) + tk.MustExec("drop table if exists tb") + tk.MustExec("create table tb(pk int primary key auto_increment, a int, b int, unique(a))") + defer tk.MustExec("drop table if exists tb") + + tk.MustExec("insert into tb (a, b) values (1, 1) on duplicate key update b = values(b)") + require.Equal(t, tk.Session().LastInsertID(), uint64(1)) + + tk.MustExec("insert into tb (a, b) values (2, 2) on duplicate key update b = values(b)") + require.Equal(t, tk.Session().LastInsertID(), uint64(2)) + + // If there is an AUTO_INCREMENT column in the table and there were some explicit successfully + // inserted values or some updated values, return the last of the inserted or updated values. + // Ref https://dev.mysql.com/doc/c-api/5.7/en/mysql-insert-id.html#:~:text=When%20called%20after%20an%20INSERT%20...%20ON,of%20the%20inserted%20or%20updated%20values + tk.MustExec("insert into tb (a, b) values (1, 2) on duplicate key update b = values(b)") + require.Equal(t, tk.Session().LastInsertID(), uint64(1)) + tk.MustQuery("select LAST_INSERT_ID()").Check(testkit.Rows("2")) + + tk.MustQuery("select * from tb").Sort().Check(testkit.Rows("1 1 2", "2 2 2")) + + // When the new row and the old row are exactly the same (no inserted or updated values), mysql_insert_id() is 0 + tk.MustExec("insert into tb (a, b) values (1, 2) on duplicate key update b = 2") + require.Equal(t, tk.Session().LastInsertID(), uint64(0)) + tk.MustQuery("select LAST_INSERT_ID()").Check(testkit.Rows("2")) + + // When the value of auto increment column is assigned explicitly, LAST_INSERT_ID() is unchanged. + // mysql_insert_id() is set to the explicit assigned value. + tk.MustExec("insert into tb values (6, 6, 6)") + require.Equal(t, tk.Session().LastInsertID(), uint64(6)) + tk.MustQuery("select LAST_INSERT_ID()").Check(testkit.Rows("2")) + + // Update statement touches neigher mysql_insert_id() nor LAST_INSERT_ID() + tk.MustExec("update tb set b = 7, pk = pk + 1 where b = 6") + require.Equal(t, tk.Session().LastInsertID(), uint64(0)) + tk.MustQuery("select LAST_INSERT_ID()").Check(testkit.Rows("2")) + + // How to distinguish LAST_INSERT_ID() and mysql_insert_id()? + // In a word, LAST_INSERT_ID() is always get from auto allocated value, while mysql_insert_id() can be + // auto allocated or explicited specified. + + // Another scenario mentioned by @lcwangcao + // What's the behaviour when transaction conflict involved? + tk.MustExec("truncate table tb") + tk.MustExec("insert into tb (a, b) values (1, 1), (2, 2)") + + tk1 := testkit.NewTestKit(t, store) + tk1.MustExec("use test") + tk1.MustExec("begin") + tk1.MustExec("update tb set b = 2 where a = 1") + go func() { + time.Sleep(100 * time.Millisecond) + tk1.MustExec("commit") + }() + // The first time this will update one row. + // Then transaction conflict and retry, in the second time it modify nothing. + tk.MustExec("insert into tb(a, b) values(1,2) on duplicate key update b = 2;") + require.Equal(t, tk.Session().LastInsertID(), uint64(0)) +>>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) } From ba64db130771ed2f0e22f71a1df10daa3e80058e Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Tue, 10 Dec 2024 18:20:06 +0800 Subject: [PATCH 2/3] resolve conflict --- pkg/executor/insert.go | 48 +++++------------------------------------- 1 file changed, 5 insertions(+), 43 deletions(-) diff --git a/pkg/executor/insert.go b/pkg/executor/insert.go index e02885e40a2dc..22dd28c673c1e 100644 --- a/pkg/executor/insert.go +++ b/pkg/executor/insert.go @@ -186,11 +186,7 @@ func (e *InsertValues) prefetchDataCache(ctx context.Context, txn kv.Transaction } // updateDupRow updates a duplicate row to a new row. -<<<<<<< HEAD -func (e *InsertExec) updateDupRow(ctx context.Context, idxInBatch int, txn kv.Transaction, row toBeCheckedRow, handle kv.Handle, _ []*expression.Assignment) error { -======= -func (e *InsertExec) updateDupRow(ctx context.Context, idxInBatch int, txn kv.Transaction, row toBeCheckedRow, handle kv.Handle, _ []*expression.Assignment, dupKeyCheck table.DupKeyCheckMode, autoColIdx int) error { ->>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) +func (e *InsertExec) updateDupRow(ctx context.Context, idxInBatch int, txn kv.Transaction, row toBeCheckedRow, handle kv.Handle, _ []*expression.Assignment, autoColIdx int) error { oldRow, err := getOldRow(ctx, e.Ctx(), txn, row.t, handle, e.GenExprs) if err != nil { return err @@ -201,18 +197,11 @@ func (e *InsertExec) updateDupRow(ctx context.Context, idxInBatch int, txn kv.Tr extraCols = e.Ctx().GetSessionVars().CurrInsertBatchExtraCols[idxInBatch] } -<<<<<<< HEAD - err = e.doDupRowUpdate(ctx, handle, oldRow, row.row, extraCols, e.OnDuplicate, idxInBatch) + err = e.doDupRowUpdate(ctx, handle, oldRow, row.row, extraCols, e.OnDuplicate, idxInBatch, autoColIdx) if e.Ctx().GetSessionVars().StmtCtx.DupKeyAsWarning && (kv.ErrKeyExists.Equal(err) || table.ErrCheckConstraintViolated.Equal(err)) { e.Ctx().GetSessionVars().StmtCtx.AppendWarning(err) return nil -======= - err = e.doDupRowUpdate(ctx, handle, oldRow, row.row, extraCols, e.OnDuplicate, idxInBatch, dupKeyCheck, autoColIdx) - if kv.ErrKeyExists.Equal(err) || table.ErrCheckConstraintViolated.Equal(err) { - ec := e.Ctx().GetSessionVars().StmtCtx.ErrCtx() - return ec.HandleErrorWithAlias(kv.ErrKeyExists, err, err) ->>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) } return err } @@ -247,26 +236,11 @@ func (e *InsertExec) batchUpdateDupRows(ctx context.Context, newRows [][]types.D e.stats.Prefetch += time.Since(prefetchStart) } -<<<<<<< HEAD -======= - // Use `optimizeDupKeyCheckForUpdate` to determine the update operation when the row meets the conflict in - // `INSERT ... ON DUPLICATE KEY UPDATE` statement. - // Though it is in an insert statement, `ON DUP KEY UPDATE` follows the dup-key check behavior of update. - // For example, it will ignore variable `tidb_constraint_check_in_place`, see the test case: - // https://github.com/pingcap/tidb/blob/3117d3fae50bbb5dabcde7b9589f92bfbbda5dc6/pkg/executor/test/writetest/write_test.go#L419-L426 - updateDupKeyCheck := optimizeDupKeyCheckForUpdate(txn, e.IgnoreErr) - // Do not use `updateDupKeyCheck` for `AddRecord` because it is not optimized for insert. - // It seems that we can just use `DupKeyCheckSkip` here because all constraints are checked. - // But we still use `optimizeDupKeyCheckForNormalInsert` to make the refactor same behavior with the original code. - // TODO: just use `DupKeyCheckSkip` here. - addRecordDupKeyCheck := optimizeDupKeyCheckForNormalInsert(e.Ctx().GetSessionVars(), txn) - _, autoColIdx, found := findAutoIncrementColumn(e.Table) if !found { autoColIdx = -1 } ->>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) for i, r := range toBeCheckedRows { if r.handleKey != nil { handle, err := tablecodec.DecodeRowKey(r.handleKey.newKey) @@ -274,11 +248,7 @@ func (e *InsertExec) batchUpdateDupRows(ctx context.Context, newRows [][]types.D return err } -<<<<<<< HEAD - err = e.updateDupRow(ctx, i, txn, r, handle, e.OnDuplicate) -======= - err = e.updateDupRow(ctx, i, txn, r, handle, e.OnDuplicate, updateDupKeyCheck, autoColIdx) ->>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) + err = e.updateDupRow(ctx, i, txn, r, handle, e.OnDuplicate, autoColIdx) if err == nil { continue } @@ -295,11 +265,7 @@ func (e *InsertExec) batchUpdateDupRows(ctx context.Context, newRows [][]types.D if handle == nil { continue } -<<<<<<< HEAD - err = e.updateDupRow(ctx, i, txn, r, handle, e.OnDuplicate) -======= - err = e.updateDupRow(ctx, i, txn, r, handle, e.OnDuplicate, updateDupKeyCheck, autoColIdx) ->>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) + err = e.updateDupRow(ctx, i, txn, r, handle, e.OnDuplicate, autoColIdx) if err != nil { if kv.IsErrNotFound(err) { // Data index inconsistent? A unique key provide the handle information, but the @@ -420,11 +386,7 @@ func (e *InsertExec) initEvalBuffer4Dup() { // doDupRowUpdate updates the duplicate row. func (e *InsertExec) doDupRowUpdate(ctx context.Context, handle kv.Handle, oldRow []types.Datum, newRow []types.Datum, -<<<<<<< HEAD - extraCols []types.Datum, cols []*expression.Assignment, idxInBatch int) error { -======= - extraCols []types.Datum, cols []*expression.Assignment, idxInBatch int, dupKeyMode table.DupKeyCheckMode, autoColIdx int) error { ->>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) + extraCols []types.Datum, cols []*expression.Assignment, idxInBatch int, autoColIdx int) error { assignFlag := make([]bool, len(e.Table.WritableCols())) // See http://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_values e.curInsertVals.SetDatums(newRow...) From 82273aa6e0fc29a338aeb637ab77bc5014c5c7ad Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Tue, 10 Dec 2024 21:23:47 +0800 Subject: [PATCH 3/3] resolve conflict --- pkg/executor/insert_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/executor/insert_test.go b/pkg/executor/insert_test.go index fd0bbb8149c82..4f88cc09cb4f7 100644 --- a/pkg/executor/insert_test.go +++ b/pkg/executor/insert_test.go @@ -1594,7 +1594,6 @@ func TestInsertLockUnchangedKeys(t *testing.T) { } } -<<<<<<< HEAD // see issue https://github.com/pingcap/tidb/issues/47787 func TestInsertBigScientificNotation(t *testing.T) { store := testkit.CreateMockStore(t) @@ -1628,7 +1627,8 @@ func TestUnsignedDecimalFloatInsertNegative(t *testing.T) { tk.MustExec("set @@sql_mode=''") tk.MustExec("insert into tf values('-100')") tk.MustQuery("select * from tf").Check(testkit.Rows("0")) -======= +} + func TestMySQLInsertID(t *testing.T) { // mysql_insert_id() differs from LAST_INSERT_ID() // See https://github.com/pingcap/tidb/issues/55965 @@ -1692,5 +1692,4 @@ func TestMySQLInsertID(t *testing.T) { // Then transaction conflict and retry, in the second time it modify nothing. tk.MustExec("insert into tb(a, b) values(1,2) on duplicate key update b = 2;") require.Equal(t, tk.Session().LastInsertID(), uint64(0)) ->>>>>>> 3dfc47f2cc2 (executor: fix mysql_insert_id() for "INSERT .. ON DUPLICATE KEY" statement (#56514)) }