*: auto discover ANSI_QUOTES #889

lance6716 · 2020-08-12T09:09:39Z

What problem does this PR solve?

close #878

What is changed and how it works?

for DB related sqls, query global/session variables and find if set ANSI_QUOTES.
for binlog related sqls, using pkg/binlog: parse statusVars and get ansi-quotes mode #882.
some sqls are generated by parser using default flag, so a default parser could handle.

still need user specified ANSI_QUOTES:

~~dump unit, will check if DM could handle this~~
~~dependency of tidb-tools, could remove after check, diff: auto discover ansi-quotes tidb-tools#381~~

Check List

Tests

Unit test
Integration test

Code changes

Side effects

Possible performance regression

Related changes

Need to cherry-pick to the release branch

syncer/syncer.go

lance6716 · 2020-08-12T12:48:31Z

syncer/syncer.go

@@ -612,18 +614,17 @@ func (s *Syncer) getTable(origSchema, origTable, renamedSchema, renamedTable str
 func (s *Syncer) trackTableInfoFromDownstream(origSchema, origTable, renamedSchema, renamedTable string) error {
 	// TODO: Switch to use the HTTP interface to retrieve the TableInfo directly


going to fix this TODO in another PR (though fix this TODO will make GetParserForConn unued, we might left GetParserForConn there in case of future using)

Now, we have TLS support, if switching to HTTP API, do we need to handle it, right?

lance6716 · 2020-08-12T13:07:50Z

/run-all-tests

GMHDBJD · 2020-08-24T02:25:10Z

pkg/utils/db.go

-// GetSQLMode returns sql_mode.
-func GetSQLMode(db *sql.DB) (tmysql.SQLMode, error) {
+// GetParser gets a parser for sql.DB which maybe enabled `ANSI_QUOTES` sql_mode
+func GetParser(db *sql.DB) (*parser.Parser, error) {
 	sqlMode, err := GetGlobalVariable(db, "sql_mode")


Session here ditto or use func of tidb-tools.

changed in 104f5df

Have not change?

want to explain that (*UpStreamConn).getParser will use GetParserForConn, which further uses session variables so could reflect variables set though DSN. And (*UpStreamConn).getParser is used in most unit, in fact GetParser is only used in relay unit and tests 😿

will change it later

lance6716 · 2020-09-03T11:33:11Z

dumpling's SessionParams and GetDSN doesn't change SQL mode. so currently we can't make dumpling's session variable different than global. If in future we find that we should manually set SQL mode, we might handling "use session variable" as well as "pass specified SQL mode to dumpling"

lance6716 · 2020-09-03T11:38:25Z

pkg/binlog/reader/util.go

+				ansiQuotes = false
+			}
+			if ansiQuotes {
+				parser2.SetSQLMode(tmysql.ModeANSIQuotes)


I may as well parse all available SQL mode and set here 😂

will check "Full List of SQL Modes" in https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html tomorrow

csuzhangxc

rest LGTM except #889 (comment)

GMHDBJD · 2020-09-04T02:31:06Z

dumpling/dumpling.go

 	db, err := sql.Open("mysql", m.dumpConfig.GetDSN(""))
 	if err != nil {
 		return
 	}
 	defer db.Close()
-	enable, err := utils.HasAnsiQuotesMode(db)
+
+	sqlMode, err := utils.GetGlobalVariable(db, "sql_mode")


Session here?

dumpling didn't expose it's connection, so in fact we can't see its session variable. and by GetDSN and config, we can change its session variable different than global ones

lance6716 · 2020-09-04T03:47:47Z

waiting pingcap/parser#1008

csuzhangxc

LGTM

lance6716 · 2020-09-07T02:26:43Z

PTAL @GMHDBJD

GMHDBJD

LGTM

ti-srebot · 2020-09-07T12:04:53Z

cherry pick to release-2.0 failed

lance6716 added 4 commits August 12, 2020 13:10

stash

1dc615f

stash

f9df7ec

Merge branch 'master' of https://github.com/pingcap/dm into dev

322bcba

*: auto discover ANSI_QUOTES

7aa308c

lance6716 commented Aug 12, 2020

View reviewed changes

syncer/syncer.go Outdated Show resolved Hide resolved

lance6716 added status/WIP This PR is still work in progress and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Aug 12, 2020

lance6716 added 2 commits August 12, 2020 18:03

fix ut

0a3a8cb

fix it

f10a4bc

lance6716 added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Aug 12, 2020

lance6716 commented Aug 12, 2020

View reviewed changes

don't know why dm-master exit

bcd4c99

This comment has been minimized.

Sign in to view

lance6716 added 3 commits August 13, 2020 10:08

fix it

6d07410

Merge branch 'master' of https://github.com/pingcap/dm into dev

ab10da4

re-sort import

86f4404

GMHDBJD reviewed Aug 24, 2020

View reviewed changes

lance6716 added 3 commits August 24, 2020 20:07

Merge branch 'master' of https://github.com/pingcap/dm into dev

d3f5b59

address comment

104f5df

Merge branch 'master' of https://github.com/pingcap/dm into dev

2e6c012

lance6716 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 Aug 25, 2020

lance6716 added 3 commits August 26, 2020 15:10

Merge branch 'master' of https://github.com/pingcap/dm into dev

8cb5efa

track master

6ac75cc

update behaviour

488db8e

lance6716 force-pushed the dev branch from cb07081 to d8d29e0 Compare September 3, 2020 11:22

lance6716 commented Sep 3, 2020

View reviewed changes

csuzhangxc reviewed Sep 3, 2020

View reviewed changes

GMHDBJD reviewed Sep 4, 2020

View reviewed changes

save my work

83bacb7

lance6716 added the status/DNM Do not merge, test is failing or blocked by another PR label Sep 4, 2020

lance6716 added 5 commits September 5, 2020 21:00

address comment

02df4e3

Merge branch 'master' into dev

5cb6a4a

update parser const

d265353

fix test

090d610

fix IT

1cbcf6b

lance6716 removed the status/DNM Do not merge, test is failing or blocked by another PR label Sep 5, 2020

fix comment

5281599

csuzhangxc reviewed Sep 6, 2020

View reviewed changes

lance6716 added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Sep 7, 2020

Merge branch 'master' into dev

0018d89

GMHDBJD approved these changes Sep 7, 2020

View reviewed changes

lance6716 merged commit 684a1be into pingcap:master Sep 7, 2020

lance6716 added a commit to lance6716/dm that referenced this pull request Sep 8, 2020

*: auto discover ANSI_QUOTES (pingcap#889)

954be35

lance6716 mentioned this pull request Sep 8, 2020

*: auto discover ANSI_QUOTES #889 #1005

Merged

lance6716 added a commit that referenced this pull request Sep 9, 2020

*: auto discover ANSI_QUOTES (#889) (#1005)

02f7846

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: auto discover ANSI_QUOTES #889

*: auto discover ANSI_QUOTES #889

lance6716 commented Aug 12, 2020 •

edited

Loading

lance6716 Aug 12, 2020 •

edited

Loading

csuzhangxc Aug 13, 2020

lance6716 commented Aug 12, 2020

This comment has been minimized.

GMHDBJD Aug 24, 2020 •

edited

Loading

lance6716 Aug 25, 2020

GMHDBJD Sep 4, 2020

lance6716 Sep 5, 2020

lance6716 Sep 5, 2020

lance6716 commented Sep 3, 2020

lance6716 Sep 3, 2020

csuzhangxc left a comment

GMHDBJD Sep 4, 2020

lance6716 Sep 4, 2020

lance6716 commented Sep 4, 2020

csuzhangxc left a comment

lance6716 commented Sep 7, 2020

GMHDBJD left a comment

ti-srebot commented Sep 7, 2020

		@@ -612,18 +614,17 @@ func (s *Syncer) getTable(origSchema, origTable, renamedSchema, renamedTable str
		func (s *Syncer) trackTableInfoFromDownstream(origSchema, origTable, renamedSchema, renamedTable string) error {
		// TODO: Switch to use the HTTP interface to retrieve the TableInfo directly

*: auto discover ANSI_QUOTES #889

*: auto discover ANSI_QUOTES #889

Conversation

lance6716 commented Aug 12, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

lance6716 Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 commented Aug 12, 2020

This comment has been minimized.

GMHDBJD Aug 24, 2020 • edited Loading

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

lance6716 commented Sep 3, 2020

Choose a reason for hiding this comment

csuzhangxc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 commented Sep 4, 2020

csuzhangxc left a comment

Choose a reason for hiding this comment

lance6716 commented Sep 7, 2020

GMHDBJD left a comment

Choose a reason for hiding this comment

ti-srebot commented Sep 7, 2020

lance6716 commented Aug 12, 2020 •

edited

Loading

lance6716 Aug 12, 2020 •

edited

Loading

GMHDBJD Aug 24, 2020 •

edited

Loading