From deb3b3bfb443e713d8b8886a9d77eae09ffddbb6 Mon Sep 17 00:00:00 2001 From: Chunzhu Li Date: Thu, 2 Feb 2023 14:54:07 +0800 Subject: [PATCH 1/8] fix check has tikv problem --- dumpling/export/dump.go | 8 +++-- dumpling/export/dump_test.go | 57 ++++++++++++++++++++++++++++++++++++ dumpling/export/sql.go | 14 ++++++--- 3 files changed, 73 insertions(+), 6 deletions(-) diff --git a/dumpling/export/dump.go b/dumpling/export/dump.go index b8e46f595c4e9..297047e92232c 100644 --- a/dumpling/export/dump.go +++ b/dumpling/export/dump.go @@ -1564,7 +1564,10 @@ func setSessionParam(d *Dumper) error { if si.ServerType == version.ServerTypeTiDB && conf.TiDBMemQuotaQuery != UnspecifiedSize { sessionParam[TiDBMemQuotaQueryName] = conf.TiDBMemQuotaQuery } - var err error + var ( + err error + ignorableSnapshot = false + ) if snapshot != "" { if si.ServerType != version.ServerTypeTiDB { return errors.New("snapshot consistency is not supported for this server") @@ -1573,13 +1576,14 @@ func setSessionParam(d *Dumper) error { conf.ServerInfo.HasTiKV, err = CheckTiDBWithTiKV(pool) if err != nil { d.L().Info("cannot check whether TiDB has TiKV, will apply tidb_snapshot by default. This won't affect dump process", log.ShortError(err)) + ignorableSnapshot = true } if conf.ServerInfo.HasTiKV { sessionParam["tidb_snapshot"] = snapshot } } } - if d.dbHandle, err = resetDBWithSessionParams(d.tctx, pool, conf.GetDriverConfig(""), conf.SessionParams); err != nil { + if d.dbHandle, err = resetDBWithSessionParams(d.tctx, pool, conf.GetDriverConfig(""), conf.SessionParams, ignorableSnapshot); err != nil { return errors.Trace(err) } return nil diff --git a/dumpling/export/dump_test.go b/dumpling/export/dump_test.go index 7d621857f3a85..3f21f967743c2 100644 --- a/dumpling/export/dump_test.go +++ b/dumpling/export/dump_test.go @@ -10,7 +10,9 @@ import ( "github.com/DATA-DOG/go-sqlmock" "github.com/coreos/go-semver/semver" + "github.com/go-sql-driver/mysql" "github.com/pingcap/errors" + "github.com/pingcap/failpoint" "github.com/pingcap/tidb/br/pkg/version" tcontext "github.com/pingcap/tidb/dumpling/context" "github.com/pingcap/tidb/parser" @@ -289,3 +291,58 @@ func TestSetDefaultSessionParams(t *testing.T) { require.Equal(t, testCase.expectedParams, testCase.sessionParams) } } + +func TestSetSessionParams(t *testing.T) { + // case 1: fail to set tidb_snapshot when fail to checkTiDBHasKV, shouldn't return an error + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer func() { + require.NoError(t, db.Close()) + }() + + mock.ExpectQuery("SELECT @@tidb_config"). + WillReturnError(errors.New("mock error")) + mock.ExpectQuery("SELECT COUNT\\(1\\) as c FROM MYSQL.TiDB WHERE VARIABLE_NAME='tikv_gc_safe_point'"). + WillReturnError(errors.New("mock error")) + tikvErr := &mysql.MySQLError{ + Number: 1105, + Message: "can not get 'tikv_gc_safe_point'", + } + mock.ExpectExec("SET SESSION tidb_snapshot"). + WillReturnError(tikvErr) + + require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/dumpling/export/SkipResetDB", "return(true)")) + defer failpoint.Disable("github.com/pingcap/tidb/dumpling/export/SkipResetDB=return(true)") + + tctx, cancel := tcontext.Background().WithLogger(appLogger).WithCancel() + defer cancel() + + conf := DefaultConfig() + conf.ServerInfo = version.ServerInfo{ + ServerType: version.ServerTypeTiDB, + HasTiKV: false, + } + conf.Snapshot = "439153276059648000" + conf.Consistency = ConsistencyTypeSnapshot + d := &Dumper{ + tctx: tctx, + conf: conf, + cancelCtx: cancel, + dbHandle: db, + } + err = setSessionParam(d) + require.NoError(t, err) + + // case 2: fail to set tidb_snapshot when succeed to checkTiDBHasKV, should return an error + mock.ExpectQuery("SELECT @@tidb_config"). + WillReturnError(errors.New("mock error")) + mock.ExpectQuery("SELECT COUNT\\(1\\) as c FROM MYSQL.TiDB WHERE VARIABLE_NAME='tikv_gc_safe_point'"). + WillReturnRows(sqlmock.NewRows([]string{"COUNT(1)"}).AddRow(1)) + mock.ExpectExec("SET SESSION tidb_snapshot"). + WillReturnError(tikvErr) + mock.ExpectClose() + mock.ExpectClose() + + err = setSessionParam(d) + require.True(t, errors.Cause(err) == tikvErr) +} diff --git a/dumpling/export/sql.go b/dumpling/export/sql.go index 837bec568b9a7..f470653ffdd96 100644 --- a/dumpling/export/sql.go +++ b/dumpling/export/sql.go @@ -827,13 +827,16 @@ func getSnapshot(db *sql.Conn) (string, error) { return str[snapshotFieldIndex], nil } -func isUnknownSystemVariableErr(err error) bool { - return strings.Contains(err.Error(), "Unknown system variable") +func isUnknownSystemVariableErr(err error, ignorableSnapshot bool) bool { + errStr := err.Error() + return strings.Contains(err.Error(), "Unknown system variable") || + (ignorableSnapshot && strings.Contains(errStr, "can not get 'tikv_gc_safe_point'")) } // resetDBWithSessionParams will return a new sql.DB as a replacement for input `db` with new session parameters. // If returned error is nil, the input `db` will be closed. -func resetDBWithSessionParams(tctx *tcontext.Context, db *sql.DB, cfg *mysql.Config, params map[string]interface{}) (*sql.DB, error) { +func resetDBWithSessionParams(tctx *tcontext.Context, db *sql.DB, cfg *mysql.Config, + params map[string]interface{}, ignorableSnapshot bool) (*sql.DB, error) { support := make(map[string]interface{}) for k, v := range params { var pv interface{} @@ -851,7 +854,7 @@ func resetDBWithSessionParams(tctx *tcontext.Context, db *sql.DB, cfg *mysql.Con s := fmt.Sprintf("SET SESSION %s = ?", k) _, err := db.ExecContext(tctx, s, pv) if err != nil { - if isUnknownSystemVariableErr(err) { + if isUnknownSystemVariableErr(err, ignorableSnapshot) { tctx.L().Info("session variable is not supported by db", zap.String("variable", k), zap.Reflect("value", v)) continue } @@ -876,6 +879,9 @@ func resetDBWithSessionParams(tctx *tcontext.Context, db *sql.DB, cfg *mysql.Con } cfg.Params[k] = s } + failpoint.Inject("SkipResetDB", func(_ failpoint.Value) { + failpoint.Return(db, nil) + }) db.Close() c, err := mysql.NewConnector(cfg) From 3fb858c8c4196bc87f7b2476e12c810d25eb6fdc Mon Sep 17 00:00:00 2001 From: Chunzhu Li Date: Thu, 2 Feb 2023 15:48:28 +0800 Subject: [PATCH 2/8] update bazel --- dumpling/export/BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/dumpling/export/BUILD.bazel b/dumpling/export/BUILD.bazel index cf4d938de6042..f47a49249adee 100644 --- a/dumpling/export/BUILD.bazel +++ b/dumpling/export/BUILD.bazel @@ -107,6 +107,7 @@ go_test( "@com_github_data_dog_go_sqlmock//:go-sqlmock", "@com_github_go_sql_driver_mysql//:mysql", "@com_github_pingcap_errors//:errors", + "@com_github_pingcap_failpoint//:failpoint", "@com_github_prometheus_client_golang//prometheus/collectors", "@com_github_stretchr_testify//require", "@org_golang_x_sync//errgroup", From 8a9612b2883f3d061ff11031798181b4f17b6397 Mon Sep 17 00:00:00 2001 From: Chunzhu Li Date: Mon, 6 Feb 2023 16:00:37 +0800 Subject: [PATCH 3/8] tmp --- dumpling/export/consistency.go | 14 ++++++++++---- dumpling/export/dump.go | 4 +++- dumpling/export/sql.go | 1 + 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/dumpling/export/consistency.go b/dumpling/export/consistency.go index b46259bc748b9..9a5799bff3636 100644 --- a/dumpling/export/consistency.go +++ b/dumpling/export/consistency.go @@ -50,9 +50,10 @@ func NewConsistencyController(ctx context.Context, conf *Config, session *sql.DB if conf.ServerInfo.ServerType != version.ServerTypeTiDB { return nil, errors.New("snapshot consistency is not supported for this server") } - return &ConsistencyNone{}, nil + _, ok := conf.SessionParams["tidb_snapshot"] + return &ConsistencyNone{canRetry: ok}, nil case ConsistencyTypeNone: - return &ConsistencyNone{}, nil + return &ConsistencyNone{canRetry: true}, nil default: return nil, errors.Errorf("invalid consistency option %s", conf.Consistency) } @@ -66,7 +67,9 @@ type ConsistencyController interface { } // ConsistencyNone dumps without adding locks, which cannot guarantee consistency -type ConsistencyNone struct{} +type ConsistencyNone struct { + canRetry bool +} // Setup implements ConsistencyController.Setup func (*ConsistencyNone) Setup(_ *tcontext.Context) error { @@ -79,7 +82,10 @@ func (*ConsistencyNone) TearDown(_ context.Context) error { } // PingContext implements ConsistencyController.PingContext -func (*ConsistencyNone) PingContext(_ context.Context) error { +func (c *ConsistencyNone) PingContext(_ context.Context) error { + if !c.canRetry { + return errors.New("connection's snapshot is not set up successfully, can't retry") + } return nil } diff --git a/dumpling/export/dump.go b/dumpling/export/dump.go index 297047e92232c..1ca1f35d1a5c9 100644 --- a/dumpling/export/dump.go +++ b/dumpling/export/dump.go @@ -65,6 +65,7 @@ type Dumper struct { selectTiDBTableRegionFunc func(tctx *tcontext.Context, conn *BaseConn, meta TableMeta) (pkFields []string, pkVals [][]string, err error) totalTables int64 charsetAndDefaultCollationMap map[string]string + specifiedSnapshot bool speedRecorder *SpeedRecorder } @@ -1468,6 +1469,7 @@ func tidbGetSnapshot(d *Dumper) error { consistency := conf.Consistency pool, tctx := d.dbHandle, d.tctx snapshotConsistency := consistency == "snapshot" + d.specifiedSnapshot = conf.Snapshot != "" if conf.Snapshot == "" && (doPdGC || snapshotConsistency) { conn, err := pool.Conn(tctx) if err != nil { @@ -1576,7 +1578,7 @@ func setSessionParam(d *Dumper) error { conf.ServerInfo.HasTiKV, err = CheckTiDBWithTiKV(pool) if err != nil { d.L().Info("cannot check whether TiDB has TiKV, will apply tidb_snapshot by default. This won't affect dump process", log.ShortError(err)) - ignorableSnapshot = true + ignorableSnapshot = !d.specifiedSnapshot // if snapshot is specified by users, we can't ignore set snapshot error } if conf.ServerInfo.HasTiKV { sessionParam["tidb_snapshot"] = snapshot diff --git a/dumpling/export/sql.go b/dumpling/export/sql.go index f470653ffdd96..c68dcff762c0e 100644 --- a/dumpling/export/sql.go +++ b/dumpling/export/sql.go @@ -856,6 +856,7 @@ func resetDBWithSessionParams(tctx *tcontext.Context, db *sql.DB, cfg *mysql.Con if err != nil { if isUnknownSystemVariableErr(err, ignorableSnapshot) { tctx.L().Info("session variable is not supported by db", zap.String("variable", k), zap.Reflect("value", v)) + delete(params, k) continue } return nil, errors.Trace(err) From da0f531ceb5cc92ae9ce66ae6fda8687836220cf Mon Sep 17 00:00:00 2001 From: Chunzhu Li Date: Wed, 8 Feb 2023 17:30:31 +0800 Subject: [PATCH 4/8] Revert "update bazel" This reverts commit 3fb858c8c4196bc87f7b2476e12c810d25eb6fdc. --- dumpling/export/BUILD.bazel | 1 - 1 file changed, 1 deletion(-) diff --git a/dumpling/export/BUILD.bazel b/dumpling/export/BUILD.bazel index f47a49249adee..cf4d938de6042 100644 --- a/dumpling/export/BUILD.bazel +++ b/dumpling/export/BUILD.bazel @@ -107,7 +107,6 @@ go_test( "@com_github_data_dog_go_sqlmock//:go-sqlmock", "@com_github_go_sql_driver_mysql//:mysql", "@com_github_pingcap_errors//:errors", - "@com_github_pingcap_failpoint//:failpoint", "@com_github_prometheus_client_golang//prometheus/collectors", "@com_github_stretchr_testify//require", "@org_golang_x_sync//errgroup", From 1f5d1a528bb4fabb445a4c24d1618aca04c20532 Mon Sep 17 00:00:00 2001 From: Chunzhu Li Date: Wed, 8 Feb 2023 17:33:51 +0800 Subject: [PATCH 5/8] revert changes --- dumpling/export/dump.go | 8 ++--- dumpling/export/dump_test.go | 57 ------------------------------------ dumpling/export/sql.go | 14 +++------ 3 files changed, 6 insertions(+), 73 deletions(-) diff --git a/dumpling/export/dump.go b/dumpling/export/dump.go index 1ca1f35d1a5c9..d792abe8c359c 100644 --- a/dumpling/export/dump.go +++ b/dumpling/export/dump.go @@ -1566,10 +1566,7 @@ func setSessionParam(d *Dumper) error { if si.ServerType == version.ServerTypeTiDB && conf.TiDBMemQuotaQuery != UnspecifiedSize { sessionParam[TiDBMemQuotaQueryName] = conf.TiDBMemQuotaQuery } - var ( - err error - ignorableSnapshot = false - ) + var err error if snapshot != "" { if si.ServerType != version.ServerTypeTiDB { return errors.New("snapshot consistency is not supported for this server") @@ -1578,14 +1575,13 @@ func setSessionParam(d *Dumper) error { conf.ServerInfo.HasTiKV, err = CheckTiDBWithTiKV(pool) if err != nil { d.L().Info("cannot check whether TiDB has TiKV, will apply tidb_snapshot by default. This won't affect dump process", log.ShortError(err)) - ignorableSnapshot = !d.specifiedSnapshot // if snapshot is specified by users, we can't ignore set snapshot error } if conf.ServerInfo.HasTiKV { sessionParam["tidb_snapshot"] = snapshot } } } - if d.dbHandle, err = resetDBWithSessionParams(d.tctx, pool, conf.GetDriverConfig(""), conf.SessionParams, ignorableSnapshot); err != nil { + if d.dbHandle, err = resetDBWithSessionParams(d.tctx, pool, conf.GetDriverConfig(""), conf.SessionParams); err != nil { return errors.Trace(err) } return nil diff --git a/dumpling/export/dump_test.go b/dumpling/export/dump_test.go index 3f21f967743c2..7d621857f3a85 100644 --- a/dumpling/export/dump_test.go +++ b/dumpling/export/dump_test.go @@ -10,9 +10,7 @@ import ( "github.com/DATA-DOG/go-sqlmock" "github.com/coreos/go-semver/semver" - "github.com/go-sql-driver/mysql" "github.com/pingcap/errors" - "github.com/pingcap/failpoint" "github.com/pingcap/tidb/br/pkg/version" tcontext "github.com/pingcap/tidb/dumpling/context" "github.com/pingcap/tidb/parser" @@ -291,58 +289,3 @@ func TestSetDefaultSessionParams(t *testing.T) { require.Equal(t, testCase.expectedParams, testCase.sessionParams) } } - -func TestSetSessionParams(t *testing.T) { - // case 1: fail to set tidb_snapshot when fail to checkTiDBHasKV, shouldn't return an error - db, mock, err := sqlmock.New() - require.NoError(t, err) - defer func() { - require.NoError(t, db.Close()) - }() - - mock.ExpectQuery("SELECT @@tidb_config"). - WillReturnError(errors.New("mock error")) - mock.ExpectQuery("SELECT COUNT\\(1\\) as c FROM MYSQL.TiDB WHERE VARIABLE_NAME='tikv_gc_safe_point'"). - WillReturnError(errors.New("mock error")) - tikvErr := &mysql.MySQLError{ - Number: 1105, - Message: "can not get 'tikv_gc_safe_point'", - } - mock.ExpectExec("SET SESSION tidb_snapshot"). - WillReturnError(tikvErr) - - require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/dumpling/export/SkipResetDB", "return(true)")) - defer failpoint.Disable("github.com/pingcap/tidb/dumpling/export/SkipResetDB=return(true)") - - tctx, cancel := tcontext.Background().WithLogger(appLogger).WithCancel() - defer cancel() - - conf := DefaultConfig() - conf.ServerInfo = version.ServerInfo{ - ServerType: version.ServerTypeTiDB, - HasTiKV: false, - } - conf.Snapshot = "439153276059648000" - conf.Consistency = ConsistencyTypeSnapshot - d := &Dumper{ - tctx: tctx, - conf: conf, - cancelCtx: cancel, - dbHandle: db, - } - err = setSessionParam(d) - require.NoError(t, err) - - // case 2: fail to set tidb_snapshot when succeed to checkTiDBHasKV, should return an error - mock.ExpectQuery("SELECT @@tidb_config"). - WillReturnError(errors.New("mock error")) - mock.ExpectQuery("SELECT COUNT\\(1\\) as c FROM MYSQL.TiDB WHERE VARIABLE_NAME='tikv_gc_safe_point'"). - WillReturnRows(sqlmock.NewRows([]string{"COUNT(1)"}).AddRow(1)) - mock.ExpectExec("SET SESSION tidb_snapshot"). - WillReturnError(tikvErr) - mock.ExpectClose() - mock.ExpectClose() - - err = setSessionParam(d) - require.True(t, errors.Cause(err) == tikvErr) -} diff --git a/dumpling/export/sql.go b/dumpling/export/sql.go index c68dcff762c0e..0b932ec821014 100644 --- a/dumpling/export/sql.go +++ b/dumpling/export/sql.go @@ -827,16 +827,13 @@ func getSnapshot(db *sql.Conn) (string, error) { return str[snapshotFieldIndex], nil } -func isUnknownSystemVariableErr(err error, ignorableSnapshot bool) bool { - errStr := err.Error() - return strings.Contains(err.Error(), "Unknown system variable") || - (ignorableSnapshot && strings.Contains(errStr, "can not get 'tikv_gc_safe_point'")) +func isUnknownSystemVariableErr(err error) bool { + return strings.Contains(err.Error(), "Unknown system variable") } // resetDBWithSessionParams will return a new sql.DB as a replacement for input `db` with new session parameters. // If returned error is nil, the input `db` will be closed. -func resetDBWithSessionParams(tctx *tcontext.Context, db *sql.DB, cfg *mysql.Config, - params map[string]interface{}, ignorableSnapshot bool) (*sql.DB, error) { +func resetDBWithSessionParams(tctx *tcontext.Context, db *sql.DB, cfg *mysql.Config, params map[string]interface{}) (*sql.DB, error) { support := make(map[string]interface{}) for k, v := range params { var pv interface{} @@ -854,7 +851,7 @@ func resetDBWithSessionParams(tctx *tcontext.Context, db *sql.DB, cfg *mysql.Con s := fmt.Sprintf("SET SESSION %s = ?", k) _, err := db.ExecContext(tctx, s, pv) if err != nil { - if isUnknownSystemVariableErr(err, ignorableSnapshot) { + if isUnknownSystemVariableErr(err) { tctx.L().Info("session variable is not supported by db", zap.String("variable", k), zap.Reflect("value", v)) delete(params, k) continue @@ -880,9 +877,6 @@ func resetDBWithSessionParams(tctx *tcontext.Context, db *sql.DB, cfg *mysql.Con } cfg.Params[k] = s } - failpoint.Inject("SkipResetDB", func(_ failpoint.Value) { - failpoint.Return(db, nil) - }) db.Close() c, err := mysql.NewConnector(cfg) From 98656951d174bebcabbd06f1cff2f19304d9b301 Mon Sep 17 00:00:00 2001 From: Chunzhu Li Date: Wed, 8 Feb 2023 17:39:05 +0800 Subject: [PATCH 6/8] revert changes --- dumpling/export/consistency.go | 14 ++++---------- dumpling/export/dump.go | 2 -- dumpling/export/sql.go | 1 - 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/dumpling/export/consistency.go b/dumpling/export/consistency.go index 9a5799bff3636..b46259bc748b9 100644 --- a/dumpling/export/consistency.go +++ b/dumpling/export/consistency.go @@ -50,10 +50,9 @@ func NewConsistencyController(ctx context.Context, conf *Config, session *sql.DB if conf.ServerInfo.ServerType != version.ServerTypeTiDB { return nil, errors.New("snapshot consistency is not supported for this server") } - _, ok := conf.SessionParams["tidb_snapshot"] - return &ConsistencyNone{canRetry: ok}, nil + return &ConsistencyNone{}, nil case ConsistencyTypeNone: - return &ConsistencyNone{canRetry: true}, nil + return &ConsistencyNone{}, nil default: return nil, errors.Errorf("invalid consistency option %s", conf.Consistency) } @@ -67,9 +66,7 @@ type ConsistencyController interface { } // ConsistencyNone dumps without adding locks, which cannot guarantee consistency -type ConsistencyNone struct { - canRetry bool -} +type ConsistencyNone struct{} // Setup implements ConsistencyController.Setup func (*ConsistencyNone) Setup(_ *tcontext.Context) error { @@ -82,10 +79,7 @@ func (*ConsistencyNone) TearDown(_ context.Context) error { } // PingContext implements ConsistencyController.PingContext -func (c *ConsistencyNone) PingContext(_ context.Context) error { - if !c.canRetry { - return errors.New("connection's snapshot is not set up successfully, can't retry") - } +func (*ConsistencyNone) PingContext(_ context.Context) error { return nil } diff --git a/dumpling/export/dump.go b/dumpling/export/dump.go index d792abe8c359c..b8e46f595c4e9 100644 --- a/dumpling/export/dump.go +++ b/dumpling/export/dump.go @@ -65,7 +65,6 @@ type Dumper struct { selectTiDBTableRegionFunc func(tctx *tcontext.Context, conn *BaseConn, meta TableMeta) (pkFields []string, pkVals [][]string, err error) totalTables int64 charsetAndDefaultCollationMap map[string]string - specifiedSnapshot bool speedRecorder *SpeedRecorder } @@ -1469,7 +1468,6 @@ func tidbGetSnapshot(d *Dumper) error { consistency := conf.Consistency pool, tctx := d.dbHandle, d.tctx snapshotConsistency := consistency == "snapshot" - d.specifiedSnapshot = conf.Snapshot != "" if conf.Snapshot == "" && (doPdGC || snapshotConsistency) { conn, err := pool.Conn(tctx) if err != nil { diff --git a/dumpling/export/sql.go b/dumpling/export/sql.go index 0b932ec821014..837bec568b9a7 100644 --- a/dumpling/export/sql.go +++ b/dumpling/export/sql.go @@ -853,7 +853,6 @@ func resetDBWithSessionParams(tctx *tcontext.Context, db *sql.DB, cfg *mysql.Con if err != nil { if isUnknownSystemVariableErr(err) { tctx.L().Info("session variable is not supported by db", zap.String("variable", k), zap.Reflect("value", v)) - delete(params, k) continue } return nil, errors.Trace(err) From 7ed18cd29cdf0051b66ed40cfc13f6cc0efb57ad Mon Sep 17 00:00:00 2001 From: Chunzhu Li Date: Wed, 8 Feb 2023 18:14:54 +0800 Subject: [PATCH 7/8] address comment --- dumpling/export/dump.go | 2 +- dumpling/export/dump_test.go | 63 ++++++++++++++++++++++++++++++++++++ dumpling/export/sql.go | 8 ++++- 3 files changed, 71 insertions(+), 2 deletions(-) diff --git a/dumpling/export/dump.go b/dumpling/export/dump.go index b8e46f595c4e9..82e093a213b6b 100644 --- a/dumpling/export/dump.go +++ b/dumpling/export/dump.go @@ -1575,7 +1575,7 @@ func setSessionParam(d *Dumper) error { d.L().Info("cannot check whether TiDB has TiKV, will apply tidb_snapshot by default. This won't affect dump process", log.ShortError(err)) } if conf.ServerInfo.HasTiKV { - sessionParam["tidb_snapshot"] = snapshot + sessionParam[snapshotVar] = snapshot } } } diff --git a/dumpling/export/dump_test.go b/dumpling/export/dump_test.go index 7d621857f3a85..f77171b1cfa8c 100644 --- a/dumpling/export/dump_test.go +++ b/dumpling/export/dump_test.go @@ -10,7 +10,9 @@ import ( "github.com/DATA-DOG/go-sqlmock" "github.com/coreos/go-semver/semver" + "github.com/go-sql-driver/mysql" "github.com/pingcap/errors" + "github.com/pingcap/failpoint" "github.com/pingcap/tidb/br/pkg/version" tcontext "github.com/pingcap/tidb/dumpling/context" "github.com/pingcap/tidb/parser" @@ -289,3 +291,64 @@ func TestSetDefaultSessionParams(t *testing.T) { require.Equal(t, testCase.expectedParams, testCase.sessionParams) } } + +func TestSetSessionParams(t *testing.T) { + // case 1: fail to set tidb_snapshot, should return error with hint + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer func() { + require.NoError(t, db.Close()) + }() + + mock.ExpectQuery("SELECT @@tidb_config"). + WillReturnError(errors.New("mock error")) + mock.ExpectQuery("SELECT COUNT\\(1\\) as c FROM MYSQL.TiDB WHERE VARIABLE_NAME='tikv_gc_safe_point'"). + WillReturnError(errors.New("mock error")) + tikvErr := &mysql.MySQLError{ + Number: 1105, + Message: "can not get 'tikv_gc_safe_point'", + } + mock.ExpectExec("SET SESSION tidb_snapshot"). + WillReturnError(tikvErr) + + require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/dumpling/export/SkipResetDB", "return(true)")) + defer failpoint.Disable("github.com/pingcap/tidb/dumpling/export/SkipResetDB=return(true)") + + tctx, cancel := tcontext.Background().WithLogger(appLogger).WithCancel() + defer cancel() + + conf := DefaultConfig() + conf.ServerInfo = version.ServerInfo{ + ServerType: version.ServerTypeTiDB, + HasTiKV: false, + } + conf.Snapshot = "439153276059648000" + conf.Consistency = ConsistencyTypeSnapshot + d := &Dumper{ + tctx: tctx, + conf: conf, + cancelCtx: cancel, + dbHandle: db, + } + err = setSessionParam(d) + require.ErrorContains(t, err, "consistency=none") + + // case 2: fail to set other + conf.ServerInfo = version.ServerInfo{ + ServerType: version.ServerTypeMySQL, + HasTiKV: false, + } + conf.Snapshot = "" + conf.Consistency = ConsistencyTypeFlush + conf.SessionParams = map[string]interface{}{ + "mock": "UTC", + } + d.dbHandle = db + mock.ExpectExec("SET SESSION mock"). + WillReturnError(errors.New("Unknown system variable mock")) + mock.ExpectClose() + mock.ExpectClose() + + err = setSessionParam(d) + require.NoError(t, err) +} diff --git a/dumpling/export/sql.go b/dumpling/export/sql.go index 837bec568b9a7..60d14ac49e14c 100644 --- a/dumpling/export/sql.go +++ b/dumpling/export/sql.go @@ -29,6 +29,7 @@ import ( const ( orderByTiDBRowID = "ORDER BY `_tidb_rowid`" + snapshotVar = "tidb_snapshot" ) type listTableType int @@ -851,7 +852,9 @@ func resetDBWithSessionParams(tctx *tcontext.Context, db *sql.DB, cfg *mysql.Con s := fmt.Sprintf("SET SESSION %s = ?", k) _, err := db.ExecContext(tctx, s, pv) if err != nil { - if isUnknownSystemVariableErr(err) { + if k == snapshotVar { + err = errors.Annotate(err, "fail to set snapshot for tidb, please set --consistency=none/--consistency=lock or fix snapshot problem") + } else if isUnknownSystemVariableErr(err) { tctx.L().Info("session variable is not supported by db", zap.String("variable", k), zap.Reflect("value", v)) continue } @@ -876,6 +879,9 @@ func resetDBWithSessionParams(tctx *tcontext.Context, db *sql.DB, cfg *mysql.Con } cfg.Params[k] = s } + failpoint.Inject("SkipResetDB", func(_ failpoint.Value) { + failpoint.Return(db, nil) + }) db.Close() c, err := mysql.NewConnector(cfg) From f64b74fe5dadcffa329a43681386aac1b9e90bc4 Mon Sep 17 00:00:00 2001 From: Chunzhu Li Date: Wed, 8 Feb 2023 19:37:03 +0800 Subject: [PATCH 8/8] fix bazel --- dumpling/export/BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/dumpling/export/BUILD.bazel b/dumpling/export/BUILD.bazel index cf4d938de6042..f47a49249adee 100644 --- a/dumpling/export/BUILD.bazel +++ b/dumpling/export/BUILD.bazel @@ -107,6 +107,7 @@ go_test( "@com_github_data_dog_go_sqlmock//:go-sqlmock", "@com_github_go_sql_driver_mysql//:mysql", "@com_github_pingcap_errors//:errors", + "@com_github_pingcap_failpoint//:failpoint", "@com_github_prometheus_client_golang//prometheus/collectors", "@com_github_stretchr_testify//require", "@org_golang_x_sync//errgroup",