executor: only rebase auto increment ID when needed #7515

jackysp · 2018-08-28T06:42:44Z

What problem does this PR solve?

Fix #7422 . It is better to compare the value of the auto-increment ID before rebasing it in updateRecord. Update an old value which was inserted from another TiDB when the auto-increment column has no change does not need to rebase its ID. After this PR, the increasing speed of the auto-increment ID value will be slower than before.

What is changed and how it works?

Compare the values before rebasing the ID.

Check List

Tests

Unit test
Manual test (add detailed scripts or steps below)
The same SQL in Updating rows by auto_increment id cause id increasing dramatically #7422

Code changes

Has exported function/method change

Side effects

Increased code complexity

Related changes

Need to be included in the release note

PTAL @zimulala @coocood

jackysp · 2018-08-28T07:12:21Z

/run-all-tests

winkyao · 2018-08-28T07:56:51Z

executor/write_test.go

+	c.Assert(err, IsNil)
+	c.Assert(s.ctx.Txn().Commit(context.Background()), IsNil)
+
+	tk.MustExec(`update t set b = 3 where a = 30001;`)


Please add comments to this case,

winkyao

Reset LGTM

jackysp · 2018-08-28T08:06:54Z

PTAL @winkyao

winkyao

LGTM

lysu

LGTM

lysu

it seems more complex...hold on

coocood · 2018-08-28T13:45:28Z

executor/write.go

