lightning: fix auto_increment out-of-range error #34146
Conversation
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
needs-cherry-pick-release-5.2
release-note-none
|
/cc @gozssky @glorv |
|
/component lightning |
ti-chi-bot
added
the
component/lightning
|
Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/7c7faa3971098434930bb313d507a1ec6321f1db |
| if incr > math.MaxInt64 { | ||
| logger.Warn("auto_increment out of the maximum value TiDB supports", zap.Uint64("auto_increment", incr)) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Do we need to set auto_increment to math.MaxInt64 when incr > math.MaxInt64?
There was a problem hiding this comment.
MaxInt64 is a legal value in TiDB (i.e. you can insert (9223372036854775807, ....) to a table) whereas MaxInt64+1 is illegal, which will trigger the error, ERROR 1467 (HY000): Failed to read auto-increment value from storage engine.
e.g.
mysql> create table test1(
-> a bigint auto_increment,
-> b int,
-> primary key(a));
Query OK, 0 rows affected (0.11 sec)
mysql> alter table test1 auto_increment=9223372036854775807;
Query OK, 0 rows affected (0.12 sec)
mysql> insert into test1(b) values(1);
Query OK, 1 row affected (0.00 sec)
mysql> select * from test1;
+---------------------+------+
| a | b |
+---------------------+------+
| 9223372036854775807 | 1 |
+---------------------+------+
1 row in set (0.00 sec)
There was a problem hiding this comment.
Does incr > math.MaxInt64 means lightning has inserted rows whose id is greater than math.MaxInt64? Will this cause duplicate entry or data corruption?
There was a problem hiding this comment.
Mostly yes. But in terms of the case shown in the linked issue, 9223372036854775807 is a valid input, but after inserting it, Lightning would try alter table xxx auto_increment=9223372036854775807+1, get syntax error from TiDB, and fail unexpectedly.
So I'm not trying to legalize auto-incr value that exceeds math.MaxInt64 but try to skip this syntax error and let Lightning fail by other clearer issues (such as data corruption, duplicate entry, etc.). As for the case in this issue, because Lightning doesn't try to write entries that are larger than 9223372036854775807, it will succeed.
Failing by duplicate entries might be more intuitive in terms of the effectiveness of reporting the error. Syntax error is helpless...
There was a problem hiding this comment.
Could confirm whether data corruption or any other error can be reported when using local backend and auto_inc is overflow?
There was a problem hiding this comment.
Yes. If the auto_incr column overflows, the checksum will be mismatched.
e.g. create a table with auto-incremental key:
CREATE TABLE `test1` (
`id` tinyint(4) NOT NULL AUTO_INCREMENT,
`a` int(11) NOT NULL,
PRIMARY KEY (`a`),
UNIQUE KEY `id` (`id`)
)insert 256 rows (where only one column "a" in the source file), and get error:
Error: [Lighting:Restore:ErrChecksumMismatch]checksum mismatched remote vs local => (checksum: 1088876813058384307 vs 11572238353217052498) (total_kvs: 383 vs 512) (total_bytes:11996 vs 16640)
tidb lightning encountered error: [Lighting:Restore:ErrChecksumMismatch]checksum mismatched remote vs local => (checksum: 1088876813058384307 vs 11572238353217052498) (total_kvs: 383 vs 512) (total_bytes:11996 vs 16640)
There was a problem hiding this comment.
"checksum mismatch" is hard for user to know what happened. How about checking overflow during encoding and give a clear error to user?
There was a problem hiding this comment.
"checksum mismatch" is hard for user to know what happened. How about checking overflow during encoding and give a clear error to user?
Yes. Actually I'm looking at it #28776. This PR only solves the unexpected syntax error.
|
/cc @D3Hunter |
| logger := log.With(zap.String("table", tableName), zap.Int64("auto_increment", incr)) | ||
| func AlterAutoIncrement(ctx context.Context, g glue.SQLExecutor, tableName string, incr uint64) error { | ||
| logger := log.With(zap.String("table", tableName), zap.Uint64("auto_increment", incr)) | ||
| if incr > math.MaxInt64 { |
There was a problem hiding this comment.
Since incr > math.MaxInt64 can only happen when there is one row that there is one rwo explitly set the max-value, so here you should alter the auto increment value to the max value then. So the behavior then is the same as import via sql.
Please also add a test case to ensure: Lightning local backend can successfully import data which explicitly set auto_increment row value to the max valid value according to tidb's restriction. After import, any new insert statement without explicitly set the auto_increment row will result with an error.
There was a problem hiding this comment.
I'll add a IT later. But it seems alter table xxx auto_increment=math.MaxInt64 will lead to some unexpected errors (I've talked to the guys from sql-infra, it might be bugs of TiDB #34142). So I won't set it until they fix this issue (leave a TODO here, or perhaps I can use alter table xxx force ...).
ti-chi-bot
added
size/L
|
/run-integration-br-test |
buchuitoudegou
force-pushed
the
auto-incr
branch
4 times, most recently
from
67691e8 to
683c730
Compare
buchuitoudegou
force-pushed
the
auto-incr
branch
from
683c730 to
061987b
Compare
|
This pull request has been accepted and is ready to merge. Commit hash: 6959514
|
ti-chi-bot
added
the
status/can-merge
|
/run-mysql-test |
|
/run-mysql-test |
1 similar comment
|
/run-mysql-test |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
|
cherry pick to release-5.2 in PR #34333 |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
|
cherry pick to release-5.3 in PR #34334 |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
|
cherry pick to release-5.4 in PR #34335 |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
|
cherry pick to release-6.0 in PR #34336 |
TiDB MergeCI notify🔴 Bad News! New failing [2] after this pr merged.
|
What problem does this PR solve?
Issue Number: close #27937
Problem Summary:
What is changed and how it works?
AlterAutoIncrement's parameter touint64whose maximum is larger thanBIGINT.incris larger thanmath.MaxInt64(i.e. maximum ofBIGINT)Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.