loader, syncer: Update sql_mode to more compatible #1869

okJiang · 2021-07-14T04:55:43Z

What problem does this PR solve?

close #1735 and #1818

What is changed and how it works?

By add/delete some upstream's global.sql_mode to make downstream's sql_mode more compatible.

Check List

Tests

Integration test

Code changes

Side effects

Related changes

* update load.go/syncer.go with modifying sql_mode, * add sql_mode Integration Test

CLAassistant · 2021-07-14T04:55:47Z

All committers have signed the CLA.

loader/loader.go

tests/sql_mode/run.sh

lance6716

also notice there're two usage of SQL mode: one to execute SQLs and one to parse upstream DDLs

syncer/syncer.go

loader/loader.go

tests/sql_mode/data/db2.increment.sql

tests/sql_mode/data/db2.prepare.sql

glorv · 2021-07-14T09:06:13Z

/run-all-tests

pkg/utils/db.go

glorv · 2021-07-14T11:05:36Z

Integration test handle_error failed by:

[2021-07-14T10:22:13.455Z] got=0 expected=2

[2021-07-14T10:22:13.456Z] command: query-status test Error .*: Incorrect int value count: 0 != expected: 2, failed the 9-th time, will retry again

@okJiang Seems after we remove STRICT_TRANS_TABLES from sql_mode, this will cause insert with invalid value to succeed instead of failed, which is expected. So https://github.com/pingcap/dm/blob/master/tests/handle_error/run.sh#L20-L38 this test will failed.

But In the original test case, the insert fails because upstream change column type but downstream (tidb) don't support this, so after skip ddl, the insert should fail bacuase data in not fit in downstream anymore.

@GMHDBJD @lance6716 @kennytm PTAL at this case, I think we should still keep the STRICT_TRANS_TABLES and all user to manually set sql_mode e.g. to just remove STRICT_TRANS_TABLES if needed.

pkg/utils/db.go

tests/sql_mode/run.sh

glorv · 2021-07-15T01:57:37Z

Integration test handle_error failed by:
[2021-07-14T10:22:13.455Z] got=0 expected=2

[2021-07-14T10:22:13.456Z] command: query-status test Error .*: Incorrect int value count: 0 != expected: 2, failed the 9-th time, will retry again
@okJiang Seems after we remove STRICT_TRANS_TABLES from sql_mode, this will cause insert with invalid value to succeed instead of failed, which is expected. So https://github.com/pingcap/dm/blob/master/tests/handle_error/run.sh#L20-L38 this test will failed.

But In the original test case, the insert fails because upstream change column type but downstream (tidb) don't support this, so after skip ddl, the insert should fail bacuase data in not fit in downstream anymore.

@GMHDBJD @lance6716 @kennytm PTAL at this case, I think we should still keep the STRICT_TRANS_TABLES and all user to manually set sql_mode e.g. to just remove STRICT_TRANS_TABLES if needed.

I rethought about this, If data have been successfully inserted in the upstream, it must be valid even if the origin value may not be. So I think we should always enable STRICT_TRANS_TABLES to avoid ignoring this kind of bad data.

GMHDBJD · 2021-07-15T02:05:39Z

I rethought about this, If data have been successfully inserted in the upstream, it must be valid even if the origin value may not be. So I think we should always enable STRICT_TRANS_TABLES to avoid ignoring this kind of bad data.

@glorv I think we should keep STRICT_TRANS_TABLES consistent with upstream, and allow users to specify it.
If the user removes STRICT_TRANS_TABLES in upstream but keeps it downstream, some data may be invalid when replicating to downstream. https://asktug.com/t/topic/63881

okJiang · 2021-07-15T02:08:29Z

Integration test handle_error failed by:
[2021-07-14T10:22:13.455Z] got=0 expected=2

[2021-07-14T10:22:13.456Z] command: query-status test Error .*: Incorrect int value count: 0 != expected: 2, failed the 9-th time, will retry again
@okJiang Seems after we remove STRICT_TRANS_TABLES from sql_mode, this will cause insert with invalid value to succeed instead of failed, which is expected. So master/tests/handle_error/run.sh#L20-L38 this test will failed.
But In the original test case, the insert fails because upstream change column type but downstream (tidb) don't support this, so after skip ddl, the insert should fail bacuase data in not fit in downstream anymore.
@GMHDBJD @lance6716 @kennytm PTAL at this case, I think we should still keep the STRICT_TRANS_TABLES and all user to manually set sql_mode e.g. to just remove STRICT_TRANS_TABLES if needed.
I rethought about this, If data have been successfully inserted in the upstream, it must be valid even if the origin value may not be. So I think we should always enable STRICT_TRANS_TABLES to avoid ignoring this kind of bad data.