+		cmp, err := newData[i].CompareDatum(sc, &oldData[i])
+		if err != nil {
+			return false, handleChanged, newHandle, 0, errors.Trace(err)
+		}
 		// Rebase auto increment id if the field is changed.
 		if mysql.HasAutoIncrementFlag(col.Flag) {


How about

if mysql.HasAutoIncrementFlag(col.Flag) && cmp != 0 {

If cmp == 0, we still need to set the last_insert_id. Actually, we already have the cases at line 595 to 627.

if cmp != 0 still can not change last_insert_id when under on duplicate key update.

tidb:

mysql> show create table ti2; +-------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | Table | Create Table | +-------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | ti2 | CREATE TABLE `ti2` ( `id` int(11) NOT NULL AUTO_INCREMENT, `v` int(11) DEFAULT NULL, `idx` int(11) DEFAULT NULL, PRIMARY KEY (`id`), UNIQUE KEY `idx` (`idx`) ) ENGINE=InnoDB DEFAULT CHARSET=latin1 COLLATE=latin1_bin AUTO_INCREMENT=30001 | +-------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (0.00 sec) mysql> select * from ti2; +----+------+------+ | id | v | idx | +----+------+------+ | 1 | 1 | 1 | +----+------+------+ 1 row in set (0.00 sec) mysql> select last_insert_id(); +------------------+ | last_insert_id() | +------------------+ | 1 | +------------------+ 1 row in set (0.00 sec) mysql> insert into ti2 (idx) values (1) on duplicate key update id = 3; Query OK, 3 rows affected (3.78 sec) mysql> select last_insert_id(); +------------------+ | last_insert_id() | +------------------+ | 3 | +------------------+ 1 row in set (0.00 sec)

mysql:

mysql> show create table ti2; +-------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | Table | Create Table | +-------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | ti2 | CREATE TABLE `ti2` ( `id` int(11) NOT NULL AUTO_INCREMENT, `v` int(11) DEFAULT NULL, `idx` int(11) DEFAULT NULL, PRIMARY KEY (`id`), UNIQUE KEY `idx` (`idx`) ) ENGINE=InnoDB AUTO_INCREMENT=2 DEFAULT CHARSET=latin1 | +-------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (0.00 sec) mysql> select * from ti2; +----+------+------+ | id | v | idx | +----+------+------+ | 1 | 1 | 1 | +----+------+------+ 1 row in set (0.00 sec) mysql> select last_insert_id(); +------------------+ | last_insert_id() | +------------------+ | 1 | +------------------+ 1 row in set (0.00 sec) mysql> insert into ti2 (idx) values (1) on duplicate key update id = 3; Query OK, 2 rows affected (0.01 sec) mysql> select last_insert_id(); +------------------+ | last_insert_id() | +------------------+ | 1 | +------------------+ 1 row in set (0.00 sec)

Oh, you're right. I think we need another pr to fix it.

lysu · 2018-08-28T14:14:47Z

executor/write_test.go

+// For issue 7422.
+// There is no need to do the rebase when updating a record if the auto-increment ID not changed.
+// This could make the auto ID increasing speed slower.
+func (s *testSuite) TestRebaseIfNeeded(c *C) {


Maybe need add insert on duplicate key update testcase, and check last_insert_id() value.

Already have the cases at line 595 to 627.

I means rebase auto_increment in on duplicate key update, current case seems not update id? e.g. use duplicate on second unique index then update primary key...

jackysp · 2018-08-30T06:49:46Z

PTAL @lysu

coocood · 2018-08-30T08:17:43Z

LGTM

jackysp · 2018-08-31T05:57:32Z

/run-all-tests

jackysp added 2 commits August 28, 2018 13:50

Only rebase auto ID when needed.

1a3841a

add a test case

455b37b

jackysp added the sig/execution SIG execution label Aug 28, 2018

fix CI

4a9a6ae

jackysp added the status/all tests passed label Aug 28, 2018

winkyao reviewed Aug 28, 2018

View reviewed changes

Address comments.

908193d

winkyao reviewed Aug 28, 2018

View reviewed changes

lysu reviewed Aug 28, 2018

View reviewed changes

lysu approved these changes Aug 28, 2018

View reviewed changes

lysu added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 28, 2018

Merge branch 'master' into rebase-if-needed

c9e10da

lysu suggested changes Aug 28, 2018

View reviewed changes

coocood reviewed Aug 28, 2018

View reviewed changes

lysu reviewed Aug 28, 2018

View reviewed changes

lysu removed the status/LGT2 Indicates that a PR has LGTM 2. label Aug 29, 2018

jackysp added 2 commits August 30, 2018 14:34

merge master

220689c

address comments

61c7e11

lysu approved these changes Aug 31, 2018

View reviewed changes

Merge branch 'master' into rebase-if-needed

ff9d03a

jackysp merged commit 720e823 into pingcap:master Aug 31, 2018

jackysp deleted the rebase-if-needed branch September 7, 2018 11:04

jackysp mentioned this pull request Oct 9, 2018

executor: only rebase auto increment ID when needed (cherry-pick) #7846

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: only rebase auto increment ID when needed #7515

executor: only rebase auto increment ID when needed #7515

jackysp commented Aug 28, 2018

jackysp commented Aug 28, 2018

winkyao Aug 28, 2018

winkyao left a comment

jackysp commented Aug 28, 2018

winkyao left a comment

lysu left a comment

lysu left a comment •

edited

Loading

coocood Aug 28, 2018

jackysp Aug 29, 2018

lysu Aug 29, 2018

jackysp Aug 29, 2018

lysu Aug 28, 2018 •

edited

Loading

jackysp Aug 29, 2018

lysu Aug 29, 2018 •

edited

Loading

jackysp commented Aug 30, 2018

coocood commented Aug 30, 2018

jackysp commented Aug 31, 2018

executor: only rebase auto increment ID when needed #7515

executor: only rebase auto increment ID when needed #7515

Conversation

jackysp commented Aug 28, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

jackysp commented Aug 28, 2018

winkyao Aug 28, 2018

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

jackysp commented Aug 28, 2018

winkyao left a comment

Choose a reason for hiding this comment

lysu left a comment

Choose a reason for hiding this comment

lysu left a comment • edited Loading

Choose a reason for hiding this comment

coocood Aug 28, 2018

Choose a reason for hiding this comment

jackysp Aug 29, 2018

Choose a reason for hiding this comment

lysu Aug 29, 2018

Choose a reason for hiding this comment

jackysp Aug 29, 2018

Choose a reason for hiding this comment

lysu Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

jackysp Aug 29, 2018

Choose a reason for hiding this comment

lysu Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

jackysp commented Aug 30, 2018

coocood commented Aug 30, 2018

jackysp commented Aug 31, 2018

lysu left a comment •

edited

Loading

lysu Aug 28, 2018 •

edited

Loading

lysu Aug 29, 2018 •

edited

Loading