Fix known mysql type conversion issues #6647
Conversation
| // Ignore ErrRange. When this error is set the returned value is "the | ||
| // maximum magnitude integer of the appropriate bitSize and sign." | ||
| if err, ok := err.(*strconv.NumError); ok && err.Err == strconv.ErrRange { | ||
| return v, nil |
There was a problem hiding this comment.
Are we expecting ints bigger than int64 or is this here to be defensive?
There was a problem hiding this comment.
Expecting larger integers, I believe this is actually the cause of the 5529 bug. In the old strategy we tried to parse as an integer and then fell back to a float64, causing the conflict. In theory we also have a unsigned integer type that we could work with, but there isn't a clean way to migrate to it.
| return int64(1), nil | ||
| case "OFF_PERMISSIVE": | ||
| return int64(0), nil | ||
| case "ON_PERMISSIVE": |
There was a problem hiding this comment.
Is the permissive aspect of this mode not important? I'm a little concerned that we're throwing away information. Is that just for backwards compatibility?
There was a problem hiding this comment.
I did do this for backwards compatibility, but since we are converting to an integer we could add a 2/3 value. This would mean we need to document the meaning of these values as they would be somewhat arbitrary.
plugins/inputs/mysql/v2/convert.go
Outdated
|
|
||
| var GlobalStatusConversions = map[string]ConversionFunc{ | ||
| "ssl_ctx_verify_depth": ParseInt, | ||
| } |
There was a problem hiding this comment.
Issue #5529 also mentions type problems with ssl_verify_depth. Shouldn't that also have an entry here?
There was a problem hiding this comment.
Yes, I'll fix this.
plugins/inputs/mysql/v2/convert.go
Outdated
| var GlobalVariableConversions = map[string]ConversionFunc{ | ||
| "gtid_mode": ParseGTIDMode, | ||
| } | ||
|
|
There was a problem hiding this comment.
gtid_mode, ssl_ctx_verify_depth, ssl_verify_depth are the fields that have caused issues for us but I wonder if there are others that will be a problem in the future. Were you able to find a list of these changes in mysql API docs for instance? I wonder if adding them one at a time will be a whack-a-mole style headache that we could prevent by finding all the changes before they turn into issues.
There was a problem hiding this comment.
I agree this is going to be whack-a-mole, but I just checked and I don't see a document describing changes in this table. I believe the table is designed more for interactive use than how we are using it.
Perhaps it would make sense to whitelist in the known variables instead of trying to parse them dynamically? I also have some concerns about breaking compatibility if I get too aggressive changing the parse behavior of the fields though.
(cherry picked from commit d858d82)
(cherry picked from commit d858d82)
Add the ability to have per field conversion functions for the global status and global variables queries. These queries both return tables of the form:
closes #5529
closes #6646
Required for all PRs: