Repository: pingcap/dumpling
Pull request #188 (2020-11-05 15:17:02)

fix tidb master status bug
<!--
Thank you for contributing to Dumpling! Please read the [CONTRIBUTING](https://github.com/pingcap/dumpling/blob/master/CONTRIBUTING.md) document **BEFORE** filing this PR.

If you want to open the **Challenge Program** pull request, please use the following template:
https://raw.githubusercontent.com/pingcap/.github/master/.github/PULL_REQUEST_TEMPLATE/challenge-program.md

You can use it with query parameters: https://github.com/pingcap/dumpling/compare/master...${you branch}?template=challenge-program.md
-->

### What problem does this PR solve? <!--add issue link with summary if exists-->
1. For TiDB, metadata will use TSO from `show master status`, which is the current TiDB TSO. If the user sets `--snapshot`, it will be different from that.
2. For TiDB not setting `--snapshot`, we will get a snapshot TSO at first to set PD gc-ttl. After that, we will use `show master status` to get another snapshot TSO, which appears to be different from the first one.

### What is changed and how it works?
Use snapshot TSO fetched at first expect for consistency lock for TiDB.

### Check List <!--REMOVE the items that are not applicable-->

Tests <!-- At least one of them must be included. -->

 - Unit test
 - Integration test

Related changes

 - Need to cherry-pick to the release branch
 
### Release note

<!-- bugfixes or new feature need a release note, must in the form of a list, such as

- support -T/--tables-list argument

or if no need to be included in the release note, just add the following line

- No release note
-->
- fix the problem that TiDB  pos in metadata is not accurate


Patch:
diff --git a/v4/export/dump.go b/v4/export/dump.go
index b988b46..b773277 100755
--- a/v4/export/dump.go
+++ b/v4/export/dump.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"database/sql"
 	"errors"
+	"strconv"
 	"strings"
 	"time"
 
@@ -45,8 +46,11 @@ func Dump(pCtx context.Context, conf *Config) (err error) {
 	ctx, cancel := context.WithCancel(pCtx)
 	defer cancel()
 
-	var doPdGC bool
-	var pdClient pd.Client
+	var (
+		doPdGC     bool
+		pdClient   pd.Client
+		snapshotTS uint64
+	)
 	if conf.ServerInfo.ServerType == ServerTypeTiDB {
 		if conf.ServerInfo.ServerVersion.Compare(*gcSafePointVersion) >= 0 {
 			pdAddrs, err := GetPdAddrs(pool)
@@ -70,39 +74,45 @@ func Dump(pCtx context.Context, conf *Config) (err error) {
 		delete(conf.SessionParams, TiDBMemQuotaQueryName)
 	}
 
-	if conf.Snapshot == "" && (doPdGC || conf.Consistency == "snapshot") {
+	snapshot := conf.Snapshot
+	if snapshot == "" && (doPdGC || conf.Consistency == "snapshot") {
 		conn, err := pool.Conn(ctx)
 		if err != nil {
 			conn.Close()
 			return withStack(err)
 		}
-		conf.Snapshot, err = getSnapshot(conn)
+		snapshot, err = getSnapshot(conn)
 		conn.Close()
 		if err != nil {
 			return err
 		}
 	}
 
-	if conf.Snapshot != "" {
+	if snapshot != "" {
 		if conf.ServerInfo.ServerType != ServerTypeTiDB {
 			return errors.New("snapshot consistency is not supported for this server")
 		}
+		snapshotTS, err = parseSnapshotToTSO(pool, snapshot)
+		if err != nil {
+			return err
+		}
 		if conf.Consistency == "snapshot" {
 			hasTiKV, err := CheckTiDBWithTiKV(pool)
 			if err != nil {
 				return err
 			}
 			if hasTiKV {
-				conf.SessionParams["tidb_snapshot"] = conf.Snapshot
+				conf.SessionParams["tidb_snapshot"] = snapshot
 			}
+			// convert yyyy:mm:ss type snapshot to TSO if consistency is snapshot
+			snapshot = strconv.FormatUint(snapshotTS, 10)
+		} else {
+			// clear snapshot to use current master status for metadata (consistency lock)
+			snapshot = ""
 		}
 	}
 
 	if doPdGC {
-		snapshotTS, err := parseSnapshotToTSO(pool, conf.Snapshot)
-		if err != nil {
-			return err
-		}
 		go updateServiceSafePoint(ctx, pdClient, defaultDumpGCSafePointTTL, snapshotTS)
 	} else if conf.ServerInfo.ServerType == ServerTypeTiDB {
 		log.Warn("If the amount of data to dump is large, criteria: (data more than 60GB or dumped time more than 10 minutes)\n" +
@@ -130,7 +140,7 @@ func Dump(pCtx context.Context, conf *Config) (err error) {
 			return withStack(err)
 		}
 		m.recordStartTime(time.Now())
-		err = m.recordGlobalMetaData(conn, conf.ServerInfo.ServerType, false)
+		err = m.recordGlobalMetaData(conn, conf.ServerInfo.ServerType, false, snapshot)
 		if err != nil {
 			log.Info("get global metadata failed", zap.Error(err))
 		}
@@ -159,7 +169,7 @@ func Dump(pCtx context.Context, conf *Config) (err error) {
 			return withStack(err)
 		}
 		m.recordStartTime(time.Now())
-		err = m.recordGlobalMetaData(conn, conf.ServerInfo.ServerType, false)
+		err = m.recordGlobalMetaData(conn, conf.ServerInfo.ServerType, false, snapshot)
 		if err != nil {
 			log.Info("get global metadata failed", zap.Error(err))
 		}
@@ -175,7 +185,7 @@ func Dump(pCtx context.Context, conf *Config) (err error) {
 	if conf.PosAfterConnect {
 		conn := connectPool.getConn()
 		// record again, to provide a location to exit safe mode for DM
-		err = m.recordGlobalMetaData(conn, conf.ServerInfo.ServerType, true)
+		err = m.recordGlobalMetaData(conn, conf.ServerInfo.ServerType, true, snapshot)
 		if err != nil {
 			log.Info("get global metadata (after connection pool established) failed", zap.Error(err))
 		}
diff --git a/v4/export/metadata.go b/v4/export/metadata.go
index b3e1c2e..d21fdb8 100644
--- a/v4/export/metadata.go
+++ b/v4/export/metadata.go
@@ -50,7 +50,7 @@ func (m *globalMetadata) recordFinishTime(t time.Time) {
 	m.buffer.WriteString("Finished dump at: " + t.Format(metadataTimeLayout) + "\n")
 }
 
-func (m *globalMetadata) recordGlobalMetaData(db *sql.Conn, serverType ServerType, afterConn bool) error {
+func (m *globalMetadata) recordGlobalMetaData(db *sql.Conn, serverType ServerType, afterConn bool, snapshot string) error {
 	// get master status info
 	m.buffer.WriteString("SHOW MASTER STATUS:")
 	if afterConn {
@@ -83,7 +83,12 @@ func (m *globalMetadata) recordGlobalMetaData(db *sql.Conn, serverType ServerTyp
 			return err
 		}
 		logFile := getValidStr(str, fileFieldIndex)
-		pos := getValidStr(str, posFieldIndex)
+		var pos string
+		if serverType == ServerTypeTiDB && snapshot != "" {
+			pos = snapshot
+		} else {
+			pos = getValidStr(str, posFieldIndex)
+		}
 		gtidSet := getValidStr(str, gtidSetFieldIndex)
 
 		if logFile != "" {


Test patch:
diff --git a/v4/export/metadata_test.go b/v4/export/metadata_test.go
index f1bd040..564a344 100644
--- a/v4/export/metadata_test.go
+++ b/v4/export/metadata_test.go
@@ -31,7 +31,7 @@ func (s *testMetaDataSuite) TestMysqlMetaData(c *C) {
 		sqlmock.NewRows([]string{"exec_master_log_pos", "relay_master_log_file", "master_host", "Executed_Gtid_Set", "Seconds_Behind_Master"}))
 
 	m := newGlobalMetadata(s.createStorage(c))
-	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, false), IsNil)
+	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, false, ""), IsNil)
 
 	c.Assert(m.buffer.String(), Equals, "SHOW MASTER STATUS:\n"+
 		"\tLog: ON.000001\n"+
@@ -69,8 +69,8 @@ func (s *testMetaDataSuite) TestMetaDataAfterConn(c *C) {
 	mock.ExpectQuery("SHOW MASTER STATUS").WillReturnRows(rows2)
 
 	m := newGlobalMetadata(s.createStorage(c))
-	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, false), IsNil)
-	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, true), IsNil)
+	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, false, ""), IsNil)
+	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, true, ""), IsNil)
 
 	c.Assert(m.buffer.String(), Equals, "SHOW MASTER STATUS:\n"+
 		"\tLog: ON.000001\n"+
@@ -102,7 +102,7 @@ func (s *testMetaDataSuite) TestMysqlWithFollowersMetaData(c *C) {
 	mock.ExpectQuery("SHOW SLAVE STATUS").WillReturnRows(followerRows)
 
 	m := newGlobalMetadata(s.createStorage(c))
-	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, false), IsNil)
+	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, false, ""), IsNil)
 
 	c.Assert(m.buffer.String(), Equals, "SHOW MASTER STATUS:\n"+
 		"\tLog: ON.000001\n"+
@@ -133,7 +133,7 @@ func (s *testMetaDataSuite) TestMysqlWithNullFollowersMetaData(c *C) {
 	mock.ExpectQuery("SHOW SLAVE STATUS").WillReturnRows(sqlmock.NewRows([]string{"SQL_Remaining_Delay"}).AddRow(nil))
 
 	m := newGlobalMetadata(s.createStorage(c))
-	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, false), IsNil)
+	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, false, ""), IsNil)
 
 	c.Assert(m.buffer.String(), Equals, "SHOW MASTER STATUS:\n"+
 		"\tLog: ON.000001\n"+
@@ -160,7 +160,7 @@ func (s *testMetaDataSuite) TestMariaDBMetaData(c *C) {
 	mock.ExpectQuery("SELECT @@global.gtid_binlog_pos").WillReturnRows(rows)
 	mock.ExpectQuery("SHOW SLAVE STATUS").WillReturnRows(rows)
 	m := newGlobalMetadata(s.createStorage(c))
-	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMariaDB, false), IsNil)
+	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMariaDB, false, ""), IsNil)
 
 	c.Assert(mock.ExpectationsWereMet(), IsNil)
 }
