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, session: refine insert unsigned bigint autoIncreID #8181

Merged
merged 24 commits into from Dec 18, 2018

Conversation

@XuHuaiyu
Copy link
Contributor

commented Nov 5, 2018

Refer: pingcap/parser#34

What problem does this PR solve?

Before this commit, the behavior of the following sqls in TiDB is wrong.

  • unsigned auto_increment integer column:
-- We cannot insert an unsigned integer value which is larger than MaxInt64 into a signed auto_increment column.
tidb> create table unsigned_autoid(`auto_inc_id` bigint unsigned not null unique key auto_increment);
tidb> insert into unsigned_autoid values(9223372036854775808);
ERROR 1690 (22003): constant 9223372036854775808 overflows bigint
  • signed auto_increment integer column:
-- The value inserted is wrong if the auto_increment col is MaxInt64 already.
tidb> create table signed_auto_inc(auto_inc_col bigint signed auto_increment, key(au
to_inc_col));
Query OK, 0 rows affected (0.01 sec)
tidb> insert into signed_auto_inc value(cast((1<<63)-1 as signed));
Query OK, 1 row affected (0.00 sec)
tidb> insert into signed_auto_inc values();
Query OK, 1 row affected (0.00 sec)
tidb> insert into signed_auto_inc values();
Query OK, 1 row affected (0.00 sec)
tidb> select * from signed_auto_inc;
+----------------------+
| auto_inc_col         |
+----------------------+
|  9223372036854775807 |
| -9223372036854775807 |
| -9223372036854775805 |
+----------------------+
3 rows in set (0.00 sec)

What is changed and how it works?

  1. Add check for the unsigned flag for allocator.
  2. Add check for MaxUint64 and MaxInt64.

Note: After this commit, we can promise the correctness for the behavior when inserting a specific value or an auto-generated value into the auto-increment column.
BUT it is not 100% consistent with MySQL now, we :

  • unsigned auto_increment id
create table unsigned_autoid(`auto_inc_id` bigint unsigned not null unique key auto_increment);

-- MySQL:
mysql> insert into unsigned_autoid values(18446744073709551614);
Query OK, 1 row affected (0.00 sec)
mysql> insert into unsigned_autoid values();
ERROR 1467 (HY000): Failed to read auto-increment value from storage engine
mysql> insert into unsigned_autoid values(18446744073709551615); # MySQL performs well.
Query OK, 1 row affected (0.00 sec)

--TiDB:
tidb> insert into unsigned_autoid values(18446744073709551614);
Query OK, 1 row affected (0.00 sec)
tidb> insert into unsigned_autoid values();
ERROR 1467 (HY000): Failed to read auto-increment value from storage engine
tidb> insert into unsigned_autoid values(18446744073709551615);
ERROR 1467 (HY000): Failed to read auto-increment value from storage engine
  • signed auto_increment id
create table signed_autoid(`auto_inc_id` bigint signed not null unique key auto_increment);

-- MySQL:
mysql> insert into signed_autoid values(9223372036854775806);
Query OK, 1 row affected (0.00 sec)
mysql> insert into signed_autoid values(); # MySQL performs well.
Query OK, 1 row affected (0.00 sec)
mysql> insert into signed_autoid values(); # MySQL returns DupEntry error
ERROR 1062 (23000): Duplicate entry '9223372036854775807' for key 'auto_inc_id'

--TiDB:
tidb> insert into signed_autoid values(9223372036854775806);
Query OK, 1 row affected (0.00 sec)
tidb> insert into signed_autoid values();
ERROR 1467 (HY000): Failed to read auto-increment value from storage engine
tidb> insert into signed_autoid values();
ERROR 1467 (HY000): Failed to read auto-increment value from storage engine

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change

Side effects

none

Related changes

  • Need to cherry-pick to the release branch

release-2.0 release-2.1


This change is Reviewable

@XuHuaiyu

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

/run-all-tests

@zz-jason
Copy link
Member

left a comment

LGTM

@zz-jason zz-jason added the status/LGT1 label Nov 5, 2018

@zz-jason

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

/run-all-tests

@XuHuaiyu

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

tk.MustExec("drop table if exists autoid")
tk.MustExec("create table autoid(`auto_inc_id` bigint(20) UNSIGNED NOT NULL AUTO_INCREMENT,UNIQUE KEY `auto_inc_id` (`auto_inc_id`))")
tk.MustExec("insert into autoid values(9223372036854775808);")
tk.MustQuery("select * from autoid").Check(testkit.Rows("9223372036854775808"))

This comment has been minimized.

Copy link
@jackysp

jackysp Nov 5, 2018

Member

How about insert auto values after this? e.g. insert into autid values ();

This comment has been minimized.

Copy link
@jackysp

jackysp Nov 5, 2018

Member
mysql> create table autoid(`auto_inc_id` bigint(20) UNSIGNED NOT NULL AUTO_INCREMENT,UNIQUE KEY `auto_inc_id` (`auto_inc_id`));
Query OK, 0 rows affected (0.02 sec)