But once always enable STRICT_TRANS_TABLES, most of the work we do is meaningless. Because

Strict mode affects handling of division by zero, zero dates, and zeros in dates
https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html#sql-mode-strict

And at the beginning we wanted to solve the problem that user insert invalid datetime #1735.

lance6716 · 2021-07-15T02:24:11Z

not catch up with your discussion, I just want to remind the data may be inserted in a session SQL mode that is different with global SQL mode

okJiang · 2021-07-15T07:51:17Z

/run-all-tests

loader/loader.go

tests/sql_mode/data/db2.increment.sql

lichunzhu

LGTM

okJiang · 2021-07-19T04:12:30Z

/run-all-tests

okJiang · 2021-07-19T12:01:25Z

PTAL @lance6716 @GMHDBJD

lance6716

rest lgtm

loader/loader.go

pkg/utils/db.go

syncer/syncer.go

lance6716 · 2021-07-20T02:37:19Z

tests/sql_mode/data/db1.prepare.sql

+);
+
+-- test sql_mode PIPES_AS_CONCAT
+insert into t_1(num) values('pipes'||'as'||'concat');


why this can be inserted into an int field 😂

Because it exists here as a bit operation OR.

lance6716 · 2021-07-20T02:38:57Z

tests/sql_mode/data/db1.prepare.sql

+-- test sql_mode NO_AUTO_VALUE_ON_ZERO
+insert into t_1(id, name) values (10, 'a');
+insert into t_1(id, name) values (0, 'b');
+insert into t_1(id, name) values (0, 'c');


since we didn't set sql mode in this SQL file, above two insert are converted to auto increment value, so the binlog is writtern with the auto increment value not zero, and this didn't test DM will add NO_AUTO_VALUE_ON_ZERO with AdjustSQLModeCompatible

since db2 (source2) has done this test, maybe we could remove above lines.

Yes, I think it just tested what you said above.

tests/sql_mode/data/db1.increment.sql

tests/sql_mode/data/db2.increment.sql

ti-chi-bot · 2021-07-20T05:48:42Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lance6716
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.

lance6716 · 2021-07-20T05:50:28Z

/run-all-tests

okJiang · 2021-07-20T07:52:00Z

/run-all-tests

glorv · 2021-07-20T08:29:25Z

/run-all-tests

glorv · 2021-07-20T10:59:48Z

/merge

ti-chi-bot · 2021-07-20T10:59:51Z

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

Commit hash: d436f23

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2021-07-20T11:22:12Z

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

okJiang added 2 commits July 14, 2021 11:40

Update sql_mode to more compatible

f1ec1a3

* update load.go/syncer.go with modifying sql_mode, * add sql_mode Integration Test

Add some notation

f115e8b

ti-chi-bot added the first-time-contributor label Jul 14, 2021

ti-chi-bot requested review from GMHDBJD and lichunzhu July 14, 2021 04:55

ti-chi-bot added the size/XL label Jul 14, 2021

GMHDBJD reviewed Jul 14, 2021

View reviewed changes

loader/loader.go Outdated Show resolved Hide resolved

GMHDBJD reviewed Jul 14, 2021

View reviewed changes

tests/sql_mode/run.sh Show resolved Hide resolved

lance6716 reviewed Jul 14, 2021

View reviewed changes

syncer/syncer.go Outdated Show resolved Hide resolved

add sql_mode in other_intergration.txt

665a450

glorv reviewed Jul 14, 2021

View reviewed changes

loader/loader.go Outdated Show resolved Hide resolved

tests/sql_mode/data/db2.increment.sql Outdated Show resolved Hide resolved

tests/sql_mode/data/db2.prepare.sql Show resolved Hide resolved

glorv added the needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 label Jul 14, 2021

glorv added this to the v2.0.5 milestone Jul 14, 2021

add omission

b504432

lance6716 added the needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated label Jul 14, 2021

Add AdjustSQLModeCompatible in pkg/utils/db.go

aca089c

lance6716 reviewed Jul 14, 2021

View reviewed changes

pkg/utils/db.go Outdated Show resolved Hide resolved

Modify func name and add comment

4505b6c

