-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
lightning: fix auto_increment out-of-range error #34146
Changes from all commits
62c7c5e
061987b
95a2442
54b2dae
53c8056
23ca467
a14195f
a7a11b7
17b9c33
7ec3615
6959514
8635dc7
2a59d47
8879e8b
f726489
3190dc5
2f04407
7c7faa3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ import ( | |
"context" | ||
"database/sql" | ||
"fmt" | ||
"math" | ||
"strconv" | ||
"strings" | ||
|
||
|
@@ -373,9 +374,17 @@ func ObtainNewCollationEnabled(ctx context.Context, g glue.SQLExecutor) (bool, e | |
// NOTE: since tidb can make sure the auto id is always be rebase even if the `incr` value is smaller | ||
// the the auto incremanet base in tidb side, we needn't fetch currently auto increment value here. | ||
// See: https://github.com/pingcap/tidb/blob/64698ef9a3358bfd0fdc323996bb7928a56cadca/ddl/ddl_api.go#L2528-L2533 | ||
func AlterAutoIncrement(ctx context.Context, g glue.SQLExecutor, tableName string, incr int64) error { | ||
logger := log.With(zap.String("table", tableName), zap.Int64("auto_increment", incr)) | ||
query := fmt.Sprintf("ALTER TABLE %s AUTO_INCREMENT=%d", tableName, incr) | ||
func AlterAutoIncrement(ctx context.Context, g glue.SQLExecutor, tableName string, incr uint64) error { | ||
var query string | ||
logger := log.With(zap.String("table", tableName), zap.Uint64("auto_increment", incr)) | ||
if incr > math.MaxInt64 { | ||
// automatically set max value | ||
logger.Warn("auto_increment out of the maximum value TiDB supports, automatically set to the max", zap.Uint64("auto_increment", incr)) | ||
incr = math.MaxInt64 | ||
query = fmt.Sprintf("ALTER TABLE %s FORCE AUTO_INCREMENT=%d", tableName, incr) | ||
} else { | ||
query = fmt.Sprintf("ALTER TABLE %s AUTO_INCREMENT=%d", tableName, incr) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, e.g.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly yes. But in terms of the case shown in the linked issue, 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. If the 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. Actually I'm looking at it #28776. This PR only solves the unexpected syntax error. |
||
task := logger.Begin(zap.InfoLevel, "alter table auto_increment") | ||
err := g.ExecuteWithLog(ctx, query, "alter table auto_increment", logger) | ||
task.End(zap.ErrorLevel, err) | ||
|
@@ -388,8 +397,16 @@ func AlterAutoIncrement(ctx context.Context, g glue.SQLExecutor, tableName strin | |
return errors.Annotatef(err, "%s", query) | ||
} | ||
|
||
func AlterAutoRandom(ctx context.Context, g glue.SQLExecutor, tableName string, randomBase int64) error { | ||
logger := log.With(zap.String("table", tableName), zap.Int64("auto_random", randomBase)) | ||
func AlterAutoRandom(ctx context.Context, g glue.SQLExecutor, tableName string, randomBase uint64, maxAutoRandom uint64) error { | ||
logger := log.With(zap.String("table", tableName), zap.Uint64("auto_random", randomBase)) | ||
if randomBase == maxAutoRandom+1 { | ||
// insert a tuple with key maxAutoRandom | ||
randomBase = maxAutoRandom | ||
} else if randomBase > maxAutoRandom { | ||
// TiDB does nothing when inserting an overflow value | ||
logger.Warn("auto_random out of the maximum value TiDB supports") | ||
return nil | ||
} | ||
query := fmt.Sprintf("ALTER TABLE %s AUTO_RANDOM_BASE=%d", tableName, randomBase) | ||
task := logger.Begin(zap.InfoLevel, "alter table auto_random") | ||
err := g.ExecuteWithLog(ctx, query, "alter table auto_random_base", logger) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
[tikv-importer] | ||
backend = 'local' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
create database db; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
create table test( | ||
a bigint auto_increment, | ||
b int, | ||
primary key(a) | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
"a","b" | ||
1,2 | ||
9223372036854775805,3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
create table test1( | ||
a bigint auto_increment, | ||
b int, | ||
primary key(a) | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
"a","b" | ||
1,2 | ||
9223372036854775807,3 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is 2^53 better? Though 2^63-1 is larger than the max_auto_random_base, it's still a valid value. Perhaps I could add a check to see if we could go on inserting tuples to test1 to prove auto_random_base is set to correct value so that the table remains available and fulfills constraints. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a reminder, we should compare the increment bits, but not the final value. e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix in 17b9c33 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
#!/bin/sh | ||
# | ||
# Copyright 2022 PingCAP, Inc. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
set -eux | ||
|
||
check_cluster_version 4 0 0 'local backend' || exit 0 | ||
|
||
ENGINE_COUNT=6 | ||
|
||
check_result() { | ||
run_sql 'SHOW DATABASES;' | ||
check_contains 'Database: db'; | ||
run_sql 'SHOW TABLES IN db;' | ||
check_contains 'Tables_in_db: test' | ||
check_contains 'Tables_in_db: test1' | ||
run_sql 'SELECT count(*) FROM db.test;' | ||
check_contains 'count(*): 2' | ||
run_sql 'SELECT count(*) FROM db.test1;' | ||
check_contains 'count(*): 2' | ||
} | ||
|
||
cleanup() { | ||
rm -f $TEST_DIR/lightning.log | ||
rm -rf $TEST_DIR/sst | ||
run_sql 'DROP DATABASE IF EXISTS db;' | ||
} | ||
|
||
cleanup | ||
|
||
# db.test contains key that is less than int64 - 1 | ||
# while db.test1 contains key that equals int64 - 1 | ||
run_lightning --sorted-kv-dir "$TEST_DIR/sst" --config "tests/$TEST_NAME/config.toml" --log-file "$TEST_DIR/lightning.log" | ||
check_result | ||
# successfully insert: max key has not reached maximum | ||
run_sql 'INSERT INTO db.test(b) VALUES(11);' | ||
# fail for insertion: db.test1 has key int64 - 1 | ||
run_sql 'INSERT INTO db.test1(b) VALUES(22);' 2>&1 | tee -a "$TEST_DIR/sql_res.$TEST_NAME.txt" | ||
check_contains 'ERROR' | ||
cleanup |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
[tikv-importer] | ||
backend = 'local' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
create database db; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
create table test( | ||
a bigint auto_random(10), | ||
b int, | ||
primary key(a) | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
"a","b" | ||
1,2 | ||
9007199254740990,3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
create table test1( | ||
a bigint auto_random(10), | ||
b int, | ||
primary key(a) | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
"a","b" | ||
1,2 | ||
9007199254740991,3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
create table test2( | ||
a bigint auto_random(10), | ||
b int, | ||
primary key(a) | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
"a","b" | ||
1,2 | ||
9007199254740992,3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
#!/bin/sh | ||
# | ||
# Copyright 2022 PingCAP, Inc. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
set -eux | ||
|
||
check_cluster_version 4 0 0 'local backend' || exit 0 | ||
|
||
ENGINE_COUNT=6 | ||
|
||
check_result() { | ||
run_sql 'SHOW DATABASES;' | ||
check_contains 'Database: db'; | ||
run_sql 'SHOW TABLES IN db;' | ||
check_contains 'Tables_in_db: test' | ||
check_contains 'Tables_in_db: test1' | ||
check_contains 'Tables_in_db: test2' | ||
run_sql 'SELECT count(*) FROM db.test;' | ||
check_contains 'count(*): 2' | ||
run_sql 'SELECT count(*) FROM db.test1;' | ||
check_contains 'count(*): 2' | ||
run_sql 'SELECT count(*) FROM db.test2;' | ||
check_contains 'count(*): 2' | ||
} | ||
|
||
cleanup() { | ||
rm -f $TEST_DIR/lightning.log | ||
rm -rf $TEST_DIR/sst | ||
run_sql 'DROP DATABASE IF EXISTS db;' | ||
} | ||
|
||
cleanup | ||
|
||
# auto_random_max = 2^{64-1-10}-1 | ||
# db.test contains key auto_random_max - 1 | ||
# db.test1 contains key auto_random_max | ||
# db.test2 contains key auto_random_max + 1 (overflow) | ||
run_lightning --sorted-kv-dir "$TEST_DIR/sst" --config "tests/$TEST_NAME/config.toml" --log-file "$TEST_DIR/lightning.log" | ||
check_result | ||
# successfully insert: d.test auto_random key has not reached maximum | ||
run_sql 'INSERT INTO db.test(b) VALUES(11);' | ||
# fail for further insertion | ||
run_sql 'INSERT INTO db.test(b) VALUES(22);' 2>&1 | tee -a "$TEST_DIR/sql_res.$TEST_NAME.txt" | ||
check_contains 'ERROR' | ||
# fail: db.test1 has key auto_random_max | ||
run_sql 'INSERT INTO db.test1(b) VALUES(11);' | ||
run_sql 'INSERT INTO db.test1(b) VALUES(22);' 2>&1 | tee -a "$TEST_DIR/sql_res.$TEST_NAME.txt" | ||
check_contains 'ERROR' | ||
# successfully insert for overflow key | ||
run_sql 'INSERT INTO db.test2(b) VALUES(33);' | ||
run_sql 'INSERT INTO db.test2(b) VALUES(44);' | ||
run_sql 'INSERT INTO db.test2(b) VALUES(55);' | ||
cleanup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usealter table xxx force ...
).