@@ -187,7 +187,7 @@ func (s *testMetaDataSuite) TestMariaDBWithFollowersMetaData(c *C) {
 	mock.ExpectQuery("SHOW ALL SLAVES STATUS").WillReturnRows(followerRows)
 
 	m := newGlobalMetadata(s.createStorage(c))
-	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, false), IsNil)
+	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, false, ""), IsNil)
 
 	c.Assert(m.buffer.String(), Equals, "SHOW MASTER STATUS:\n"+
 		"\tLog: ON.000001\n"+
@@ -225,7 +225,7 @@ func (s *testMetaDataSuite) TestEarlierMysqlMetaData(c *C) {
 		sqlmock.NewRows([]string{"exec_master_log_pos", "relay_master_log_file", "master_host", "Executed_Gtid_Set", "Seconds_Behind_Master"}))
 
 	m := newGlobalMetadata(s.createStorage(c))
-	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, false), IsNil)
+	c.Assert(m.recordGlobalMetaData(conn, ServerTypeMySQL, false, ""), IsNil)
 
 	c.Assert(m.buffer.String(), Equals, "SHOW MASTER STATUS:\n"+
 		"\tLog: mysql-bin.000001\n"+
@@ -233,3 +233,36 @@ func (s *testMetaDataSuite) TestEarlierMysqlMetaData(c *C) {
 		"\tGTID:\n\n")
 	c.Assert(mock.ExpectationsWereMet(), IsNil)
 }