mysql> insert into autoid values(9223372036854775808);
Query OK, 1 row affected (0.00 sec)

mysql> select * from autoid;
+---------------------+
| auto_inc_id         |
+---------------------+
| 9223372036854775808 |
+---------------------+
1 row in set (0.00 sec)

mysql> insert into autoid values();
Query OK, 1 row affected (0.00 sec)

mysql> select * from autoid;
+---------------------+
| auto_inc_id         |
+---------------------+
| 9223372036854775808 |
|                   2 |
+---------------------+
2 rows in set (0.00 sec)

@XuHuaiyu XuHuaiyu added the status/DNM label Nov 5, 2018

@zimulala
Copy link
Member

left a comment

The ID generated on the TiKV side is int64. We see the function of "func (m *Meta) GenAutoTableID(dbID, tableID, step int64) (int64, error)".

recordID, err = d.ToInt64(sc)
if e.filterErr(err) != nil {
return types.Datum{}, errors.Trace(err)
}

This comment has been minimized.

Copy link
@eurekaka

eurekaka Nov 6, 2018

Contributor

ConvertTo checks mysql.HasUnsignedFlag, how about:

datum, err := d.ConvertTo(sc, &c.FieldType)
if e.filterErr(err) != nil {
    return types.Datum{}, err
}
recordID = datum.GetInt64()
@XuHuaiyu

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

@XuHuaiyu XuHuaiyu removed the status/DNM label Nov 8, 2018

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2018

pingcap/parser#34 is merged.
Please update the go.mod file to use the latest parser

@crazycs520
Copy link
Contributor

left a comment

I got a problem:

create table autoid(`auto_inc_id` bigint unsigned not null unique key auto_increment);
insert into autoid values(9223372036854775808);
insert into autoid values();
insert into autoid values(18446744073709551615);
insert into autoid values();
select * from autoid
+----------------------+
| auto_inc_id          |
+----------------------+
| 9223372036854775808  |
| 9223372036854805810  |
| 18446744073709551615 |
| 30001                |
+----------------------+
4 rows in set
Time: 0.006s

This may cause by func (t *TxStructure) HInc(key []byte, field []byte, step int64) (int64, error)

tk.MustExec("drop table if exists autoid")
tk.MustExec("create table autoid(`auto_inc_id` bigint(20) NOT NULL AUTO_INCREMENT,UNIQUE KEY `auto_inc_id` (`auto_inc_id`))")
tk.MustExec("insert into autoid values(9223372036854775806);")
tk.MustExec("insert into autoid values();")

This comment has been minimized.

Copy link
@crazycs520

crazycs520 Nov 12, 2018

Contributor

the test has a problem:

mysql root@127.0.0.1:test> create table autoid(`auto_inc_id` bigint(20) NOT NULL AUTO_INCREMENT,UNIQUE KEY `auto_inc_id` (`auto_inc_id`));
Query OK, 0 rows affected
mysql root@127.0.0.1:test> insert into autoid values(9223372036854775806);
Query OK, 1 row affected
mysql root@127.0.0.1:test> select auto_inc_id,_tidb_rowid from autoid
+---------------------+---------------------+
| auto_inc_id         | _tidb_rowid         |
+---------------------+---------------------+
| 9223372036854775806 | 9223372036854775807 |
+---------------------+---------------------+
1 row in set
mysql root@127.0.0.1:test> insert into autoid values(); # auto_id = rowid= 9223372036854775807, cause below admin check error.
Query OK, 1 row affected
mysql root@127.0.0.1:test> select auto_inc_id,_tidb_rowid from autoid
+---------------------+---------------------+
| auto_inc_id         | _tidb_rowid         |
+---------------------+---------------------+
| 9223372036854775807 | 9223372036854775807 |
+---------------------+---------------------+
mysql root@127.0.0.1:test> admin check table autoid;
(8003, u'autoid err:[admin:1]index:&admin.RecordData{Handle:9223372036854775807, Values:[]types.Datum{types.Datum{k:0x1, collation:0x0, decimal:0x0, length:0x0, i:9223372036854775806, b:[]uint8(nil), x:interface {}(nil)}}} != record:&admin.RecordData{Handle:9223372036854775807, Values:[]types.Datum{types.Datum{k:0x1, collation:0x0, decimal:0x0, length:0x0, i:9223372036854775807, b:[]uint8(nil), x:interface {}(nil)}}}')
mysql root@127.0.0.1:test> select count(*) from autoid;
+----------+
| count(*) |
+----------+
| 1        |
+----------+
1 row in set
mysql root@127.0.0.1:test> select count(*) from autoid use index(auto_inc_id);
+----------+
| count(*) |
+----------+
| 2        |
+----------+
1 row in set

This comment has been minimized.

Copy link
@zimulala

zimulala Nov 13, 2018

Member

Please address this comment @XuHuaiyu.

