This repository has been archived by the owner on Aug 21, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 85
*: cherry-pick recent PRs for dumpling v5.0.1 #271
Merged
Merged
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
1677aea
Add conn session vars (#265)
recall704 7f739b1
add estimate total rows count to metrics for dump table (#254)
recall704 f47b9f6
sql: fix table sample sql generator bug (#262)
lichunzhu 7d1fccd
export: fix a metric not using labelNames (#266)
lance6716 236f366
export: fix estimateTotalRowsCounter nil pointer (#267)
lichunzhu fb89ccc
log: replace pingcap global logger when start dumpling binary (#270)
lichunzhu 1b33d20
fix bug
lichunzhu 278a128
add integration test
lichunzhu 0f32fad
fix integration test
lichunzhu ba962eb
Revert "fix bug"
lichunzhu a7c8c08
tmp
lichunzhu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here using
?
will let sql driver to check the type ofpv
, to decide ifpv
need quotes.Instead, I prefer always use the input value as-is, that is to say we always use
%s
and printf to construct a SQL. So user should quote the value if it's a string type variable in SQL.https://github.com/go-sql-driver/mysql/blob/bcc459a906419e2890a50fc2c99ea6dd927a88f2/connection.go#L68-L78
(I deleted previous misleading comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might append
--param
to dsn 😂 otherwise we can't control https://github.com/go-sql-driver/mysql#parameters and have to write a doc about dealing with,
in parameter KVs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conf.SessionParams
is amap[string]interface{}
. I think using an original value to construct SQL is in-proper for being used as a library.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the detailed in-proper case?
if user input
key=val
, successful parsingval
as a integer does not meankey
is an interger MySQL variable, at that time it will raise an error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that
SET SESSION X = 123
andSET SESSION X = '123'
are equivalent for system variables.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lance6716 @kennytm
Fixed in 1b33d20, and add an integration test in 278a128. PTAL again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I didn't reply to above comment in time 😂 I didn't found a case for system variables that
SET SESSION X = 123
will cause error if X is a string type variable. Will check it now.