executor: lock duplicated keys on insert-ignore & replace-nothing #42210
Conversation
Signed-off-by: zyguan <[email protected]>
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
do-not-merge/needs-triage-completed
release-note-none
| @@ -1239,6 +1243,10 @@ func (e *InsertValues) batchCheckAndInsert(ctx context.Context, rows [][]types.D | |||
| } 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-ingore | |||
| txnCtx.AddUnchangedRowKey(uk.newKey) | |||
There was a problem hiding this comment.
It seems mysql only locks unique keys as well.
/* init */ select version();
-- init >> +-----------+
-- init | version() |
-- init +-----------+
-- init | 8.0.28 |
-- init +-----------+
/* init */ drop table if exists t;
-- init >> 0 rows affected
/* init */ create table t (a int primary key, b int unique key, c int);
-- init >> 0 rows affected
/* init */ insert into t values (1, 1, 1);
-- init >> 1 rows affected
/* t1 */ begin;
-- t1 >> 0 rows affected
/* t2 */ begin;
-- t2 >> 0 rows affected
/* t1 */ insert ignore into t values (2, 1, 1);
-- t1 >> 0 rows affected
/* t2 */ select b from t where a = 1 for update;
-- t2 >> +---+
-- t2 | b |
-- t2 +---+
-- t2 | 1 |
-- t2 +---+
/* t2 */ select a from t where b = 1 for update;
-- t2 >> blocked
/* t1 */ commit;
-- t1 >> 0 rows affected
-- t2 >> resumed
-- t2 >> +---+
-- t2 | a |
-- t2 +---+
-- t2 | 1 |
-- t2 +---+
/* t2 */ commit;
-- t2 >> 0 rows affected
There was a problem hiding this comment.
The behaviors of this part are not described in the documents yet, we could add them there.
Signed-off-by: zyguan <[email protected]>
| @@ -1239,6 +1243,10 @@ func (e *InsertValues) batchCheckAndInsert(ctx context.Context, rows [][]types.D | |||
| } 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-ingore | |||
| txnCtx.AddUnchangedRowKey(uk.newKey) | |||
There was a problem hiding this comment.
The behaviors of this part are not described in the documents yet, we could add them there.
| @@ -539,7 +539,7 @@ func TestOptimisticConflicts(t *testing.T) { | |||
| tk.MustExec("begin pessimistic") | |||
| // This SQL use BatchGet and cache data in the txn snapshot. | |||
| // It can be changed to other SQLs that use BatchGet. | |||
| tk.MustExec("insert ignore into conflict values (1, 2)") | |||
| tk.MustExec("select * from conflict where id in (1, 2, 3)") | |||
There was a problem hiding this comment.
I think this query can be removed.
There was a problem hiding this comment.
May I know why the query is unnecessary? IMO, it mean to test the snapshot cache. We cache (1, 2) here and don't want to see the following select-for-update (L547) returns the cached row. Although, it won't read from the cache even when we forget to invalidate the cache because it will read from lock cache firstly now (also, we won't reuse the snapshot with for-update-ts now). I think it's ok to keep the query since the test point still makes sense.
ti-chi-bot
added
status/LGT2
Co-authored-by: you06 <[email protected]>
Co-authored-by: you06 <[email protected]>
zyguan
changed the title
|
/retest |
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: b4cda16
|
ti-chi-bot
added
the
status/can-merge
|
/retest |
1 similar comment
|
/retest |
ti-chi-bot
added
needs-cherry-pick-release-5.3
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
…hing (pingcap#42210) (pingcap#42285)" This reverts commit f01eaf1.
…hing (pingcap#42210) (pingcap#42285)" This reverts commit f01eaf1.
What problem does this PR solve?
Issue Number: close #42121
Problem Summary: When doing insert-ingore or replace without changes, tidb doesn't lock conflict keys properly.
What is changed and how it works?
Lock keys even when insert-ingnore meets duplicated rows or replace without changes.
Check List
Tests
Side effects
The change may introduce more lock records, which may further lead performance regression like #25659 .
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.