alloc.base++
log.Debugf("[kv] Alloc id %d, table ID:%d, from %p, database ID:%d", alloc.base, tableID, alloc, alloc.dbID)
if alloc.isUnsigned {
if uint64(alloc.base) == math.MaxUint64 {

This comment has been minimized.

Copy link
@crazycs520

crazycs520 Nov 12, 2018

Contributor

!alloc.isUnsigned need this kind of check( if alloc.base == math.MaxInt64 ) too.

This comment has been minimized.

Copy link
@zimulala

zimulala Nov 13, 2018

Member

Could we put this check to line204? Then we needn't do check each time Alloc is called.

@crazycs520

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2018

I have a question: Why auto_increment and _tidb_rowid both use the same allocator?
@zimulala

@@ -137,13 +137,12 @@ func (alloc *allocator) Rebase(tableID, requiredBase int64, allocIDs bool) error
if allocIDs {
if alloc.isUnsigned {

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Nov 13, 2018

Contributor

You want to prevent overflow?
Should we

if uNewBase > (math.Uint64 - step) {
   panic
}

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Nov 13, 2018

Author Contributor

Yes,
but why we panic here? @tiancaiamao

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Nov 13, 2018

Contributor

Well, maybe error or warning...
I mean, if the ID overflow, it should not be changed to math.Uint64

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Nov 13, 2018

Author Contributor

This check is used to prevent the alloc.end overflow.
alloc.base is checked here.

It may be reasonable to change alloc.end to math.Uint64 if it will overflow?
@tiancaiamao

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Nov 13, 2018

Contributor

If alloc.end overflow, what will happen?

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Nov 13, 2018

Author Contributor

@tiancaiamao
The result of TiDB master:

tidb> create table t(a bigint signed not null auto_increment, unique key(a));
Query OK, 0 rows affected (0.02 sec)

tidb> insert into t value(9223372036854775806);
Query OK, 1 row affected (0.00 sec)

tidb> insert into t value();
Query OK, 1 row affected (0.00 sec)

tidb> insert into t value();
Query OK, 1 row affected (0.00 sec)

tidb> insert into t value();
Query OK, 1 row affected (0.00 sec)

tidb> insert into t value();
Query OK, 1 row affected (0.00 sec)

tidb> select * from t;
+----------------------+
| a                    |
+----------------------+
| -9223372036854775808 |
| -9223372036854775806 |
| -9223372036854775804 |
| -9223372036854775802 |
|  9223372036854775806 |
+----------------------+
5 rows in set (0.00 sec)

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Nov 13, 2018

Contributor

Overwrite the old row id is dangerous for data consistency.
Is it acceptable to change the behavior to throw error?

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Nov 13, 2018

Author Contributor

As a tradeoff, #8181 (review) may be better?

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Nov 13, 2018

Contributor

I think throw error is simple, easier, and more robust.

Show resolved Hide resolved meta/autoid/autoid.go Outdated

XuHuaiyu added some commits Nov 13, 2018

@XuHuaiyu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

should be merged with pingcap/parser#79

@jackysp
Copy link
Member

left a comment

LGTM

}
alloc.base, alloc.end = newBase, newEnd
}

alloc.base++
log.Debugf("[kv] Alloc id %d, table ID:%d, from %p, database ID:%d", alloc.base, tableID, alloc, alloc.dbID)
log.Debugf("[kv] Alloc id %d, table ID:%d, from %p, database ID:%d", uint64(alloc.base), tableID, alloc, alloc.dbID)

This comment has been minimized.

Copy link
@zimulala

zimulala Dec 14, 2018

Member

Could we remove "uint64"?

@@ -57,7 +57,7 @@ func (*testSuite) TestT(c *C) {
})
c.Assert(err, IsNil)

alloc := autoid.NewAllocator(store, 1)
alloc := autoid.NewAllocator(store, 1, false)

This comment has been minimized.

Copy link
@zimulala

zimulala Dec 14, 2018

Member

Please add some tests for NewAllocator when isUnsigned is true. Otherwise, the test coverage of this package will decrease.

@crazycs520
Copy link
Contributor

left a comment

LGTM

@winkyao
Copy link
Member

left a comment

LGTM

@winkyao

This comment has been minimized.

Copy link
Member

commented Dec 15, 2018

@XuHuaiyu CI failed.

jackysp and others added some commits Dec 17, 2018

@XuHuaiyu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

/run-all-tests tidb-test=pr/651

XuHuaiyu added some commits Dec 18, 2018

@zimulala
Copy link
Member

left a comment

LGTM

@XuHuaiyu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

/run-all-tests tidb-test=pr/651

@XuHuaiyu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

/run-all-tests tidb-test=pr/651

@XuHuaiyu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

/run-common-tests
/run-integration-common-test

@XuHuaiyu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

/run-common-tests tidb-test=pr/651
/run-integration-common-test tidb-test=pr/651

@zz-jason
Copy link
Member

left a comment

LGTM

@XuHuaiyu XuHuaiyu merged commit 7056bb0 into pingcap:master Dec 18, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@XuHuaiyu XuHuaiyu deleted the XuHuaiyu:unsigned_autoid branch Dec 18, 2018

XuHuaiyu added a commit to XuHuaiyu/tidb that referenced this pull request Feb 18, 2019

zz-jason added a commit that referenced this pull request Feb 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.