Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix variable formatting for ErrTruncateWrongValue #25

Merged
merged 4 commits into from
Nov 26, 2018

Conversation

alapha23
Copy link
Contributor

@alapha23 alapha23 commented Nov 8, 2018

System variables of mysql could be 50 characters at most according to the document, which is group_replication_flow_control_certifier_threshold.

Modify the ErrTruncateWrongValue so that system variable names will not be truncated in the error message.

We could presume error message will not exceed mysql limits, as there are longer error messages as the official document suggests, e.g.

Error number: 1041; Symbol: ER_OUT_OF_RESOURCES; SQLSTATE: HY000
Message: Out of memory; check if mysqld or some other process uses all available memory; if not, you may have to use 'ulimit' to allow mysqld to use more memory or you can add more swap space 

Please correct me if I misunderstood anything. Thank you in advance!

@shenli
Copy link
Member

shenli commented Nov 8, 2018

@alapha23 Could you post the error message from MySQL in that case?

@lysu
Copy link
Collaborator

lysu commented Nov 8, 2018

@alapha23 maybe also take a PR in tidb repo and add a unit test for

variable.ErrTruncatedWrongValue.GenWithStackByArgs("group_replication_flow_control_certifier_threshold", "1")

to make CI green and make tidb's PR referenece this one

test parser from tidb can use https://github.com/pingcap/parser/blob/master/README.md's method.

@alapha23
Copy link
Contributor Author

alapha23 commented Nov 8, 2018

@shenli @lysu
So this parser is relevant to this pr on tidb

As you might have seen from circleci that mysql system variable validate_password_mixed_case_count_value is truncated because of the error message formatting,

FAIL: set_test.go:283: testSuite.TestValidateSetVar
set_test.go:526:
    tk.MustQuery("show warnings").Check(testutil.RowsWithSep("|", "Warning|1292|Truncated incorrect validate_password_mixed_case_count value: '-1'"))
/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:55:
    res.c.Assert(resBuff.String(), check.Equals, needBuff.String(), res.comment)
... obtained string = "[Warning 1292 Truncated incorrect validate_password_mixed_case_cou value: '-1']\n"
... expected string = "[Warning 1292 Truncated incorrect validate_password_mixed_case_count value: '-1']\n"
... sql:show warnings, args:[]

I presume we are able to know the error message is decent or not by check ci of the tidb pr I mentioned above.

What do you think?
Thank you in advance!

@alapha23
Copy link
Contributor Author

alapha23 commented Nov 8, 2018

@lysu Thank you for suggesting the readme. I will take a careful look and follow the steps.

@tiancaiamao
Copy link
Collaborator

@lysu PTAL

@lysu
Copy link
Collaborator

lysu commented Nov 21, 2018

@shenli I have checked error message from MySQL in that case is same with this PR's result

@lysu
Copy link
Collaborator

lysu commented Nov 21, 2018

@alapha23 let me take a look..

@alapha23
Copy link
Contributor Author

@lysu
Thank you for the comment!
I encountered errors after switching the parser.
The parameter collate seems to differ between expected string and obtained string.

For example, make dev under tidb would have

expr_to_pb_test.go:327:
    c.Assert(string(js), Equals, jsons[funcNames[i]], Commentf("%v\n", funcNames[i]))
... obtained string = "{\"tp\":10000,\"children\":[{\"tp\":201,\"val\":\"gAAAAAAAAAE=\",\"sig\":0,\"field_type\":{\"tp\":5,\"flag\":0,\"flen\":-1,\"decimal\":-1,\"collate\":46,\"charset\":\"\"}},{\"tp\":201,\"val\":\"gAAAAAAAAAI=\",\"sig\":0,\"field_type\":{\"tp\":5,\"flag\":0,\"flen\":-1,\"decimal\":-1,\"collate\":46,\"charset\":\"\"}}],\"sig\":200,\"field_type\":{\"tp\":5,\"flag\":128,\"flen\":-1,\"decimal\":-1,\"collate\":63,\"charset\":\"binary\"}}"
... expected string = "{\"tp\":10000,\"children\":[{\"tp\":201,\"val\":\"gAAAAAAAAAE=\",\"sig\":0,\"field_type\":{\"tp\":5,\"flag\":0,\"flen\":-1,\"decimal\":-1,\"collate\":83,\"charset\":\"\"}},{\"tp\":201,\"val\":\"gAAAAAAAAAI=\",\"sig\":0,\"field_type\":{\"tp\":5,\"flag\":0,\"flen\":-1,\"decimal\":-1,\"collate\":83,\"charset\":\"\"}}],\"sig\":200,\"field_type\":{\"tp\":5,\"flag\":128,\"flen\":-1,\"decimal\":-1,\"collate\":63,\"charset\":\"binary\"}}"
... plus

There are quite a few errors of this kind and you could refer to the whole output message in log.txt

Thank you very much!

@lysu
Copy link
Collaborator

lysu commented Nov 22, 2018

@alapha23 hi, it seem pingcap/tidb#8227 CI passed

what means switching the parser ?

@alapha23
Copy link
Contributor Author

@lysu
CI passed because I removed the system variable which is longer than the limit. In that case I was using pingcap/parser

For switching the parser, I mean I was trying to use this modified parser for further testing.
Above error happened when using this modified parser.

What do you think of the error message?
Sorry for the trouble. And thank you for your consistent help.

@lysu
Copy link
Collaborator

lysu commented Nov 26, 2018

LGTM

@lysu lysu added the status/LGT1 LGT1 label Nov 26, 2018
@lysu
Copy link
Collaborator

lysu commented Nov 26, 2018

@tiancaiamao @yu34po PTAL

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zz-jason zz-jason added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Nov 26, 2018
@zz-jason zz-jason merged commit d6a0ca9 into pingcap:master Nov 26, 2018
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
lyonzhi pushed a commit to lyonzhi/parser that referenced this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants