sync: refactor dml.go #2234

GMHDBJD · 2021-10-18T09:22:59Z

What problem does this PR solve?

refactor dml.go for later develop

use INSERT ON DUPLICATE KEY UPDATE instead of REPLACE in safemode
generate DML SQL string before execute rather than before addJob.

Check List

Tests

orginal test
unit test

GMHDBJD

PTAL

GMHDBJD · 2021-10-19T07:12:39Z

syncer/dml.go

+}
+
+// genKeyList format keys.
+func genKeyList(table string, columns []*model.ColumnInfo, dataSeq []interface{}) string {


same as

dm/syncer/dml.go

Line 414 in 256afb4

func genKeyList(table string, columns []*model.ColumnInfo, dataSeq []interface{}) string {

GMHDBJD · 2021-10-19T07:13:28Z

syncer/dml.go

+
+// genMultipleKeys gens keys with UNIQUE NOT NULL value.
+// if not UNIQUE NOT NULL value, use table name instead.
+func genMultipleKeys(ti *model.TableInfo, value []interface{}, table string) []string {


same as

dm/syncer/dml.go

Line 435 in 256afb4

func genMultipleKeys(ti *model.TableInfo, value []interface{}, table string) []string {

GMHDBJD · 2021-10-19T07:14:03Z

syncer/dml.go

+}
+
+// genWhere generates where condition.
+func (dml *DML) genWhere(buf *strings.Builder) []interface{} {


refactor from

dm/syncer/dml.go

Line 541 in 256afb4

func genWhere(buf *strings.Builder, columns []*model.ColumnInfo, data []interface{}) {

lance6716 · 2021-10-19T09:04:43Z

I forget why we choose INSERT ON DUPLICATE KEY UPDATE? seems INSERT ON DUPLICATE KEY UPDATE and REPLACE can both use BatchGet when have multiple values

test they two ⬆️

syncer/dml.go

GMHDBJD · 2021-10-20T03:49:31Z

I forget why we choose INSERT ON DUPLICATE KEY UPDATE? seems INSERT ON DUPLICATE KEY UPDATE and REPLACE can both use BatchGet when have multiple values

I check the TiDB code, REPLACE use get and INSERT ON DUPLICATE KEY UPDATE use batch get.
https://github.com/pingcap/tidb/blob/94e30df8e2d8ba2a1a26f153f40067ba3acd78eb/executor/replace.go#L130
https://github.com/pingcap/tidb/blob/94e30df8e2d8ba2a1a26f153f40067ba3acd78eb/executor/insert.go#L72

Another reason is that if update statement doesn't update index value, change it to REPLACE statement will delete and insert it, which will update index. While change it to INSERT ON DUPLICATE KEY UPDATE will be a update, which will not update index.

Furthermore, if we are in safemode, REPLACE on same value will still delete and insert the row again, but INSERT ON DUPLICATE UPDATE will not update the row at all.

I tested it before and it was in line with this conclusion

lance6716 · 2021-10-20T04:27:40Z

in my memory REPLACE will also use batch get to get all rows before mutation https://github.com/pingcap/tidb/blob/94e30df8e2d8ba2a1a26f153f40067ba3acd78eb/executor/replace.go#L233

good to reduce index kv mutation 👍 seems ON DUPLICATE KEY UPDATE will use TableCommon.UpdateRecord. compared with TableCommon.AddRecord, it will not put index KVs.

Ehco1996

LGTM

syncer/dml_test.go

GMHDBJD · 2021-10-22T02:49:22Z

/run-all-tests

lance6716

rest lgtm

lance6716 · 2021-10-25T02:26:36Z

syncer/dml.go

@@ -33,6 +33,7 @@ import (
 // genDMLParam stores pruned columns, data as well as the original columns, data, index.
 type genDMLParam struct {
 	tableID         string              // as a key in map like `schema`.`table`


better to note this is target table ID (maybe in future PR)

lance6716 · 2021-10-25T02:31:43Z

syncer/dml.go

-		sqls = append(sqls, sql)
-		values = append(values, value)
-		keys = append(keys, ks)
+		dmls = append(dmls, newDML(insert, param.safeMode, param.tableID, param.sourceTable, nil, value, nil, originalValue, columns, ti))


slightly recommend that if DML struct will always stay same package with syncer, I think it's more readable that we use literal to create it. Like

DML{op: insert, ..., sourceTableInfo: ti}

So many args, so keep this function.

lance6716 · 2021-10-25T02:39:58Z

syncer/dml.go

+		key := genKeyList(table, cols, vals)
+		if len(key) > 0 { // ignore `null` value.
+			multipleKeys = append(multipleKeys, key)
+			// TODO: break here? one unique index is enough?


this comment is added by me about one year ago, now it can be removed 😂

lance6716 · 2021-10-25T03:05:47Z

syncer/job.go

@@ -87,18 +84,15 @@ type job struct {

 func (j *job) String() string {
 	// only output some important information, maybe useful in execution.
-	return fmt.Sprintf("tp: %s, sql: %s, args: %v, key: %s, ddls: %s, last_location: %s, start_location: %s, current_location: %s", j.tp, j.sql, j.args, j.key, j.ddls, j.location, j.startLocation, j.currentLocation)


how can we know the DML values in debug mode now?

GMHDBJD · 2021-10-25T05:18:15Z

/merge

ti-chi-bot · 2021-10-25T05:18:18Z

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

Commit hash: 2ebdb46

ti-chi-bot · 2021-10-25T06:17:24Z

In response to a cherrypick label: new pull request created: #2256.

GMHDBJD and others added 30 commits August 29, 2021 09:39

refine sync dml

52e9f13

fix bug

96008ac

fix data race

b54a927

fix data race

53de6ce

fix test

22dad39

update

9606c90

update

57145f9

fix test

90f17bc

fix test

34d4989

fix ut

687d4e0

fix it

0ae0f33

save work

3163999

fix wait

6f1fe91

fix

4851cd4

Merge remote-tracking branch 'upstream/master' into refineSyncDML

b7cf22d

update causality and dml worker

30be49c

Merge remote-tracking branch 'upstream/master' into refineSyncDML

ff65707

refine

2ca637c

fix ut

9b5c1fe

Merge branch 'master' into refineSyncDML

735a087

fix causality

cb7a298

Merge branch 'master' into refineSyncDML

53ed0e5

refine queue size metrics

1401f4d

fix

5da87a1

Merge branch 'master' into refineSyncDML

f7392fb

debug ci

06dda5d

fix

f31328e

revert

f02faeb

add test back

7c44aec

update metrics

22c9374

GMHDBJD commented Oct 19, 2021

View reviewed changes

lance6716 changed the title ~~sync: refactor dml.go [WIP]~~ sync: refactor dml.go Oct 19, 2021

Ehco1996 reviewed Oct 20, 2021

View reviewed changes

GMHDBJD added 2 commits October 20, 2021 04:13

address comment

401e560

address comment

ebb36fb

Ehco1996 approved these changes Oct 21, 2021

View reviewed changes

syncer/dml_test.go Outdated Show resolved Hide resolved

ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Oct 21, 2021

GMHDBJD and others added 2 commits October 21, 2021 05:34

add no primary/unique key test

d99bb92

Merge branch 'master' into refactorDML

9993694

lance6716 reviewed Oct 25, 2021

View reviewed changes

address comment

4fc1ee6

lance6716 approved these changes Oct 25, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Oct 25, 2021

Merge branch 'master' into refactorDML

2ebdb46

ti-chi-bot added the status/can-merge label Oct 25, 2021

ti-chi-bot merged commit d625271 into pingcap:master Oct 25, 2021

ti-chi-bot pushed a commit to ti-chi-bot/dm that referenced this pull request Oct 25, 2021

sync: refactor dml.go (pingcap#2234)

684752c

ti-chi-bot mentioned this pull request Oct 25, 2021

sync: refactor dml.go (#2234) #2256

Merged

ti-chi-bot added a commit that referenced this pull request Oct 25, 2021

sync: refactor dml.go (#2234) (#2256)

e109e02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync: refactor dml.go #2234

sync: refactor dml.go #2234

GMHDBJD commented Oct 18, 2021 •

edited

Loading

GMHDBJD left a comment

GMHDBJD Oct 19, 2021

GMHDBJD Oct 19, 2021

GMHDBJD Oct 19, 2021

lance6716 commented Oct 19, 2021

GMHDBJD commented Oct 20, 2021 •

edited

Loading

lance6716 commented Oct 20, 2021 •

edited

Loading

Ehco1996 left a comment

GMHDBJD commented Oct 22, 2021

lance6716 left a comment

lance6716 Oct 25, 2021

lance6716 Oct 25, 2021

GMHDBJD Oct 25, 2021

lance6716 Oct 25, 2021

lance6716 Oct 25, 2021

GMHDBJD commented Oct 25, 2021

ti-chi-bot commented Oct 25, 2021

ti-chi-bot commented Oct 25, 2021

sync: refactor dml.go #2234

sync: refactor dml.go #2234

Conversation

GMHDBJD commented Oct 18, 2021 • edited Loading

What problem does this PR solve?

Check List

GMHDBJD 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

lance6716 commented Oct 19, 2021

GMHDBJD commented Oct 20, 2021 • edited Loading

lance6716 commented Oct 20, 2021 • edited Loading

Ehco1996 left a comment

Choose a reason for hiding this comment

GMHDBJD commented Oct 22, 2021

lance6716 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

GMHDBJD commented Oct 25, 2021

ti-chi-bot commented Oct 25, 2021

ti-chi-bot commented Oct 25, 2021

GMHDBJD commented Oct 18, 2021 •

edited

Loading

GMHDBJD commented Oct 20, 2021 •

edited

Loading

lance6716 commented Oct 20, 2021 •

edited

Loading