+
+func (s *testMetaDataSuite) TestTiDBSnapshotMetaData(c *C) {
+	db, mock, err := sqlmock.New()
+	c.Assert(err, IsNil)
+	defer db.Close()
+	conn, err := db.Conn(context.Background())
+	c.Assert(err, IsNil)
+
+	logFile := "tidb-binlog"
+	pos := "420633329401856001"
+	rows := sqlmock.NewRows([]string{"File", "Position", "Binlog_Do_DB", "Binlog_Ignore_DB"}).
+		AddRow(logFile, pos, "", "")
+	mock.ExpectQuery("SHOW MASTER STATUS").WillReturnRows(rows)
+
+	m := newGlobalMetadata(s.createStorage(c))
+	c.Assert(m.recordGlobalMetaData(conn, ServerTypeTiDB, false, ""), IsNil)
+	c.Assert(m.buffer.String(), Equals, "SHOW MASTER STATUS:\n"+
+		"\tLog: tidb-binlog\n"+
+		"\tPos: 420633329401856001\n"+
+		"\tGTID:\n\n")
+
+	snapshot := "420633273211289601"
+	rows = sqlmock.NewRows([]string{"File", "Position", "Binlog_Do_DB", "Binlog_Ignore_DB"}).
+		AddRow(logFile, pos, "", "")
+	mock.ExpectQuery("SHOW MASTER STATUS").WillReturnRows(rows)
+	m = newGlobalMetadata(s.createStorage(c))
+	c.Assert(m.recordGlobalMetaData(conn, ServerTypeTiDB, false, snapshot), IsNil)
+	c.Assert(m.buffer.String(), Equals, "SHOW MASTER STATUS:\n"+
+		"\tLog: tidb-binlog\n"+
+		"\tPos: 420633273211289601\n"+
+		"\tGTID:\n\n")
+	c.Assert(mock.ExpectationsWereMet(), IsNil)
+}