feat!: upgrade mysql charset and collation to utf8mb4 #1065

Danyal-Faheem · 2024-05-16T13:21:47Z

fixes #938.

Changes

Changes the default character set of MySQL to utf8mb4 and the default collation to utf8mb4_unicode_ci.
Also updated the dosqlshell command to use utf8mb4 as the default character set.
Added a new do command tutor local do change-charset-collation to allow users to upgrade the charset and collation of their tutor installations
- Users can upgrade all the tables at once
- Users can choose to upgrade only certain tables or apps
- Users can also choose to upgrade all bar certain tables or apps

Testing

I have tested these changes on tutor dev and tutor K8s. I followed these steps:
- Ran tutor [dev|k8s] launch
- Imported the demo course and made sure it was working fine
- Created admin and student users successfully
- Created a new course with emojis in the title and in subsection titles and made sure changes were being published and reflected in the LMS

changelog.d/20240520_150131_danyal.faheem_upgrade_mysql_utf8mb4.md

DawoudSheraz · 2024-05-29T06:31:42Z

docs/local.rst

+
+Your database's charset and collation might not support specific characters or emojis. To upgrade your charset and collation of all the tables in your database to utf8mb4 and utf8mb4_unicode_ci respectively, run::
+
+    tutor local do change-charset-collation --all-tables --charset=utf8mb4 --collation=utf8mb4_unicode_ci
+
+Alternatively, if you only want to upgrade certain tables or exclude certain tables, you can use the `status` option. To upgrade the `courseware_studentmodule` and `courseware_studentmodulehistory` tables, run::
+
+    tutor local do change-charset-collation --status=include courseware_studentmodule courseware_studentmodulehistory
+
+Tutor performs pattern matching from the start of the table name so you can just enter the name of the app to include/exclude all the tables under that app. To upgrade all the tables in the database except the ones under the student and wiki apps, run::
+
+    tutor local do change-charset-collation --status=exclude student wiki


We should specify that the change is not reversible.

I've added a warning note at the top.

…tion string

DawoudSheraz · 2024-06-04T09:48:20Z

tutor/commands/upgrade/common.py

