-
-
Notifications
You must be signed in to change notification settings - Fork 238
fix: make SET system type case-insensitive #2702
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
Conversation
jycor
left a 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.
Thanks for the contribution!
MySQL does use the utf8mb4_0900_ai_ci collation for @@sql_mode, so we should change that, but only that variable and not all system set type
sql/types/system_set.go
Outdated
| // NewSystemSetType returns a new systemSetType. | ||
| func NewSystemSetType(varName string, values ...string) sql.SystemVariableType { | ||
| return systemSetType{MustCreateSetType(values, sql.Collation_Default), varName} | ||
| return systemSetType{MustCreateSetType(values, sql.Collation_ascii_general_ci), varName} |
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 shouldn't change all our system types to use that collation, and they should still use collation_default. Also it should be utf8mb4_0900_ai_ci and not utf8mb4_ascii_general_ci.
Instead, NewSystemSetType() should take in a sql.CollationID and we should pass it sql.Collation_utf8mb4_0900_ai_ci for sql_mode in go-msql-server/sql/variables/system_variables.go.
Fortunately, NewSystemSetType() is only used in a few places, so there won't be too many changes.
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.
Hi @jycor, thanks for your review! I'm uncertain about which collation should be used for other system variables. Could you please direct me to where I can find MySQL's implementation?
mysql> set global log_output = 'table,file';
Query OK, 0 rows affected (0.00 sec)
mysql> select @@global.log_output;
+---------------------+
| @@global.log_output |
+---------------------+
| FILE,TABLE |
+---------------------+
1 row in set (0.00 sec)
mysql> set global log_output = default;
Query OK, 0 rows affected (0.00 sec)
mysql> select @@global.log_output;
+---------------------+
| @@global.log_output |
+---------------------+
| FILE |
+---------------------+
1 row in set (0.00 sec)
mysql> set global protocol_compression_algorithms = 'zLIB,Zstd';
Query OK, 0 rows affected (0.01 sec)
mysql> select @@global.protocol_compression_algorithms;
+------------------------------------------+
| @@global.protocol_compression_algorithms |
+------------------------------------------+
| zLIB,Zstd |
+------------------------------------------+
1 row in set (0.01 sec)
mysql> set global protocol_compression_algorithms = default;
Query OK, 0 rows affected (0.00 sec)
mysql> select @@global.protocol_compression_algorithms;
+------------------------------------------+
| @@global.protocol_compression_algorithms |
+------------------------------------------+
| zlib,zstd,uncompressed |
+------------------------------------------+
1 row in set (0.00 sec)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 have learned the following facts from MySQL's source code:
protocol_compression_algorithmsis defined to be aSys_var_charptrand is validated using the case-insensitive collation oflatin1.sql_modeandlog_outputare defined asSys_var_setand are parsed using theto_upperfunction ofsystem_charset_info.
jycor
left a 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.
Thanks for the changes and even digging into the MySQL source code.
LGTM!
Resolves #2701