drainer:sync lob data to oracle #1158

cartersz · 2022-04-19T07:50:48Z

What problem does this PR solve?

Issue Number: ref #1157

What is changed and how it works?

Using prepare statement method ,when access to oracle

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Has exported function/method change
Has exported variable/fields change
Has interface methods change
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

Need to cherry-pick to the release branch
Need to update the documentation
Need to update the tidb-ansible repository
Need to be included in the release note

Release note

No release note

ti-chi-bot · 2022-04-19T07:50:49Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

glorv
lichunzhu

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

cartersz · 2022-04-19T07:53:40Z

@lichunzhu PTAL

lichunzhu · 2022-04-22T02:09:09Z

/run-all-tests

lichunzhu · 2022-04-24T12:24:44Z

drainer/translator/mysql.go

@@ -31,6 +31,8 @@ import (

 const implicitColID = -1

+var destDBType = "tidb"


Can we use an iota for this variable? I'm affray that string type is not accurate and may affect the efficiency.

Suggested change

var destDBType = "tidb"

type destDBType int

var (

mysql = iota

oracle

)

OK，good idea

lichunzhu · 2022-04-24T12:40:39Z

drainer/translator/mysql.go

+		data = types.NewDatum(fmt.Sprintf("%v", data.GetValue()))
+	case mysql.TypeDuration:
+		//only for oracle db
+		if destDBType == "oracle" {


lichunzhu · 2022-04-24T12:41:11Z

drainer/translator/mysql.go

+	case mysql.TypeDuration:
+		//only for oracle db
+		if destDBType == "oracle" {
+			return data, errors.New("unsupport column type[time]")


Suggested change

return data, errors.New("unsupport column type[time]")

return data, errors.New("unsupport column type[time]")

lichunzhu · 2022-04-24T12:45:43Z

drainer/translator/mysql.go

+		//only for oracle db
+		if destDBType == "oracle" {
+			stype := types.TypeToStr(ft.Tp, ft.Charset)
+			if stype == "blob" || stype == "tinyblob" || stype == "mediumblob" || stype == "longblob" {


Can we just use IsTypeBlob in parser here?

lichunzhu · 2022-04-24T12:47:54Z

pkg/loader/model.go

+		if dml.DestDBType == "oracle" {
+			colName = escapeName(name)
+		} else {
+			colName = quoteName(name)
 		}
-		value := dml.Values[name]
-		if value == nil {
-			fmt.Fprintf(builder, "%s = NULL", escapeName(name))
+		if dml.DestDBType == "oracle" {
+			fmt.Fprintf(builder, "%s = :%d", colName, oracleHolderPos)
+			oracleHolderPos++
 		} else {
-			fmt.Fprintf(builder, "%s = %s", escapeName(name), genOracleValue(dml.UpColumnsInfoMap[name], value))
+			fmt.Fprintf(builder, "%s = ?", colName)
 		}


Can we directly write here? colName seems useless here but will take another copy time.

cartersz · 2022-04-26T06:19:58Z

pkg/loader/model.go

@@ -37,6 +35,11 @@ const (
 	DeleteDMLType  DMLType = 3
 )

+const (
+	MysqlDB = iota


cartersz · 2022-04-27T08:18:20Z

/run-all-tests

glorv

rest LGTM

glorv · 2022-04-28T07:55:55Z

drainer/sync/mysql.go

-	opts = append(opts, loader.DestinationDBType(destDBType), loader.WorkerCount(worker), loader.BatchSize(batchSize),
-		loader.SaveAppliedTS(destDBType == "tidb" || destDBType == "oracle"), loader.SetloopBackSyncInfo(info))
+	opts = append(opts, loader.DestinationDBType(destDBTypeInt), loader.WorkerCount(worker), loader.BatchSize(batchSize),
+		loader.SaveAppliedTS(destDBTypeInt == loader.MysqlDB || destDBTypeInt == loader.OracleDB), loader.SetloopBackSyncInfo(info))


From L60-63, the destDBTypeInt is either MysqlDB or OracleDB, then destDBTypeInt == loader.MysqlDB || destDBTypeInt == loader.OracleDB is always true

when downstream is mysql or oracle DB, we need to save appliedTS, if it is not , we do not need to save appliedTS.

The logic has been changed. When destDBType is mysql we shouldn't save applied ts while for tidb we should save.

glorv · 2022-04-28T07:57:18Z

drainer/translator/mysql.go

@@ -31,6 +31,8 @@ import (

 const implicitColID = -1

+var destDBType = loader.MysqlDB


why need a global variable here?

some method use this variable to check downstream DB, i don't like to pass it from one method to another

drainer/translator/mysql.go

glorv · 2022-04-28T10:52:03Z

pkg/loader/model.go

+const (
+	DBTypeUnknown = iota
+	MysqlDB
+	OracleDB
+)


Suggested change

const (

DBTypeUnknown = iota

MysqlDB

OracleDB

)

type DBType int

const (

DBTypeUnknown DBType = iota

MysqlDB

OracleDB

)

Always use DBType instead of int

glorv

LGTM

cartersz · 2022-04-28T11:46:27Z

/run-all-tests

lichunzhu · 2022-04-29T03:11:16Z

drainer/sync/mysql.go

-	opts = append(opts, loader.DestinationDBType(destDBType), loader.WorkerCount(worker), loader.BatchSize(batchSize),
-		loader.SaveAppliedTS(destDBType == "tidb" || destDBType == "oracle"), loader.SetloopBackSyncInfo(info))
+	opts = append(opts, loader.DestinationDBType(destDBTypeInt), loader.WorkerCount(worker), loader.BatchSize(batchSize),
+		loader.SaveAppliedTS(destDBTypeInt == loader.MysqlDB || destDBTypeInt == loader.OracleDB), loader.SetloopBackSyncInfo(info))


The logic has been changed. When destDBType is mysql we shouldn't save applied ts while for tidb we should save.

lichunzhu · 2022-04-29T03:12:40Z

drainer/translator/mysql.go

+	case mysql.TypeDuration:
+		//only for oracle db
+		if destDBType == loader.OracleDB {
+			return data, errors.New("unsupported column type[time]")


Suggested change

return data, errors.New("unsupported column type[time]")

return types.Datum{}, errors.New("unsupported column type[time]")

lichunzhu · 2022-04-29T03:41:05Z

drainer/translator/mysql.go

+
+func isBlob(ft types.FieldType) bool {
+	stype := types.TypeToStr(ft.Tp, ft.Charset)
+	if stype == "blob" || stype == "tinyblob" || stype == "mediumblob" || stype == "longblob" {


Suggested change

if stype == "blob" || stype == "tinyblob" || stype == "mediumblob" || stype == "longblob" {

switch stype:

case "blob", "tinyblob", "mediumblob", "longblob":

return true

lichunzhu · 2022-04-29T03:41:29Z

drainer/translator/pb.go

@@ -19,6 +19,8 @@ import (
 	"strings"
 	"time"

+	"github.com/pingcap/tidb-binlog/pkg/loader"


move this to L34~35

lichunzhu · 2022-04-29T08:16:47Z

pkg/loader/model.go

-		fmt.Fprintf(builder, "%s = ?", quoteName(name))
+		if dml.DestDBType == OracleDB {
+			fmt.Fprintf(builder, "%s = :%d", escapeName(name), oracleHolderPos)
+			oracleHolderPos++


To me oracleHolderPos = i + 1? Can we directly use i + 1 for this?

for i, name := range dml.columnNames() {

lichunzhu · 2022-04-29T08:17:21Z

pkg/loader/model.go

 	wnames, wargs := dml.whereSlice()
-	for i := 0; i < len(wnames); i++ {
+	for i, pOracleHolderPos := 0, oracleHolderPos; i < len(wnames); i++ {


ditto, is pOracleHolderPos always equal to i+1?

lichunzhu · 2022-04-29T08:21:22Z

pkg/loader/model.go

 	sql = builder.String()
 	return
 }

-func (dml *DML) buildWhere(builder *strings.Builder) (args []interface{}) {
+func (dml *DML) buildWhere(builder *strings.Builder, oracleHolderPos int) (args []interface{}) {


However, I'm a bit afraid this combination will make drainer has a lower effiency. We will do a lot of extra if check however half of them are useless because we won't change destDBType when we runs drainer.
refer: https://zhuanlan.zhihu.com/p/143275246 提前进行分支预测

lichunzhu · 2022-04-29T08:22:01Z

pkg/loader/model.go

@@ -343,14 +323,16 @@ func (dml *DML) oracleDeleteNewValueSQL() (sql string) {
 		}
 	}

-	for i := 0; i < len(colNames); i++ {
+	for i, oracleHolderPos := 0, 1; i < len(colNames); i++ {


cartersz · 2022-05-05T08:24:36Z

/run-all-tests

lichunzhu

LGTM

lichunzhu · 2022-05-05T13:36:14Z

/merge

ti-chi-bot · 2022-05-05T13:36:16Z

This pull request has been accepted and is ready to merge.

Commit hash: 4e59a23

support lob

d656aab

ti-chi-bot added the size/XL label Apr 19, 2022

lichunzhu reviewed Apr 24, 2022

View reviewed changes

fix comment

6481ff7

ti-chi-bot added size/XXL and removed size/XL labels Apr 26, 2022

cartersz commented Apr 26, 2022

View reviewed changes

pkg/loader/model.go Outdated

@@ -37,6 +35,11 @@ const (

DeleteDMLType DMLType = 3

)

const (

MysqlDB = iota

Copy link

Contributor Author

cartersz Apr 26, 2022

Choose a reason for hiding this comment

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

Ok

add comment for MysqlDB/OracleDB

cf892d9

glorv reviewed Apr 28, 2022

View reviewed changes

fix comment 2

34bb725

glorv reviewed Apr 28, 2022

View reviewed changes

drainer/translator/mysql.go Outdated Show resolved Hide resolved

glorv reviewed Apr 28, 2022

View reviewed changes

fix comment 3

205bf01

glorv approved these changes Apr 28, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 label Apr 28, 2022

fix comment on DBType

61479ca

lichunzhu reviewed Apr 29, 2022

View reviewed changes

fix comment 4

4e59a23

lichunzhu approved these changes May 5, 2022

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels May 5, 2022

ti-chi-bot added the status/can-merge label May 5, 2022

ti-chi-bot merged commit 09d4a33 into pingcap:master May 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drainer:sync lob data to oracle #1158

drainer:sync lob data to oracle #1158

cartersz commented Apr 19, 2022

ti-chi-bot commented Apr 19, 2022 •

edited

Loading

cartersz commented Apr 19, 2022

lichunzhu commented Apr 22, 2022

lichunzhu Apr 24, 2022

lichunzhu Apr 24, 2022

cartersz Apr 25, 2022

lichunzhu Apr 24, 2022

lichunzhu Apr 24, 2022

lichunzhu Apr 24, 2022

lichunzhu Apr 24, 2022

cartersz Apr 25, 2022

cartersz Apr 26, 2022

cartersz commented Apr 27, 2022

glorv left a comment

glorv Apr 28, 2022

cartersz Apr 28, 2022

lichunzhu Apr 29, 2022

glorv Apr 28, 2022

cartersz Apr 28, 2022

glorv Apr 28, 2022

glorv left a comment

cartersz commented Apr 28, 2022

lichunzhu Apr 29, 2022

lichunzhu Apr 29, 2022

lichunzhu Apr 29, 2022

lichunzhu Apr 29, 2022

lichunzhu Apr 29, 2022

lichunzhu Apr 29, 2022

lichunzhu Apr 29, 2022

lichunzhu Apr 29, 2022

cartersz commented May 5, 2022

lichunzhu left a comment

lichunzhu commented May 5, 2022

ti-chi-bot commented May 5, 2022

		@@ -31,6 +31,8 @@ import (

		const implicitColID = -1

		var destDBType = "tidb"

-var destDBType = "tidb"
+type destDBType int
+var (
+    mysql = iota
+    oracle
+)

	return data, errors.New("unsupport column type[time]")
	return data, errors.New("unsupport column type[time]")

		@@ -31,6 +31,8 @@ import (

		const implicitColID = -1

		var destDBType = loader.MysqlDB

	return data, errors.New("unsupported column type[time]")
	return types.Datum{}, errors.New("unsupported column type[time]")

-	if stype == "blob" || stype == "tinyblob" || stype == "mediumblob" || stype == "longblob" {
+	switch stype:
+	case "blob", "tinyblob", "mediumblob", "longblob":
+		return true

drainer:sync lob data to oracle #1158

drainer:sync lob data to oracle #1158

Conversation

cartersz commented Apr 19, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 19, 2022 • edited Loading

cartersz commented Apr 19, 2022

lichunzhu commented Apr 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartersz commented Apr 27, 2022

glorv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glorv left a comment

Choose a reason for hiding this comment

cartersz commented Apr 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartersz commented May 5, 2022

lichunzhu left a comment

Choose a reason for hiding this comment

lichunzhu commented May 5, 2022

ti-chi-bot commented May 5, 2022

ti-chi-bot commented Apr 19, 2022 •

edited

Loading