-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add support of utf8mb4 for mysql #6992
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6992 +/- ##
=========================================
- Coverage 41.51% 41.5% -0.01%
=========================================
Files 440 440
Lines 59457 59459 +2
=========================================
- Hits 24683 24678 -5
- Misses 31556 31561 +5
- Partials 3218 3220 +2
Continue to review full report at Codecov.
|
connStr = fmt.Sprintf("%s:%s@%s(%s)/%s%scharset=utf8&parseTime=true&tls=%s", | ||
DbCfg.User, DbCfg.Passwd, connType, DbCfg.Host, DbCfg.Name, Param, tls) | ||
connStr = fmt.Sprintf("%s:%s@%s(%s)/%s%scharset=%s&parseTime=true&tls=%s", | ||
DbCfg.User, DbCfg.Passwd, connType, DbCfg.Host, DbCfg.Name, Param, DbCfg.Charset, tls) |
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.
Whilst we're changing this it might be sensible to escape these parts.
Co-Authored-By: zeripath <art27@cantab.net>
Co-Authored-By: zeripath <art27@cantab.net>
Co-Authored-By: zeripath <art27@cantab.net>
@zeripath done. |
I take it there is no way to migrate existing |
@silverwind migrate from utf8 is possible but it’s out of scope of this PR. |
Approved, but should we make breaking change and default to utf8mb4? |
@techknowlogick That could be another PR. Of course, I think utf8mb4 as default is more reasonable. |
Simplest InnoDB version check according to: https://www.fromdual.com/innodb-version SHOW GLOBAL VARIABLES LIKE 'innodb_ver%';
|
Ok fancy putting a pr in to test this? |
The real thing to test is: By default there is a 767 byte limit on indexes, which means that you will exceed the index limit length if you have varchar(255) with a 4 byte encoding like utf8mb4 (255 * 4 = 1020). This is fixed by the innodb_large_prefix setting which was introduce in 5.6.3: https://dev.mysql.com/doc/refman/5.6/en/innodb-parameters.html#sysvar_innodb_large_prefix This is still off by default, but many hosts turn it on. If testing I think the right thing is to test if innodb_large_prefix is set to either 1 or on (both syntax are supported).
More complicated, this feature only works when the ROW_FORMAT is COMPRESSED or DYNAMIC. The Default for MySQL 5.6 was COMPACT and 5.7 is Dynamic -- so it wouldn't work right on 5.6 unless whatever created the rows also specified ROW_FORMAT=DYNAMIC|COMPRESSED in the create statement (in addition to using innodb_large_prefix) . To simplify, it might be easiest to require 5.7+ and then check the innodb_large_prefix value to make sure it is 1|ON. Also it will just work without these settings, and the failure would then only happen if somebody entered a string that was more than 191 characters into one of the index fields, with |
A few weeks ago, I followed the conversion steps of the Nextcloud database as seen here: https://docs.nextcloud.com/server/16/admin_manual/configuration_database/mysql_4byte_support.html Maybe it can be useful for you guys, there were a few SQLs I needed to run, and also an SQL query generator SQL query 😃 FYI, the above InnoDB version you can see in the previous comment is from a MariaDB install on Ubuntu. I don't know how to get 5.7, |
Keep note that the
|
This PR will allow you change mysql's charset to
utf8mb4
. This may require your InnoDB greater than 5.6 as @philfry said on #3516 (comment), but gitea will not check mysql's version currently. Users should know their database version themselves.should fix #5660, #6988 & #2711