django-celery-beat
django-celery-beat copied to clipboard
celery
Fix max_length issue for mysql
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.
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 Yes it should work since it defaults to the original max_length to maintain backwards compatibility.
is it possible to test the change to check it won't regress? @tinylambda could you check this PR?
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 ？
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
@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 Are you suggesting, removing the
max_length=200restriction altogether?hmmm, not sure if that will have some other side-effects as a result.
I means removing unique=True
@tinylambda i dont think the issue is with the uniqueness constraint. its with the max length value being too large for mysql.
@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 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
@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 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.
@tinylambda I feel that might be a bit overkill. Any issues with the current approach?
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 https://github.com/milind-shakya-sp/django-celery-beat/pull/1 in your PR to prove the backwards compatibility with my tests.
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 :-)
will it help? the last master build also failed: https://travis-ci.org/celery/django-celery-beat/builds/513951725?utm_source=github_status&utm_medium=notification
rebased but got some new errors. will look at them after sometime unless @tuky wants to take a stab at them first.
So, I did a little research on this. and here are the options from what I can see.
- 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.
- 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).
- 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.
- 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.
