Skip to content
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

Fix max_length issue for mysql #202

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

milind-shakya-sp
Copy link
Contributor

@milind-shakya-sp milind-shakya-sp commented Dec 3, 2018

Current issues with mysql + django-celery-beat

If you want to run django-celery-beat with MySQL, you might run into some issues.
One such issue is when you try to run python manage.py migrate django_celery_beat, you might get the following error

 django.db.utils.OperationalError: (1071, 'Specified key was too long; max key length is 767 bytes')

To get around this issue, with this pr, we can set the following in the settings file

DJANGO_CELERY_BEAT_NAME_MAX_LENGTH=191
(or any other value if any other db other than MySQL is causing similar issues.)
max_length of 191 seems to work for MySQL.

@milind-shakya-sp milind-shakya-sp force-pushed the fix_max_length_mysql_issue branch 2 times, most recently from 0da676f to 2184d6d Compare December 3, 2018 23:22
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you rebase and regen the migrations to a backward compatible way?

@milind-shakya-sp
Copy link
Contributor Author

Rebased. @auvipy I agree in most cases, we would'nt update an existing migration and instead create a new one, but in this case, its required to update the existing migration, since without it, things will break for MySQL, when it tries to run the first migration. This way it will work for all databases.

@auvipy
Copy link
Member

auvipy commented Dec 5, 2018

could you check this works in a backward compatible way?

@milind-shakya-sp
Copy link
Contributor Author

@auvipy Yes it should work since it defaults to the original max_length to maintain backwards compatibility.

@auvipy
Copy link
Member

auvipy commented Dec 7, 2018

is it possible to test the change to check it won't regress? @tinylambda could you check this PR?

@tinylambda
Copy link
Contributor

tinylambda commented Dec 10, 2018

is it possible to test the change to check it won't regress? @tinylambda could you check this PR?

what about removing the db constraints to avoid the MySQL's limitation, instead, we use Django model's signal mechanism pre_save to avoid duplication at the framework level ?

@tinylambda
Copy link
Contributor

is it possible to test the change to check it won't regress? @tinylambda could you check this PR?

what about removing the db constraints to avoid the limitation, instead, we use Django model's signal mechanism pre_save to avoid duplication?

@milind-shakya-sp

@milind-shakya-sp
Copy link
Contributor Author

@tinylambda Are you suggesting, removing the max_length=200 restriction altogether?

hmmm, not sure if that will have some other side-effects as a result.

@tinylambda
Copy link
Contributor

tinylambda commented Dec 11, 2018

@tinylambda Are you suggesting, removing the max_length=200 restriction altogether?

hmmm, not sure if that will have some other side-effects as a result.

I means removing unique=True

@milind-shakya-sp
Copy link
Contributor Author

@tinylambda i dont think the issue is with the uniqueness constraint. its with the max length value being too large for mysql.

@tinylambda
Copy link
Contributor

@tinylambda i dont think the issue is with the uniqueness constraint. its with the max length value being too large for mysql.

You need this: https://stackoverflow.com/questions/1814532/1071-specified-key-was-too-long-max-key-length-is-767-bytes?answertab=active#tab-top

unique=True will create an index on the column.

@tinylambda
Copy link
Contributor

@tinylambda i dont think the issue is with the uniqueness constraint. its with the max length value being too large for mysql.

You need this: https://stackoverflow.com/questions/1814532/1071-specified-key-was-too-long-max-key-length-is-767-bytes?answertab=active#tab-top

unique=True will create an index on the column.

Any update ? @milind-shakya-sp

@milind-shakya-sp
Copy link
Contributor Author

@tinylambda I haven't had the chance to test the changes when we remove unique=True, but don't we actually want unique=True, since that will prevent PeriodicTask with duplicate names?

@tinylambda
Copy link
Contributor

@tinylambda I haven't had the chance to test the changes when we remove unique=True, but don't we actually want unique=True, since that will prevent PeriodicTask with duplicate names?

What about adding another column to store the md5sum of the name, and add unique=True on this column, because md5sum is fixed-length (32 chars), its behavior is predictable.

@milind-shakya-sp
Copy link
Contributor Author

@tinylambda I feel that might be a bit overkill. Any issues with the current approach?

@tuky tuky mentioned this pull request Feb 11, 2019
@tuky
Copy link

tuky commented Feb 11, 2019

What about adding another column to store the md5sum of the name, and add unique=True on this column, because md5sum is fixed-length (32 chars), its behavior is predictable.

I can come up with only one quite artificial advantage of this approach: we don't lose eight characters for MySQL utf8mb4 users (they cannot use the app in its current form anyways).

but we would still have to touch the existing migrations, because they just won't work for utf8mb4. I agree, that the md5 approach would be overkill, involving lots of refactoring and assuring in tests, that we never forget to fill in the value. it would also require a data migration for existing projects to calculate the md5.

in a manual review i cannot find any backwards compatibility breaking changes. existing installations of this app won't see any changes. and new installations behave the same, too.

