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.

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

executor: Fix a bug in 'INSERT ... ON DUPLICATE KEY UPDATE' #7675

Merged
merged 4 commits into from
Sep 14, 2018

Conversation

bb7133
Copy link
Member

@bb7133 bb7133 commented Sep 12, 2018

What problem does this PR solve?

Fix a bug in 'INSERT ... ON DUPLICATE KEY UPDATE' when unique key are updated, for example:

mysql> select * from t1;
+------+------+------+
| id   | a    | b    |
+------+------+------+
|    1 |    1 |    1 |
|    2 |    1 |    2 |
|    3 |    3 |    1 |
+------+------+------+
3 rows in set (0.00 sec)

mysql> create table t2(a int key, b int, unique(b));
Query OK, 0 rows affected (0.08 sec)

mysql> insert into t2 select a, b from t1 order by id on duplicate key update a=t1.a, b=t1.b;
Query OK, 6 rows affected (0.01 sec)

mysql> select * from t2;
+---+------+
| a | b    |
+---+------+
| 3 |    1 |
+---+------+
1 row in set (0.00 sec)

, which is incorrect. We should expect the following result instead:

mysql> select * from t2;
+---+------+
| a | b    |
+---+------+
| 1 |    2 |
| 3 |    1 |
+---+------+
2 rows in set (0.00 sec)

After this PR, it will be fixed.

What is changed and how it works?

In InsertExec's updateDupKeyValues function, dupKVs(a map that holds existing primary keys and unique keys for duplication detection) is updated by removing keys from the inserting row first, then adding keys from the updated row. But we should remove keys from the old row instead, because old row is the one that to be replaced.

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change

Side effects

  • No side effect

Related changes

  • Nothing else

@bb7133
Copy link
Member Author

bb7133 commented Sep 12, 2018

@jackysp @zz-jason PTAL, thanks!

@zz-jason zz-jason added contribution This PR is from a community contributor. sig/execution SIG execution type/bugfix This PR fixes a bug. labels Sep 12, 2018
@zz-jason
Copy link
Member

@XuHuaiyu @winoros PTAL

@zz-jason
Copy link
Member

/run-all-tests

@zz-jason
Copy link
Member

LGTM

@zz-jason zz-jason added status/LGT1 Indicates that a PR has LGTM 1. status/all tests passed labels Sep 12, 2018
@shenli
Copy link
Member

shenli commented Sep 12, 2018

@jackysp PTAL

// There is only one row per update.
fillBackKeysInRows, err := e.getKeysNeedCheck(e.ctx, e.Table, [][]types.Datum{updatedRow})
if err != nil {
return errors.Trace(err)
}
// Delete old keys and fill back new key-values of the updated row.
e.deleteDupKeys(row)
cleanupRows, err := e.getKeysNeedCheck(e.ctx, e.Table, [][]types.Datum{oldRow})
Copy link
Member

Choose a reason for hiding this comment

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

We could use a function to handle this part and

// Cleanup keys map, because the record was removed.

together.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review @jackysp, I plan to define a function for this and put it in the base struct InsertValues, what do u think?

Copy link
Member

Choose a reason for hiding this comment

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

I think to put it into batchChecker is better. Thanks for your contribution, @bb7133 !

@bb7133
Copy link
Member Author

bb7133 commented Sep 14, 2018

Comments addressed, PTAL @jackysp , thanks!

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @bb7133 !

@jackysp
Copy link
Member

jackysp commented Sep 14, 2018

/run-all-tests

@jackysp jackysp added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 14, 2018
@jackysp jackysp merged commit 33a30cc into pingcap:master Sep 14, 2018
@bb7133
Copy link
Member Author

bb7133 commented Sep 14, 2018

Thank you @jackysp

@bb7133 bb7133 deleted the bb7133/fix_insert branch March 7, 2019 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants