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: only rebase auto increment ID when needed #7515

Merged
merged 8 commits into from
Aug 31, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
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
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

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

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"))
}