Maybe @milind-shakya-sp you can include milind-shakya-sp#1 in your PR to prove the backwards compatibility with my tests.

@tuky
Copy link

tuky commented Feb 15, 2019

It's quite unfortunate, the build failed just because of an unrelated RemovedInSphinx20Warning. Apart from that you can now see in the build logs that we successfully tested the new setting :-)

@milind-shakya-sp milind-shakya-sp force-pushed the fix_max_length_mysql_issue branch 2 times, most recently from f74dcc9 to 30ed551 Compare March 4, 2019 21:33
auvipy
auvipy previously approved these changes Mar 12, 2019
@auvipy
Copy link
Member

auvipy commented Apr 23, 2019

could you please rebase? and check the Travis?

@tuky
Copy link

tuky commented Apr 23, 2019

@milind-shakya-sp
Copy link
Contributor Author

rebased but got some new errors. will look at them after sometime unless @tuky wants to take a stab at them first.

help_text='Useful description',
max_length=getattr(
settings,
'DJANGO_CELERY_BEAT_NAME_MAX_LENGTH',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa... you can't base migrations off of a settings option that people can change, that makes things completely unpredictable. Just choose a more correct value, one that won't ever have the issue again.

Copy link

@tuky tuky May 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa...

Can you elaborate on your surprise?

  1. The code is not "unpredictable", the defaults are backwards compatible
  2. Projects with even more stars follow this pattern (https://github.com/python-social-auth/social-app-django/blob/master/social_django/migrations/0001_initial.py)
  3. Simply changing this to 191 would solve it for me, but everyone using this with e.g. postgresql would be facing a DB schema which differs from their migrations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liquidpele can't really choose "a more correct value" in this case since some values work for some databases and the same values cause errors in other databases.

As @tuky points out driving the max_length value from the settings is not a new pattern and its pretty well-established.

If there is another approach you have in mind, please share.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa... you can't base migrations off of a settings option that people can change, that makes things completely unpredictable. Just choose a more correct value, one that won't ever have the issue again.

IMHO this is allowed in some case... but we need to handle this with care. let us not rush on merging this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not that some values work and some don't, it's simply a limitation in mysql, one that is already fixed in newer versions where they increased the prefix limit for innodb tables.

One question I have, and @auvipy may know this... is the task_id ever NOT a uuid? I thought it was always a uuid, but a length of 255 there makes me think it could be other things too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do somehow agree with you. Need some analysis to answer your question properly

@@ -37,7 +37,8 @@ recreate = False
commands =
pip install -U https://github.com/celery/celery/zipball/master#egg=celery
pip install -U https://github.com/celery/kombu/zipball/master#egg=kombu
py.test -xv
py.test -xv --ignore=t/unit/test_models.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you ignoring the test you added?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new tests are run with the line below: py.test -xv -k 'ModelMigrationTests'. It is necessary, to run the new tests separately, because they override settings used in models and it is therefore necessary, to do some six.moves.reload_module calls in the tests, which breaks subsequent test runs, that rely on the same models.

@@ -70,4 +71,4 @@ usedevelop = true
commands =
pip install -U https://github.com/celery/celery/zipball/master#egg=celery
pip install -U https://github.com/celery/kombu/zipball/master#egg=kombu
py.test -x --cov=django_celery_beat --cov-report=xml --no-cov-on-fail
py.test -x --cov=django_celery_beat --cov-report=xml --no-cov-on-fail --ignore=t/unit/test_models.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to exclude things like this.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking about the consequences of the current migrations after this

help_text='Useful description',
max_length=getattr(
settings,
'DJANGO_CELERY_BEAT_NAME_MAX_LENGTH',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa... you can't base migrations off of a settings option that people can change, that makes things completely unpredictable. Just choose a more correct value, one that won't ever have the issue again.

IMHO this is allowed in some case... but we need to handle this with care. let us not rush on merging this.

@liquidpele
Copy link
Contributor

So, I did a little research on this. and here are the options from what I can see.

  1. I really dislike the use of settings that modify migration files, but I would be okay with merging based on the fact that django-celery-results already has the same hack.
  2. Since this is only an issue with mysql before 5.7, which is already 6 years old, so I also think it would be acceptable to say upgrade your mysql to something current, but the older versions are still supported on most platforms (mysql support grid).
  3. We could detect if we're on mysql, and if so automatically set the max length to 190, which is below the amount for the issue. This would be similar to using the django setting, but would keep control of the values on our end and make it "just work" rather than people having to google the error and setting a special setting for it.
  4. Make a custom migration that only sets unique on only the first 190 chars of the field if running on mysql. The length would remain 200, but it the unique constraint would apply to only the first 190 chars.

I think I like either 3 or 4 the most because then people wouldn't even hit the issue to begin with.

@auvipy auvipy self-assigned this Jan 18, 2021
@auvipy
Copy link
Member

auvipy commented Jan 20, 2021

upgrading minimal MySQL version should let us not make this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants