Support gbk Encoding To Fix #532 #533
Conversation
|
also related to #228 |
|
Thank you. Adding GBK will create a different problem as per https://github.com/go-sql-driver/mysql#interpolateparams. We would need to turn off
If the former, then we can conditionally disable |
|
I add an option |
|
Do you think you have the answer though? |
|
I think this PR only handles the case with For question 2, i am not sure about the solution. |
|
Thank you. Please be advised that #526 is to be merged (if all goes well), and will affect your PR once merged, since it also touches (and moves) MySQL URI definitions. |
Do you have any idea about this case? |
I don't... |
go/mysql/connection.go
Outdated
| return fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?interpolateParams=true&autocommit=true&charset=utf8mb4,utf8,latin1", this.User, this.Password, hostname, this.Key.Port, databaseName) | ||
| var multibyteCharset string | ||
| if includeMultibyte { | ||
| multibyteCharset = ",gbk,gb2312,big5,cp932,sjis" |
There was a problem hiding this comment.
is multibyteCharset a correct description? utf8 is also a multibyte charset. Is there a better definition?
There was a problem hiding this comment.
How about riskCharset and includeRiskCharset?
There was a problem hiding this comment.
fixed according to your comments.
go/mysql/connection.go
Outdated
| if includeMultibyte { | ||
| multibyteCharset = ",gbk,gb2312,big5,cp932,sjis" | ||
| } | ||
| return fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?interpolateParams=%t&autocommit=true&charset=utf8mb4,utf8,latin1%s", this.User, this.Password, hostname, this.Key.Port, databaseName, !includeMultibyte, multibyteCharset) |
There was a problem hiding this comment.
I'd create a variable called interpolateParams that happens to be set to !includeMultibyte, and use interpolateParams in the Sprintf statement. The logic to [not] use interpolateParams may end up considering additional inputs to the charset.
There was a problem hiding this comment.
Thanks
We need manual close interpolateParams if column charset with multibyte encodings like BIG5, CP932, GB2312, GBK or SJIS ?
riskCharset , Is there any risk for this fix ?
There was a problem hiding this comment.
There's no risk for disabling interpolateParams, it is a matter of performance.
|
resolve conflict after #526 merged |
|
Any progress about this issue ? |
|
@cenkore apologies for the delay! I hope to work on this in the next few days. |
shlomi-noach
temporarily deployed
to
production/mysql_role=ghost_testing
|
Sent to testing. |
Glad to see it. |
|
Can you please design a test (similar to https://github.com/github/gh-ost/tree/master/localtests/utf8mb4) that would express the support for the new changes? |
|
Ignore the above. I will use #532 as a test. |
|
Here's a funny thing: I added this test and it passes with a "normal" run, without adding the |
https://github.com/go-sql-driver/mysql/blob/master/dsn.go#L73 It seems that the validation in driver is on Perhaps, we do NOT need the flag. |
|
Superseded by #560 |
To fix #532
Description
This PR adds
gbkencoding support.