Ehco1996 reviewed Jul 14, 2021

View reviewed changes

pkg/utils/db.go Show resolved Hide resolved

tests/sql_mode/run.sh Outdated Show resolved Hide resolved

okJiang added 3 commits July 15, 2021 10:31

Replace SQLMode2Str with tmysql.Str2SQLMode

dc74602

revert user configue

2452d7e

Adjust sql order

7250ee0

lichunzhu reviewed Jul 16, 2021

View reviewed changes

loader/loader.go Show resolved Hide resolved

glorv reviewed Jul 16, 2021

View reviewed changes

tests/sql_mode/data/db2.increment.sql Show resolved Hide resolved

Add log information

c50551a

lichunzhu approved these changes Jul 16, 2021

View reviewed changes

ti-chi-bot added status/LGT1 One reviewer already commented LGTM needs-rebase labels Jul 16, 2021

Merge branch 'master' into master

c0dd4df

ti-chi-bot removed the needs-rebase label Jul 19, 2021

Merge branch 'master' into master

ee63893

test

dcbd125

lance6716 reviewed Jul 20, 2021

View reviewed changes

fix review

d49933a

lance6716 approved these changes Jul 20, 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 Jul 20, 2021

Merge branch 'master' into master

d436f23

ti-chi-bot added the status/can-merge label Jul 20, 2021

ti-chi-bot merged commit caad97b into pingcap:master Jul 20, 2021

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

This is an automated cherry-pick of pingcap#1869

1966a72

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Jul 20, 2021

loader, syncer: Update sql_mode to more compatible (#1869) #1894

Merged

ti-chi-bot added a commit that referenced this pull request Jul 22, 2021

loader, syncer: Update sql_mode to more compatible (#1869) (#1894)

04f010c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loader, syncer: Update sql_mode to more compatible #1869

loader, syncer: Update sql_mode to more compatible #1869

okJiang commented Jul 14, 2021

CLAassistant commented Jul 14, 2021 •

edited

Loading

lance6716 left a comment

glorv commented Jul 14, 2021

glorv commented Jul 14, 2021 •

edited

Loading

glorv commented Jul 15, 2021

GMHDBJD commented Jul 15, 2021

okJiang commented Jul 15, 2021

lance6716 commented Jul 15, 2021 •

edited

Loading

okJiang commented Jul 15, 2021

lichunzhu left a comment

okJiang commented Jul 19, 2021

okJiang commented Jul 19, 2021

lance6716 left a comment

lance6716 Jul 20, 2021

okJiang Jul 20, 2021

lance6716 Jul 20, 2021

okJiang Jul 20, 2021

ti-chi-bot commented Jul 20, 2021

lance6716 commented Jul 20, 2021

okJiang commented Jul 20, 2021

glorv commented Jul 20, 2021

glorv commented Jul 20, 2021

ti-chi-bot commented Jul 20, 2021

ti-chi-bot commented Jul 20, 2021

loader, syncer: Update sql_mode to more compatible #1869

loader, syncer: Update sql_mode to more compatible #1869

Conversation

okJiang commented Jul 14, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

CLAassistant commented Jul 14, 2021 • edited Loading

lance6716 left a comment

Choose a reason for hiding this comment

glorv commented Jul 14, 2021

glorv commented Jul 14, 2021 • edited Loading

glorv commented Jul 15, 2021

GMHDBJD commented Jul 15, 2021

okJiang commented Jul 15, 2021

lance6716 commented Jul 15, 2021 • edited Loading

okJiang commented Jul 15, 2021

lichunzhu left a comment

Choose a reason for hiding this comment

okJiang commented Jul 19, 2021

okJiang commented Jul 19, 2021

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 Jul 20, 2021

Choose a reason for hiding this comment

okJiang Jul 20, 2021

Choose a reason for hiding this comment

lance6716 Jul 20, 2021

Choose a reason for hiding this comment

okJiang Jul 20, 2021

Choose a reason for hiding this comment

ti-chi-bot commented Jul 20, 2021

lance6716 commented Jul 20, 2021

okJiang commented Jul 20, 2021

glorv commented Jul 20, 2021

glorv commented Jul 20, 2021

ti-chi-bot commented Jul 20, 2021

ti-chi-bot commented Jul 20, 2021

CLAassistant commented Jul 14, 2021 •

edited

Loading

glorv commented Jul 14, 2021 •

edited

Loading

lance6716 commented Jul 15, 2021 •

edited

Loading