fix snapshot problem #111

lichunzhu · 2020-06-30T09:56:04Z

What problem does this PR solve?

fix #112

Now dumpling use db.Exec("set tidb_snapshot = ?") to set tidb_snapshot, but this can only influence that connection. The connection pool's tidb_snapshot is not set.
Now dumpling get tables before we set snapshot, which means the fetched tables are not the tables we actually want. But for consistency "lock" it needs all tables to lock before the dump process.

What is changed and how it works?

Add SessionParams in conf. After we adjust all the SessionParams dumpling will reset sql.DB with the new dsn. This can make sure all connections' session variable has been set.
Delete consistency snapshot. Current process order:
a. set snapshot ts into session params
b. fetch all the tables.
c. set consistency.

Check List

Tests

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

Side effects

Increased code complexity

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Release note

fix the problem that --snapshot argument can't work

codecov · 2020-06-30T10:01:19Z

Codecov Report

Merging #111 into master will decrease coverage by 1.35%.
The diff coverage is 6.15%.

@@            Coverage Diff             @@
##           master     #111      +/-   ##
==========================================
- Coverage   51.92%   50.56%   -1.36%     
==========================================
  Files          17       17              
  Lines        1770     1780      +10     
==========================================
- Hits          919      900      -19     
- Misses        782      816      +34     
+ Partials       69       64       -5

kennytm

rest LGTM

kennytm · 2020-06-30T10:26:32Z

v4/export/sql.go

+		v = wrapQuotes(v)
+		s := fmt.Sprintf("SET SESSION %s = %s", k, v)
+		_, err := db.Exec(s)


Suggested change

v = wrapQuotes(v)

s := fmt.Sprintf("SET SESSION %s = %s", k, v)

_, err := db.Exec(s)

s := fmt.Sprintf("SET SESSION %s = ?", k)

_, err := db.Exec(s, v)

I use this format at first, but at last this failed when I try to set tidb_mem_quota_query.

[2020/06/30 18:39:05.433 +08:00] [ERROR] [main.go:191] ["dump failed error stack info"] [error="Error 1231: Variable 'tidb_mem_quota_query' can't be set to the value of '%!s(MISSING)'"] [errorVerbose="err = Error 1231: Variable 'tidb_mem_quota_query' can't be set to the value of '%!s(MISSING)'\ngoroutine 1 [running]:\nruntime/debug.Stack(0x19fc520, 0xc0001fc220, 0x19fd9e0)\n\t/usr/local/go/src/runtime/debug/stack.go:24 +0x9d\ngithub.com/pingcap/dumpling/v4/export.withStack(0x19fc520, 0xc0001fc220, 0xc000038000, 0xc000152570)\n\t/Users/chauncy/code/goPath/src/github.com/pingcap/dumpling/v4/export/error.go:59 +0x8d\ngithub.com/pingcap/dumpling/v4/export.resetDBWithSessionParams(0xc0001fe000, 0xc000152540, 0x2a, 0xc0001d2600, 0x2a, 0x0, 0x0)\n\t/Users/chauncy/code/goPath/src/github.com/pingcap/dumpling/v4/export/sql.go:389 +0x684\ngithub.com/pingcap/dumpling/v4/export.Dump(0x1a13400, 0xc000038090, 0xc00012cea0, 0x0, 0x0)\n\t/Users/chauncy/code/goPath/src/github.com/pingcap/dumpling/v4/export/dump.go:101 +0x327\nmain.main()\n\t/Users/chauncy/code/goPath/src/github.com/pingcap/dumpling/cmd/dumpling/main.go:189 +0x132f\n"] [stack="github.com/pingcap/log.Error\n\t/Users/chauncy/code/goPath/pkg/mod/github.com/pingcap/log@v0.0.0-20200511115504-543df19646ad/global.go:42\nmain.main\n\t/Users/chauncy/code/goPath/src/github.com/pingcap/dumpling/cmd/dumpling/main.go:191\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:203"]

arguments:

Running command: ['/Users/chauncy/code/goPath/src/github.com/pingcap/dumpling/bin/dumpling', '-T', 'TICASE_3204_WTHYZC.dumpsnap', '--dump-empty-database=false', '--snapshot', '2020-06-30 18:39:03', '-h', '127.0.0.1', '-P', '4000', '-u', 'root', '-p', '', '-o', '/var/folders/sr/_hc37vmj0db6ht4_w6rw34880000gn/T/tmptv4miiea']

@lichunzhu why is it %!s(MISSING)

@kennytm Before db.Exec this variable's values is "34359738368". Maybe after translation it becomes %!s(MISSING).

kennytm · 2020-06-30T10:54:22Z

v4/export/config.go