@@ -66,3 +66,55 @@ def get_mongo_upgrade_parameters(
    mv '/openedx/data/ora2/SET-ME-PLEASE (ex. bucket-name)' /openedx/data/ora2/openedxuploads
 fi
 """
+
+def get_mysql_change_charset_query(


nit: we should move this util to another file as we are not using this in upgrade anymore.

regisb · 2024-06-04T10:13:46Z

docs/local.rst

+
+Your database's charset and collation might not support specific characters or emojis. To upgrade your charset and collation of all the tables in your database to utf8mb4 and utf8mb4_unicode_ci respectively, run::
+
+    tutor local do change-charset-collation --all-tables --charset=utf8mb4 --collation=utf8mb4_unicode_ci


In which situation(s) will users have to run this command? Is it for users upgrading from Quince? Is it optional?

Yes, they can run it after upgrading from Quince. It is optional as tutor runs fine without it unless you add specific characters such as emojis that will throw DB errors.

That explanation should be present in the docs :)

regisb · 2024-06-04T10:14:46Z

docs/local.rst

+
+    tutor local do change-charset-collation --all-tables --charset=utf8mb4 --collation=utf8mb4_unicode_ci
+
+Alternatively, if you only want to upgrade certain tables or exclude certain tables, you can use the `status` option. To upgrade the `courseware_studentmodule` and `courseware_studentmodulehistory` tables, run::


All verbatim comments need double backticks. E.g: ``status``.

regisb · 2024-06-04T10:19:36Z

tutor/commands/jobs.py

+    context_settings={"ignore_unknown_options": True},
+)
+@click.option("--all-tables", is_flag=True, help="change all tables in the openedx database")
+@click.option("-s", "--status", type=click.Choice(['include', 'exclude'], case_sensitive=False), help="Whether to include or exclude certain tables/apps")


This combination of options is not very intuitive. It means that the end users needs to specify at least one option -- and that is not a great pattern, as options are considered... optional :)

Instead, I suggest that:

all tables are process by default.

If --include is specified, then only those tables are processed.

If --exclude is specified, then those tables are not processed.

Note that with this approach, --include and --exclude are mutually compatible (--exclude would override --include).

all tables are process by default

I did not want to make this the default scenario in case someone accidentally executes the command while trying out the different options provided and they did not want to upgrade all the tables since this is a potentially irreversible change. Upgrading all tables at once can also take a long time.

Maybe we can prompt the user if they wish to proceed?

Note that with this approach, --include and --exclude are mutually compatible (--exclude would override --include).

I think this can be achieved and sounds more plausible. I'll update the options.

Maybe we can prompt the user if they wish to proceed?

Yes, this sounds perfect. Please make sure to include a -I/--non-interactive option for headless scripts.

regisb · 2024-06-04T10:23:41Z

tutor/commands/jobs.py

+@click.command(
+    short_help="Upgrade mysql to a specific charset and collation",
+    help=(
+        "Upgrade mysql to a specific charset and collation. You can either upgrade all tables, specify only certain tables to upgrade or specify certain tables to exclude from the upgrade process"


This docstring is misleading. It implies that it allows the user to upgrade to just any charset/collation. Instead, I suggest that we rename this command to something like convert-mysql-utf8mb4-charset.

The original command was meant to allow users to upgrade to any charset/collation, however that caused issues with the connection string. However, since we have removed that option, I think the convert-mysql-utf8mb4-charset makes more sense. I'll update it.

ormsbee · 2024-06-05T03:29:54Z

tutor/commands/upgrade/common.py

+
+                    SET FOREIGN_KEY_CHECKS = 0;
+                    Select _table_name;
+                    SET @statement = CONCAT('ALTER TABLE ', _table_name, ' CONVERT TO CHARACTER SET {charset} COLLATE {collation};'); 


Will this change columns that currently use utf8mb4_bin collation to be utf8mb4_unicode_ci? If so, that's a problem. The relatively small number of columns that use utf8mb4_bin are usually doing so because they explicitly want the values to be case sensitive, because case insensitivity on things like course keys has given us a lot of grief in the past.

To clarify that last statement, an example was that MongoDB and the Python layer of CourseKeys are case sensitive, but the key stored in our CourseOverview model is not case sensitive. So at one point we had two course runs in production that had keys that differed only by case (because someone had misspelled when doing the initial creation), and updates to one could clobber the values for the other because they were fighting over the same row in CourseOverview for updates. (The inactive course run was still getting automated push updates from an external system.)

I realize that fixing CourseOverview is well outside the scope of this work, and I'm not trying to suggest we tackle that here. I only put it out there as an example of why apps might want to explicitly specify a sensitive field for storing things like identifier keys.

Will this change columns that currently use utf8mb4_bin collation to be utf8mb4_unicode_ci?

It seems this is the case. I think we can add a condition that columns with the utf8mb4_bin collation should not be upgraded. Any other specific charsets or collations that we should look out for here?

Or we can just upgrade all the columns that are using utf8mb3_general_ci so that if some other column has their charset/collation fixed by the model in the future, we don't have to update the check for them.

Based on your findings, it seems like the only forced collations that are not utf8mb4_unicode_ci are:

utf8mb3_bin: This should be mapped into utf8mb4_bin, since that's the logically equivalent case-sensitive collation.

utf8mb4_bin: This should be preserved as-is, as discussed.

utf8mb4_general_ci: This would be mapped to utf8mb4_unicode_ci, but it looks like it only exists for the bundles app, and that app is unused in Redwood and removed entirely just after the Redwood cut, so it's fine to ignore or update it, depending on what's more convenient in the code.

I've updated the command to perform the following mappings instead:

utf8mb3_general_ci to utf8mb4_unicode_ci

utf8mb3_* to utf8mb4_* meaning that the utf8mb3_bin collations are converted to utf8mb4_bin now.

utf8mb4_bin: This should be preserved as-is, as discussed.

We ignore any columns that already have the utf8mb4 charset.

utf8mb4_general_ci: This would be mapped to utf8mb4_unicode_ci, but it looks like it only exists for the bundles app, and that app is unused in Redwood and removed entirely just after the Redwood cut, so it's fine to ignore or update it, depending on what's more convenient in the code.

Yup, trying it out on the nightly release, the bundles app was not there. Therefore, I've ignored it.

ormsbee · 2024-06-05T03:34:21Z

tutor/commands/upgrade/common.py

+                    LEAVE tables_loop;
+                    END IF;
+
+                    SET FOREIGN_KEY_CHECKS = 0;


Does this cause any issue when trying to do a partial migration that includes the course_overviews_courseoverview table? That's the only table I'm aware of that has a varchar primary key and foreign keys from other tables.

Danyal-Faheem mentioned this pull request May 20, 2024

We must migrate the mysql databases from utf8mb3 to utf8mb4 #938

Open

Danyal-Faheem marked this pull request as draft May 22, 2024 15:08

DawoudSheraz force-pushed the redwood branch 2 times, most recently from b203c94 to 938abca Compare May 28, 2024 06:18

Danyal-Faheem force-pushed the danyalfaheem/upgrade-mysql-utf8mb4 branch from 0e17056 to 80796da Compare May 28, 2024 16:49

Danyal-Faheem requested a review from DawoudSheraz May 28, 2024 16:54

feat!: upgrade mysql charset and collation to utf8mb4

438b312

Danyal-Faheem force-pushed the danyalfaheem/upgrade-mysql-utf8mb4 branch from 47f01f9 to 438b312 Compare May 28, 2024 17:17

DawoudSheraz reviewed May 29, 2024

View reviewed changes

changelog.d/20240520_150131_danyal.faheem_upgrade_mysql_utf8mb4.md Outdated Show resolved Hide resolved

DawoudSheraz reviewed May 29, 2024

View reviewed changes

fix: remove charset, collation arguments due to conflicts with connec…

f23e4f6

…tion string

Danyal-Faheem mentioned this pull request May 30, 2024

Supply mysql8.4.0 by default #1071

Open

Danyal-Faheem marked this pull request as ready for review June 3, 2024 10:35

fix: make requested changes

c5678ab

DawoudSheraz reviewed Jun 4, 2024

View reviewed changes

regisb reviewed Jun 4, 2024

View reviewed changes

ormsbee reviewed Jun 5, 2024

View reviewed changes

fix: update CLI options and modify charset at the column level

e8f60e8

Danyal-Faheem requested review from DawoudSheraz, regisb and ormsbee June 6, 2024 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: upgrade mysql charset and collation to utf8mb4 #1065

feat!: upgrade mysql charset and collation to utf8mb4 #1065

Danyal-Faheem commented May 16, 2024 •

edited

DawoudSheraz May 29, 2024

Danyal-Faheem Jun 3, 2024

DawoudSheraz Jun 4, 2024

regisb Jun 4, 2024

Danyal-Faheem Jun 4, 2024 •

edited

regisb Jun 4, 2024

regisb Jun 4, 2024

regisb Jun 4, 2024

Danyal-Faheem Jun 4, 2024

regisb Jun 4, 2024

regisb Jun 4, 2024

Danyal-Faheem Jun 4, 2024

ormsbee Jun 5, 2024

ormsbee Jun 5, 2024

Danyal-Faheem Jun 5, 2024

ormsbee Jun 5, 2024

Danyal-Faheem Jun 6, 2024

ormsbee Jun 5, 2024


		Your database's charset and collation might not support specific characters or emojis. To upgrade your charset and collation of all the tables in your database to utf8mb4 and utf8mb4_unicode_ci respectively, run::

		tutor local do change-charset-collation --all-tables --charset=utf8mb4 --collation=utf8mb4_unicode_ci


		tutor local do change-charset-collation --all-tables --charset=utf8mb4 --collation=utf8mb4_unicode_ci

		Alternatively, if you only want to upgrade certain tables or exclude certain tables, you can use the `status` option. To upgrade the `courseware_studentmodule` and `courseware_studentmodulehistory` tables, run::

feat!: upgrade mysql charset and collation to utf8mb4 #1065

Are you sure you want to change the base?

feat!: upgrade mysql charset and collation to utf8mb4 #1065

Conversation

Danyal-Faheem commented May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danyal-Faheem Jun 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danyal-Faheem commented May 16, 2024 •

edited

Danyal-Faheem Jun 4, 2024 •

edited