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

lightning: fix auto_increment out-of-range error #34146

Merged
merged 18 commits into from
Apr 29, 2022
Merged

lightning: fix auto_increment out-of-range error #34146

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
62c7c5e
fix: auto_increment exceeds limit
buchuitoudegou Apr 21, 2022
061987b
fix comment: fix auto_random & add it
buchuitoudegou Apr 21, 2022
95a2442
fix: limit auto_random maximum
buchuitoudegou Apr 22, 2022
54b2dae
fix: lint
buchuitoudegou Apr 22, 2022
53c8056
fix: comment
buchuitoudegou Apr 22, 2022
23ca467
revert: run_sql_fail
buchuitoudegou Apr 24, 2022
a14195f
update it
buchuitoudegou Apr 25, 2022
a7a11b7
fix it: prove db.test1 valid
buchuitoudegou Apr 25, 2022
17b9c33
fix it: refine auto_random testcase
buchuitoudegou Apr 28, 2022
7ec3615
fix it: refactor auto_random and auto_incr
buchuitoudegou Apr 28, 2022
6959514
fix ut
buchuitoudegou Apr 28, 2022
8635dc7
Merge branch 'master' into auto-incr
ti-chi-bot Apr 28, 2022
2a59d47
Merge branch 'master' into auto-incr
ti-chi-bot Apr 28, 2022
8879e8b
Merge branch 'master' into auto-incr
ti-chi-bot Apr 28, 2022
f726489
Merge branch 'master' into auto-incr
ti-chi-bot Apr 28, 2022
3190dc5
Merge branch 'master' into auto-incr
ti-chi-bot Apr 29, 2022
2f04407
Merge branch 'master' into auto-incr
ti-chi-bot Apr 29, 2022
7c7faa3
Merge branch 'master' into auto-incr
ti-chi-bot Apr 29, 2022
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
2 changes: 1 addition & 1 deletion br/pkg/lightning/restore/table_restore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,7 @@ func (tr *TableRestore) postProcess(
err = AlterAutoRandom(ctx, rc.tidbGlue.GetSQLExecutor(), tr.tableName, tr.alloc.Get(autoid.AutoRandomType).Base()+1)
} else if common.TableHasAutoRowID(tblInfo) || tblInfo.GetAutoIncrementColInfo() != nil {
// only alter auto increment id iff table contains auto-increment column or generated handle
err = AlterAutoIncrement(ctx, rc.tidbGlue.GetSQLExecutor(), tr.tableName, tr.alloc.Get(autoid.RowIDAllocType).Base()+1)
err = AlterAutoIncrement(ctx, rc.tidbGlue.GetSQLExecutor(), tr.tableName, uint64(tr.alloc.Get(autoid.RowIDAllocType).Base())+1)
}
rc.alterTableLock.Unlock()
saveCpErr := rc.saveStatusCheckpoint(ctx, tr.tableName, checkpoints.WholeTableEngineID, err, checkpoints.CheckpointStatusAlteredAutoInc)
Expand Down
9 changes: 7 additions & 2 deletions br/pkg/lightning/restore/tidb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"database/sql"
"fmt"
"math"
"strconv"
"strings"

Expand Down Expand Up @@ -373,8 +374,12 @@ 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))
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 {
Copy link
Contributor

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.

Copy link
Contributor Author

@buchuitoudegou buchuitoudegou Apr 21, 2022 •
edited
Loading

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 use alter table xxx force ...).

logger.Warn("auto_increment out of the maximum value TiDB supports", zap.Uint64("auto_increment", incr))
return nil
}
Copy link
Contributor

@sleepymole sleepymole Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to set auto_increment to math.MaxInt64 when incr > math.MaxInt64?

Copy link
Contributor Author

@buchuitoudegou buchuitoudegou Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The 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, 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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does incr > math.MaxInt64 means lightning has inserted rows whose id is greater than math.MaxInt64? Will this cause duplicate entry or data corruption?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

@buchuitoudegou buchuitoudegou Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Yes. Actually I'm looking at it #28776. This PR only solves the unexpected syntax error.

query := fmt.Sprintf("ALTER TABLE %s AUTO_INCREMENT=%d", tableName, incr)
task := logger.Begin(zap.InfoLevel, "alter table auto_increment")
err := g.ExecuteWithLog(ctx, query, "alter table auto_increment", logger)
Expand Down
4 changes: 4 additions & 0 deletions br/pkg/lightning/restore/tidb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package restore
import (
"context"
"database/sql"
"math"
"testing"

"github.com/DATA-DOG/go-sqlmock"
Expand Down Expand Up @@ -409,6 +410,9 @@ func TestAlterAutoInc(t *testing.T) {

err := AlterAutoIncrement(ctx, s.tiGlue.GetSQLExecutor(), "`db`.`table`", 12345)
require.NoError(t, err)

err = AlterAutoIncrement(ctx, s.tiGlue.GetSQLExecutor(), "`db`.`table`", uint64(math.MaxInt64+1))
require.NoError(t, err)
}

func TestAlterAutoRandom(t *testing.T) {
Expand Down