@@ -53,7 +53,7 @@ type Config struct {
 	FileType          string
 	EscapeBackslash   bool
 	DumpEmptyDatabase bool
-	TiDBMemQuotaQuery uint64
+	SessionParams     map[string]string


perhaps this should be

Suggested change

SessionParams map[string]string

SessionParams map[string]interface{}

so that we could assign a number to it

But we still have to transfer it to string for dsn 🤔

overvenus

LGTM

…SnapshotProblem

…pling into fixSnapshotProblem

kennytm

Rest LGTM

kennytm · 2020-06-30T11:58:14Z

v4/export/sql.go

-		dsn += fmt.Sprintf("&%s=%s", k, url.QueryEscape(v))
+		var s string
+		if str, ok := v.(string); ok {
+			s = wrapQuotation(str)


let's just wrap the quotation mark directly. the only case the str passes the strings.HasPrefix stuff is when user invokes dumpling as

./dumpling --snapshot "'stuff'"

which is so unnatural that there's no point supporting it.

Suggested change

s = wrapQuotation(str)

s = wrapStringWith(str, "'")

(the wrapBackTicks function probably has the same bug, let's check it in the future)

lichunzhu · 2020-06-30T12:17:34Z

@kennytm PTAL again

* hotfix snapshot * update code * fix again * use session param to reset db after setup * address comment * address comment

lichunzhu added 4 commits June 30, 2020 11:08

hotfix snapshot

f6efe67

update code

a155d58

fix again

da9987c

use session param to reset db after setup

346ce48

lichunzhu requested a review from kennytm June 30, 2020 09:56

Merge branch 'master' into fixSnapshotProblem

e8e6c4b

lichunzhu added the status/PTAL label Jun 30, 2020

kennytm reviewed Jun 30, 2020

View reviewed changes

overvenus previously approved these changes Jun 30, 2020

View reviewed changes

lichunzhu added 3 commits June 30, 2020 19:40

address comment

1b4c859

Merge branch 'master' of https://github.com/pingcap/dumpling into fix…

bcf9f32

…SnapshotProblem

Merge branch 'fixSnapshotProblem' of https://github.com/lichunzhu/dum…

b4e8178

…pling into fixSnapshotProblem

lichunzhu dismissed overvenus’s stale review via b4e8178 June 30, 2020 11:41

kennytm reviewed Jun 30, 2020

View reviewed changes

address comment

959641e

kennytm approved these changes Jun 30, 2020

View reviewed changes

kennytm added the status/LGT1 One reviewer approved (LGTM1) label Jun 30, 2020

kennytm merged commit ff92fcf into pingcap:master Jun 30, 2020

lichunzhu deleted the fixSnapshotProblem branch June 30, 2020 12:38

kennytm mentioned this pull request Oct 20, 2020

support session parameters #167

Merged

lichunzhu mentioned this pull request Apr 2, 2021

Add conn session vars #265

Merged

tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021

fix snapshot problem (pingcap#111)

3fb332a

* hotfix snapshot * update code * fix again * use session param to reset db after setup * address comment * address comment

tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021

fix snapshot problem (pingcap#111)

a2679df

* hotfix snapshot * update code * fix again * use session param to reset db after setup * address comment * address comment

tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021

fix snapshot problem (pingcap#111)

82d8465

* hotfix snapshot * update code * fix again * use session param to reset db after setup * address comment * address comment

tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021

fix snapshot problem (pingcap#111)

b275625

* hotfix snapshot * update code * fix again * use session param to reset db after setup * address comment * address comment

tisonkun pushed a commit to tisonkun/tidb that referenced this pull request Oct 20, 2021

fix snapshot problem (pingcap/dumpling#111)

b3e4ddd

* hotfix snapshot * update code * fix again * use session param to reset db after setup * address comment * address comment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix snapshot problem #111

fix snapshot problem #111

lichunzhu commented Jun 30, 2020 •

edited

codecov bot commented Jun 30, 2020 •

edited

kennytm left a comment

kennytm Jun 30, 2020

lichunzhu Jun 30, 2020

lichunzhu Jun 30, 2020

kennytm Jun 30, 2020

lichunzhu Jun 30, 2020 •

edited

kennytm Jun 30, 2020

lichunzhu Jun 30, 2020

overvenus left a comment

kennytm left a comment

kennytm Jun 30, 2020

lichunzhu Jun 30, 2020

lichunzhu commented Jun 30, 2020

	SessionParams map[string]string
	SessionParams map[string]interface{}

fix snapshot problem #111

fix snapshot problem #111

Conversation

lichunzhu commented Jun 30, 2020 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

codecov bot commented Jun 30, 2020 • edited

Codecov Report

kennytm 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

lichunzhu Jun 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overvenus left a comment

Choose a reason for hiding this comment

kennytm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichunzhu commented Jun 30, 2020

lichunzhu commented Jun 30, 2020 •

edited

codecov bot commented Jun 30, 2020 •

edited

lichunzhu Jun 30, 2020 •

edited