Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

executer: fix the last_insert_id in insert on duplicate key update #7534

Merged
merged 4 commits into from Aug 30, 2018
Merged

executer: fix the last_insert_id in insert on duplicate key update #7534

merged 4 commits into from Aug 30, 2018

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Aug 29, 2018

What problem does this PR solve? 

DROP TABLE IF EXISTS t1;
CREATE TABLE t1 (f1 INT AUTO_INCREMENT PRIMARY KEY,
f2 VARCHAR(5) NOT NULL UNIQUE);
INSERT t1 (f2) VALUES ('test');
SELECT LAST_INSERT_ID();
INSERT t1 (f2) VALUES ('test') ON DUPLICATE KEY UPDATE f1 = 2;
SELECT LAST_INSERT_ID();

The last insert ID should only be set when insert into an auto-increment column. The above SQL should obtain

LAST_INSERT_ID()
1
LAST_INSERT_ID()
1

What is changed and how it works?

Set the last insert ID when addRecord in the insert statement.

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

PTAL @lysu @zimulala

@jackysp jackysp added the sig/execution SIG execution label Aug 29, 2018
@jackysp jackysp changed the title executer: fix last insert id in insert on duplicate key update executer: fix the last_insert_id in insert on duplicate key update Aug 29, 2018
@jackysp
Copy link
Member Author

jackysp commented Aug 29, 2018

/run-all-tests

zimulala
zimulala reviewed Aug 29, 2018
View reviewed changes
executor/write.go Outdated
@@ -41,17 +41,15 @@ var (
// 1. changed (bool) : does the update really change the row values. e.g. update set i = 1 where i = 1;
// 2. handleChanged (bool) : is the handle changed after the update.
// 3. newHandle (int64) : if handleChanged == true, the newHandle means the new handle after update.
// 4. lastInsertID (uint64) : the lastInsertID should be set by the newData.
// 5. err (error) : error in the update.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/5/4

@jackysp
Copy link
Member Author

jackysp commented Aug 29, 2018

/run-unit-test

coocood
coocood reviewed Aug 29, 2018
View reviewed changes
executor/insert_common.go
if err != nil {
return 0, errors.Trace(err)
}
if e.lastInsertID != 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the e.lastInsertID is not set anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set it at line 449.

@coocood
Copy link
Member

coocood commented Aug 29, 2018

LGTM

@jackysp
Copy link
Member Author

jackysp commented Aug 30, 2018

/run-all-tests

zimulala
zimulala approved these changes Aug 30, 2018
View reviewed changes
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zimulala zimulala added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 30, 2018
@jackysp jackysp merged commit 9070bb6 into pingcap:master Aug 30, 2018
@jackysp jackysp deleted the last_insert_id branch September 7, 2018 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coocood coocood coocood left review comments

@zimulala zimulala zimulala approved these changes

Assignees
No one assigned
Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jackysp @coocood @zimulala