Adjust models to properly support MySQL - #51
Conversation
| return name, path, args, kwargs | ||
|
|
||
|
|
||
| class MultiCollationCharField(MultiCollationMixin, models.CharField): |
There was a problem hiding this comment.
@bradenmacdonald, @kdmccormick, @feanil: May I get a sanity check from one of you folks on whether this class (and the mixin above it that does all the work) makes sense?
There was a problem hiding this comment.
Makes sense to me!
Though btw, is there a compelling reason to run tests in sqlite? I generally feel like it's better to test as your run in prod, which would mean using MySQL for tests, which would avoid most of the concerns here.
There was a problem hiding this comment.
Maybe this is silly on my part, but I'd like to hold the door open for sqlite on the low end. I have this vague idea of a light configuration that uses sqlite for everything.
Though I do agree that testing in MySQL would be helpful.
| ), | ||
| ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
| ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), | ||
| ('key', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=1000)), |
There was a problem hiding this comment.
@bradenmacdonald, @kdmccormick, @feanil: This is how the migration gets written out.
ormsbee
changed the title
d67627e to
6fc1475
Compare
This codebase was originally developed and tested using only SQLite. In this commit, we're adding proper support for MySQL. In particular: * The size of ``key`` fields and titles were reduced from 1000 to 500. This is to accommodate MySQL's index size limit (3072 bytes which translates to 768 unicode code points in 4-byte encoding). Saving a little headroom for future compound indexes. * fields.py now has helper classes to support multiple collations so that we can specify utf8mb4 for the charset in MySQL. * fields.py now has helper functions that allow us to normalize case sensitivity across databases. By default, fields would otherwise be case sensitive in SQLite and case insensitive in MySQL. This is important for correctness, since ``key`` fields are meant to be be case sensitive for the purposes of uniqueness constraints.
6fc1475 to
f964832
Compare
ormsbee
changed the title
| def use_compressed_table_format(apps, schema_editor): | ||
| """ | ||
| Use the COMPRESSED row format for TextContent if we're using MySQL. | ||
|
|
||
| This table will hold a lot of OLX, which compresses very well using MySQL's | ||
| built-in zlib compression. This is especially important because we're | ||
| keeping so much version history. | ||
| """ | ||
| if schema_editor.connection.vendor == 'mysql': | ||
| table_name = apps.get_model("oel_contents", "TextContent")._meta.db_table | ||
| sql = f"ALTER TABLE {table_name} ROW_FORMAT=COMPRESSED;" | ||
| schema_editor.execute(sql) |
There was a problem hiding this comment.
For any reviewers: this is the main new thing here, manually added.
| # Call out to custom code here to change row format for TextContent | ||
| migrations.RunPython(use_compressed_table_format, reverse_code=migrations.RunPython.noop, atomic=False), |
There was a problem hiding this comment.
This is where we call the new function for creating compressed tables.
| uuid = immutable_uuid_field() | ||
| key = key_field() | ||
| title = models.CharField(max_length=1000, null=False, blank=False) | ||
| title = case_insensitive_char_field(max_length=500, blank=False) |
There was a problem hiding this comment.
I can't get the publishing API tests to pass on MySQL? Errors are:
FAILED tests/openedx_learning/core/publishing/test_api.py::CreateLearningPackageTestCase::test_auto_datetime - django.db.utils.OperationalError: (1366, "Incorrect string value: '\\xF0\\x9F\\x94\\xA5' for column 'title' at row 1")
FAILED tests/openedx_learning/core/publishing/test_api.py::CreateLearningPackageTestCase::test_normal - django.db.utils.OperationalError: (1366, "Incorrect string value: '\\xF0\\x9F\\x94\\xA5' for column 'title' at row 1")
I've configured the tests to run against a mysql docker container running with --character-set-server=utf8mb4 --collation-server=utf8mb4_bin, and everything on the test database looks like it should be ok, but still the tests fail.. am I missing something?
cf https://stackoverflow.com/q/10957238
mysql> use oel_test;
mysql> show full columns from oel_publishing_learningpackage;
+---------+--------------+--------------------+------+-----+---------+----------------+---------------------------------+---------+
| Field | Type | Collation | Null | Key | Default | Extra | Privileges | Comment |
+---------+--------------+--------------------+------+-----+---------+----------------+---------------------------------+---------+
| id | bigint(20) | NULL | NO | PRI | NULL | auto_increment | select,insert,update,references | |
| uuid | char(32) | utf8mb4_bin | NO | UNI | NULL | | select,insert,update,references | |
| key | varchar(500) | utf8mb4_bin | NO | UNI | NULL | | select,insert,update,references | |
| title | varchar(500) | utf8mb4_unicode_ci | NO | | NULL | | select,insert,update,references | |
| created | datetime(6) | NULL | NO | | NULL | | select,insert,update,references | |
| updated | datetime(6) | NULL | NO | | NULL | | select,insert,update,references | |
+---------+--------------+--------------------+------+-----+---------+----------------+---------------------------------+---------+
6 rows in set (0.00 sec)
mysql> show variables like 'character%';
+--------------------------+----------------------------+
| Variable_name | Value |
+--------------------------+----------------------------+
| character_set_client | utf8mb4 |
| character_set_connection | utf8mb4 |
| character_set_database | utf8mb4 |
| character_set_filesystem | binary |
| character_set_results | utf8mb4 |
| character_set_server | utf8mb4 |
| character_set_system | utf8 |
| character_sets_dir | /usr/share/mysql/charsets/ |
+--------------------------+----------------------------+
(That character_set_system: utf8 doesn't seem to be the problem, cf https://stackoverflow.com/a/55957424)
There was a problem hiding this comment.
The Django connection also has to specify utf8mb4, i.e. DATABASES['default']['OPTIONS']['charset'] = 'utf8mb4'.
There was a problem hiding this comment.
Ah cool -- I was using the DATABASES["TEST"] stanza, not DATABASES["OPTIONS"]. That worked!
pomegranited
left a comment
pomegranited
left a comment
There was a problem hiding this comment.
👍 Thank you for providing these foundational fields and capabilities @ormsbee ! Works great.
- I tested this by modifying
test_settings.pyto run the tests for this repo against a localmysqlinstance. - I read through the code
- Includes documentation
99e88bc to
c448a48
Compare
This will run all CI tests against MySQL 8.0. It changes the tox
py38-django{32|42} envs to use a new MySQL settings file. Running pytest
manually will continue to use the in-memory SQLite database.
c448a48 to
7aa2776
Compare
3 participants
This codebase was originally developed and tested using only SQLite. In this commit, we're adding proper support for MySQL. In particular:
keyfields and titles were reduced from 1000 to 500. This is to accommodate MySQL's index size limit (3072 bytes which translates to 768 unicode code points in 4-byte encoding). Saving a little headroom for future compound indexes.fields.pynow has helper classes to support multiple collations so that we can specify utf8mb4 for the charset in MySQL.fields.pynow has helper functions that allow us to normalize case sensitivity across databases. By default, fields would otherwise be case sensitive in SQLite and case insensitive in MySQL. This is important for correctness, sincekeyfields are meant to be be case sensitive for the purposes of uniqueness constraints.Notes for Reviewers
Most of the migration file changes are just auto-generated, because I'm remaking the initial migrations instead of adding new ones. FWIW, this should be the last PR that blows away existing migration files to replace them, since we're going to be making this an installable package soon and making it part of the edx-platform install.
Most of the weird magic happens in the new
collations.pymodule.I'm not sure how to plug MySQL into CI to properly test the model behavior. Not sure if I should try to tack that into another commit on this PR, or to do in a follow-on.