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

executor: only rebase auto increment ID when needed #7515

Merged
merged 8 commits into from
Aug 31, 2018
Merged

executor: only rebase auto increment ID when needed #7515

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1a3841a
Only rebase auto ID when needed.
jackysp Aug 28, 2018
455b37b
add a test case
jackysp Aug 28, 2018
4a9a6ae
fix CI
jackysp Aug 28, 2018
908193d
Address comments.
jackysp Aug 28, 2018
c9e10da
Merge branch 'master' into rebase-if-needed
lysu Aug 28, 2018
220689c
merge master
jackysp Aug 30, 2018
61c7e11
address comments
jackysp Aug 30, 2018
ff9d03a
Merge branch 'master' into rebase-if-needed
jackysp Aug 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions executor/write.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ func updateRecord(ctx sessionctx.Context, h int64, oldData, newData []types.Datu
return false, handleChanged, newHandle, 0, errors.Trace(err)
}
}
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) {
Copy link
Member

Choose a reason for hiding this comment

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

How about

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

if newData[i].IsNull() {
Expand All @@ -83,15 +87,13 @@ func updateRecord(ctx sessionctx.Context, h int64, oldData, newData []types.Datu
return false, handleChanged, newHandle, 0, errors.Trace(errTI)
}
lastInsertID = uint64(val)
err := t.RebaseAutoID(ctx, val, true)
if err != nil {
return false, handleChanged, newHandle, 0, errors.Trace(err)
if cmp != 0 {
err := t.RebaseAutoID(ctx, val, true)
if err != nil {
return false, handleChanged, newHandle, 0, errors.Trace(err)
}
}
}
cmp, err := newData[i].CompareDatum(sc, &oldData[i])
if err != nil {
return false, handleChanged, newHandle, 0, errors.Trace(err)
}
if cmp != 0 {
changed = true
modified[i] = true
Expand Down
25 changes: 25 additions & 0 deletions executor/write_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2031,3 +2031,28 @@ func (s *testSuite) TestReplaceLog(c *C) {

tk.MustQuery(`admin cleanup index testLog b;`).Check(testkit.Rows("1"))
}

// 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) {
Copy link
Contributor

@lysu lysu Aug 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Already have the cases at line 595 to 627.

Copy link
Contributor

@lysu lysu Aug 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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...

tk := testkit.NewTestKit(c, s.store)
tk.MustExec("use test")
tk.MustExec(`create table t (a int not null primary key auto_increment, b int unique key);`)
tk.MustExec(`insert into t (b) values (1);`)

s.ctx = mock.NewContext()
s.ctx.Store = s.store
tbl, err := s.domain.InfoSchema().TableByName(model.NewCIStr("test"), model.NewCIStr("t"))
c.Assert(err, IsNil)
c.Assert(s.ctx.NewTxn(), IsNil)
// AddRecord directly here will skip to rebase the auto ID in the insert statement,
// which could simulate another TiDB adds a large auto ID.
_, err = tbl.AddRecord(s.ctx, types.MakeDatums(30001, 2), false)
c.Assert(err, IsNil)
c.Assert(s.ctx.Txn().Commit(context.Background()), IsNil)

tk.MustExec(`update t set b = 3 where a = 30001;`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comments to this case,

tk.MustExec(`insert into t (b) values (4);`)
tk.MustQuery(`select a from t where b = 4;`).Check(testkit.Rows("2"))
}