From 69a85099d02f633368a914bb17f93dd4a1103f2f Mon Sep 17 00:00:00 2001 From: tangenta Date: Wed, 25 Nov 2020 14:15:54 +0800 Subject: [PATCH 1/4] executor: fix split table with large integers --- ddl/backfilling.go | 3 ++- errno/errname.go | 2 +- executor/errors.go | 1 + executor/executor_test.go | 26 +++++++++++++++++--------- executor/split.go | 11 +++++++---- 5 files changed, 28 insertions(+), 15 deletions(-) diff --git a/ddl/backfilling.go b/ddl/backfilling.go index ab7922b4aaeba..364a15b27e931 100644 --- a/ddl/backfilling.go +++ b/ddl/backfilling.go @@ -335,7 +335,8 @@ func splitTableRanges(t table.PhysicalTable, store kv.Storage, startKey, endKey return nil, errors.Trace(err) } if len(ranges) == 0 { - return nil, errors.Trace(errInvalidSplitRegionRanges) + errMsg := fmt.Sprintf("cannot find region in range [%s, %s]", startKey.String(), endKey.String()) + return nil, errors.Trace(errInvalidSplitRegionRanges.GenWithStackByArgs(errMsg)) } return ranges, nil } diff --git a/errno/errname.go b/errno/errname.go index 3d6b299837e04..195a0c42db168 100644 --- a/errno/errname.go +++ b/errno/errname.go @@ -962,7 +962,7 @@ var MySQLErrName = map[uint16]*mysql.ErrMessage{ ErrUnknownTypeLength: mysql.Message("Unknown length for type %d", nil), ErrUnknownFractionLength: mysql.Message("Unknown length for type %d and fraction %d", nil), ErrInvalidDDLJobVersion: mysql.Message("Version %d of DDL job is greater than current one: %d", nil), - ErrInvalidSplitRegionRanges: mysql.Message("Failed to split region ranges", nil), + ErrInvalidSplitRegionRanges: mysql.Message("Failed to split region ranges: %s", nil), ErrReorgPanic: mysql.Message("Reorg worker panic", nil), ErrInvalidDDLState: mysql.Message("Invalid %s state: %v", nil), ErrCancelledDDLJob: mysql.Message("Cancelled DDL job", nil), diff --git a/executor/errors.go b/executor/errors.go index d648b56b81c81..5982f4bcb7aee 100644 --- a/executor/errors.go +++ b/executor/errors.go @@ -43,6 +43,7 @@ var ( ErrRoleNotGranted = dbterror.ClassPrivilege.NewStd(mysql.ErrRoleNotGranted) ErrDeadlock = dbterror.ClassExecutor.NewStd(mysql.ErrLockDeadlock) ErrQueryInterrupted = dbterror.ClassExecutor.NewStd(mysql.ErrQueryInterrupted) + ErrInvalidSplitRegionRanges = dbterror.ClassExecutor.NewStd(mysql.ErrInvalidSplitRegionRanges) ErrBRIEBackupFailed = dbterror.ClassExecutor.NewStd(mysql.ErrBRIEBackupFailed) ErrBRIERestoreFailed = dbterror.ClassExecutor.NewStd(mysql.ErrBRIERestoreFailed) diff --git a/executor/executor_test.go b/executor/executor_test.go index 546238b94d1fb..2e95d6aea9665 100644 --- a/executor/executor_test.go +++ b/executor/executor_test.go @@ -17,6 +17,7 @@ import ( "context" "flag" "fmt" + "github.com/pingcap/tidb/errno" "io/ioutil" "math" "net" @@ -4565,9 +4566,7 @@ func (s *testSplitTable) TestSplitRegion(c *C) { // Test for split index region. // Check min value is more than max value. tk.MustExec(`split table t index idx1 between (0) and (1000000000) regions 10`) - _, err = tk.Exec(`split table t index idx1 between (2,'a') and (1,'c') regions 10`) - c.Assert(err, NotNil) - c.Assert(err.Error(), Equals, "Split index `idx1` region lower value (2,a) should less than the upper value (1,c)") + tk.MustGetErrCode(`split table t index idx1 between (2,'a') and (1,'c') regions 10`, errno.ErrInvalidSplitRegionRanges) // Check min value is invalid. _, err = tk.Exec(`split table t index idx1 between () and (1) regions 10`) @@ -4597,9 +4596,7 @@ func (s *testSplitTable) TestSplitRegion(c *C) { // Test for split table region. tk.MustExec(`split table t between (0) and (1000000000) regions 10`) // Check the lower value is more than the upper value. - _, err = tk.Exec(`split table t between (2) and (1) regions 10`) - c.Assert(err, NotNil) - c.Assert(err.Error(), Equals, "Split table `t` region lower value 2 should less than the upper value 1") + tk.MustGetErrCode(`split table t between (2) and (1) regions 10`, errno.ErrInvalidSplitRegionRanges) // Check the lower value is invalid. _, err = tk.Exec(`split table t between () and (1) regions 10`) @@ -4627,9 +4624,7 @@ func (s *testSplitTable) TestSplitRegion(c *C) { c.Assert(err.Error(), Equals, "[types:1265]Incorrect value: 'aa' for column '_tidb_rowid'") // Test split table region step is too small. - _, err = tk.Exec(`split table t between (0) and (100) regions 10`) - c.Assert(err, NotNil) - c.Assert(err.Error(), Equals, "Split table `t` region step value should more than 1000, step 10 is invalid") + tk.MustGetErrCode(`split table t between (0) and (100) regions 10`, errno.ErrInvalidSplitRegionRanges) // Test split region by syntax. tk.MustExec(`split table t by (0),(1000),(1000000)`) @@ -4652,6 +4647,19 @@ func (s *testSplitTable) TestSplitRegion(c *C) { tk.MustQuery("split region for partition table t partition (p3,p4) between (100000000) and (1000000000) regions 5;").Check(testkit.Rows("8 1")) } +func (s *testSplitTable) TestSplitRegionEdgeCase(c *C) { + tk := testkit.NewTestKit(c, s.store) + tk.MustExec("use test;") + + tk.MustExec("drop table if exists t;") + tk.MustExec("create table t(a bigint(20) auto_increment primary key);") + tk.MustExec("split table t between (-9223372036854775808) and (9223372036854775807) regions 16;") + + tk.MustExec("drop table if exists t;") + tk.MustExec("create table t(a int(20) auto_increment primary key);") + tk.MustGetErrCode("split table t between (-9223372036854775808) and (9223372036854775807) regions 16;", errno.ErrDataOutOfRange) +} + func (s *testSplitTable) TestClusterIndexSplitTableIntegration(c *C) { tk := testkit.NewTestKit(c, s.store) tk.MustExec("drop database if exists test_cluster_index_index_split_table_integration;") diff --git a/executor/split.go b/executor/split.go index afc549a60d139..c9892d5b33e95 100644 --- a/executor/split.go +++ b/executor/split.go @@ -546,7 +546,8 @@ func (e *SplitTableRegionExec) calculateIntBoundValue() (lowerValue int64, step lowerRecordID := e.lower[0].GetUint64() upperRecordID := e.upper[0].GetUint64() if upperRecordID <= lowerRecordID { - return 0, 0, errors.Errorf("Split table `%s` region lower value %v should less than the upper value %v", e.tableInfo.Name, lowerRecordID, upperRecordID) + errMsg := fmt.Sprintf("lower value %v should less than the upper value %v", lowerRecordID, upperRecordID) + return 0, 0, ErrInvalidSplitRegionRanges.GenWithStackByArgs(errMsg) } step = int64((upperRecordID - lowerRecordID) / uint64(e.num)) lowerValue = int64(lowerRecordID) @@ -554,13 +555,15 @@ func (e *SplitTableRegionExec) calculateIntBoundValue() (lowerValue int64, step lowerRecordID := e.lower[0].GetInt64() upperRecordID := e.upper[0].GetInt64() if upperRecordID <= lowerRecordID { - return 0, 0, errors.Errorf("Split table `%s` region lower value %v should less than the upper value %v", e.tableInfo.Name, lowerRecordID, upperRecordID) + errMsg := fmt.Sprintf("lower value %v should less than the upper value %v", lowerRecordID, upperRecordID) + return 0, 0, ErrInvalidSplitRegionRanges.GenWithStackByArgs(errMsg) } - step = (upperRecordID - lowerRecordID) / int64(e.num) + step = int64(uint64(upperRecordID-lowerRecordID) / uint64(e.num)) lowerValue = lowerRecordID } if step < minRegionStepValue { - return 0, 0, errors.Errorf("Split table `%s` region step value should more than %v, step %v is invalid", e.tableInfo.Name, minRegionStepValue, step) + errMsg := fmt.Sprintf("the region size is too small, expected at least %d, but got %d", step, minRegionStepValue) + return 0, 0, ErrInvalidSplitRegionRanges.GenWithStackByArgs(errMsg) } return lowerValue, step, nil } From 47b9523545e35831931e354e4999a82bd7f0c69e Mon Sep 17 00:00:00 2001 From: tangenta Date: Wed, 25 Nov 2020 17:55:23 +0800 Subject: [PATCH 2/4] fix linter --- errors.toml | 5 +++++ executor/executor_test.go | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/errors.toml b/errors.toml index f756d02f8c80d..f1f8c4fa2a0e1 100644 --- a/errors.toml +++ b/errors.toml @@ -576,6 +576,11 @@ error = ''' Export failed: %s ''' +["executor:8212"] +error = ''' +Failed to split region ranges: %s +''' + ["expression:1139"] error = ''' Got error '%-.64s' from regexp diff --git a/executor/executor_test.go b/executor/executor_test.go index 2e95d6aea9665..c8fb789f82a92 100644 --- a/executor/executor_test.go +++ b/executor/executor_test.go @@ -17,7 +17,6 @@ import ( "context" "flag" "fmt" - "github.com/pingcap/tidb/errno" "io/ioutil" "math" "net" @@ -42,6 +41,7 @@ import ( "github.com/pingcap/tidb/ddl" "github.com/pingcap/tidb/domain" "github.com/pingcap/tidb/domain/infosync" + "github.com/pingcap/tidb/errno" "github.com/pingcap/tidb/executor" "github.com/pingcap/tidb/expression" "github.com/pingcap/tidb/infoschema" From 4bf4c820232f63e2442ba80263ba1b42d2f04df4 Mon Sep 17 00:00:00 2001 From: tangenta Date: Fri, 12 Mar 2021 18:17:15 +0800 Subject: [PATCH 3/4] fix integration test --- executor/split.go | 15 ++++++++++----- util/testkit/testkit.go | 2 +- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/executor/split.go b/executor/split.go index 00a226ee96912..1da3c5b5afa5b 100644 --- a/executor/split.go +++ b/executor/split.go @@ -22,7 +22,6 @@ import ( "time" "github.com/cznic/mathutil" - "github.com/pingcap/errors" "github.com/pingcap/kvproto/pkg/metapb" "github.com/pingcap/parser/model" "github.com/pingcap/parser/mysql" @@ -241,9 +240,13 @@ func (e *SplitIndexRegionExec) getSplitIdxPhysicalKeysFromBound(physicalID int64 lowerStr, err1 := datumSliceToString(e.lower) upperStr, err2 := datumSliceToString(e.upper) if err1 != nil || err2 != nil { - return nil, errors.Errorf("Split index `%v` region lower value %v should less than the upper value %v", e.indexInfo.Name, e.lower, e.upper) + errMsg := fmt.Sprintf("Split index `%v` region lower value %v should less than the upper value %v", + e.indexInfo.Name, e.lower, e.upper) + return nil, ErrInvalidSplitRegionRanges.GenWithStackByArgs(errMsg) } - return nil, errors.Errorf("Split index `%v` region lower value %v should less than the upper value %v", e.indexInfo.Name, lowerStr, upperStr) + errMsg := fmt.Sprintf("Split index `%v` region lower value %v should less than the upper value %v", + e.indexInfo.Name, lowerStr, upperStr) + return nil, ErrInvalidSplitRegionRanges.GenWithStackByArgs(errMsg) } return getValuesList(lowerIdxKey, upperIdxKey, e.num, keys), nil } @@ -602,11 +605,13 @@ func (e *SplitTableRegionExec) getSplitTablePhysicalKeysFromBound(physicalID int lowerStr, err1 := datumSliceToString(e.lower) upperStr, err2 := datumSliceToString(e.upper) if err1 != nil || err2 != nil { - return nil, errors.Errorf("Split table `%v` region lower value %v should less than the upper value %v", + errMsg := fmt.Sprintf("Split table `%v` region lower value %v should less than the upper value %v", e.tableInfo.Name.O, e.lower, e.upper) + return nil, ErrInvalidSplitRegionRanges.GenWithStackByArgs(errMsg) } - return nil, errors.Errorf("Split table `%v` region lower value %v should less than the upper value %v", + errMsg := fmt.Sprintf("Split table `%v` region lower value %v should less than the upper value %v", e.tableInfo.Name.O, lowerStr, upperStr) + return nil, ErrInvalidSplitRegionRanges.GenWithStackByArgs(errMsg) } low := tablecodec.EncodeRecordKey(recordPrefix, lowerHandle) up := tablecodec.EncodeRecordKey(recordPrefix, upperHandle) diff --git a/util/testkit/testkit.go b/util/testkit/testkit.go index e82ad885fe939..4992e28663b1a 100644 --- a/util/testkit/testkit.go +++ b/util/testkit/testkit.go @@ -329,7 +329,7 @@ func (tk *TestKit) MustGetErrCode(sql string, errCode int) { tk.c.Assert(err, check.NotNil) originErr := errors.Cause(err) tErr, ok := originErr.(*terror.Error) - tk.c.Assert(ok, check.IsTrue, check.Commentf("expect type 'terror.Error', but obtain '%T'", originErr)) + tk.c.Assert(ok, check.IsTrue, check.Commentf("expect type 'terror.Error', but obtain '%T': %v", originErr, originErr)) sqlErr := terror.ToSQLError(tErr) tk.c.Assert(int(sqlErr.Code), check.Equals, errCode, check.Commentf("Assertion failed, origin err:\n %v", sqlErr)) } From 45de1c35cde814e9bca29f663b6b78fee7165e13 Mon Sep 17 00:00:00 2001 From: tangenta Date: Mon, 15 Mar 2021 14:44:56 +0800 Subject: [PATCH 4/4] fix integration test --- executor/executor_test.go | 4 ++-- executor/split.go | 24 +++++++----------------- 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/executor/executor_test.go b/executor/executor_test.go index 50f12a10554ad..51cf16173365b 100644 --- a/executor/executor_test.go +++ b/executor/executor_test.go @@ -4851,9 +4851,9 @@ func (s *testSplitTable) TestClusterIndexSplitTableIntegration(c *C) { tk.MustGetErrMsg("split table t between ('aaa', 0.0) and (100.0, 'aaa') regions 10;", errMsg) // lower bound >= upper bound. - errMsg = "Split table `t` region lower value (aaa,0) should less than the upper value (aaa,0)" + errMsg = "[executor:8212]Failed to split region ranges: Split table `t` region lower value (aaa,0) should less than the upper value (aaa,0)" tk.MustGetErrMsg("split table t between ('aaa', 0.0) and ('aaa', 0.0) regions 10;", errMsg) - errMsg = "Split table `t` region lower value (bbb,0) should less than the upper value (aaa,0)" + errMsg = "[executor:8212]Failed to split region ranges: Split table `t` region lower value (bbb,0) should less than the upper value (aaa,0)" tk.MustGetErrMsg("split table t between ('bbb', 0.0) and ('aaa', 0.0) regions 10;", errMsg) // Exceed limit 1000. diff --git a/executor/split.go b/executor/split.go index 1da3c5b5afa5b..6cdd953772834 100644 --- a/executor/split.go +++ b/executor/split.go @@ -237,13 +237,8 @@ func (e *SplitIndexRegionExec) getSplitIdxPhysicalKeysFromBound(physicalID int64 } if bytes.Compare(lowerIdxKey, upperIdxKey) >= 0 { - lowerStr, err1 := datumSliceToString(e.lower) - upperStr, err2 := datumSliceToString(e.upper) - if err1 != nil || err2 != nil { - errMsg := fmt.Sprintf("Split index `%v` region lower value %v should less than the upper value %v", - e.indexInfo.Name, e.lower, e.upper) - return nil, ErrInvalidSplitRegionRanges.GenWithStackByArgs(errMsg) - } + lowerStr := datumSliceToString(e.lower) + upperStr := datumSliceToString(e.upper) errMsg := fmt.Sprintf("Split index `%v` region lower value %v should less than the upper value %v", e.indexInfo.Name, lowerStr, upperStr) return nil, ErrInvalidSplitRegionRanges.GenWithStackByArgs(errMsg) @@ -307,12 +302,12 @@ func getUint64FromBytes(bs []byte, pad byte) uint64 { return binary.BigEndian.Uint64(buf) } -func datumSliceToString(ds []types.Datum) (string, error) { +func datumSliceToString(ds []types.Datum) string { str := "(" for i, d := range ds { s, err := d.ToString() if err != nil { - return str, err + return fmt.Sprintf("%v", ds) } if i > 0 { str += "," @@ -320,7 +315,7 @@ func datumSliceToString(ds []types.Datum) (string, error) { str += s } str += ")" - return str, nil + return str } // SplitTableRegionExec represents a split table regions executor. @@ -602,13 +597,8 @@ func (e *SplitTableRegionExec) getSplitTablePhysicalKeysFromBound(physicalID int return nil, err } if lowerHandle.Compare(upperHandle) >= 0 { - lowerStr, err1 := datumSliceToString(e.lower) - upperStr, err2 := datumSliceToString(e.upper) - if err1 != nil || err2 != nil { - errMsg := fmt.Sprintf("Split table `%v` region lower value %v should less than the upper value %v", - e.tableInfo.Name.O, e.lower, e.upper) - return nil, ErrInvalidSplitRegionRanges.GenWithStackByArgs(errMsg) - } + lowerStr := datumSliceToString(e.lower) + upperStr := datumSliceToString(e.upper) errMsg := fmt.Sprintf("Split table `%v` region lower value %v should less than the upper value %v", e.tableInfo.Name.O, lowerStr, upperStr) return nil, ErrInvalidSplitRegionRanges.GenWithStackByArgs(errMsg)