Fix max_length issue for mysql #202

milind-shakya-sp · 2018-12-03T22:19:39Z

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.

auvipy

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

milind-shakya-sp · 2018-12-04T15:14:48Z

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 · 2018-12-05T16:41:13Z

could you check this works in a backward compatible way?

milind-shakya-sp · 2018-12-05T16:54:14Z

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

auvipy · 2018-12-07T18:26:02Z

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

tinylambda · 2018-12-10T03:01:42Z

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 · 2018-12-10T03:03:22Z

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 · 2018-12-10T15:46:48Z

@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 · 2018-12-11T02:04:36Z

@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 · 2018-12-11T03:53:44Z

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

tinylambda · 2018-12-11T06:45:57Z

@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 · 2018-12-12T04:20:47Z

@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 · 2018-12-12T15:49:07Z

@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 · 2018-12-13T03:08:50Z

@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 · 2018-12-13T16:29:35Z

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

tuky · 2019-02-11T10:35:54Z

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 · 2019-02-15T12:54:16Z

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 :-)

auvipy · 2019-04-23T03:25:52Z

could you please rebase? and check the Travis?

tuky · 2019-04-23T07:54:21Z

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

milind-shakya-sp · 2019-04-23T17:58:47Z

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

liquidpele · 2019-05-01T23:34:51Z

django_celery_beat/migrations/0001_initial.py

+                    help_text='Useful description',
+                    max_length=getattr(
+                        settings,
+                        'DJANGO_CELERY_BEAT_NAME_MAX_LENGTH',


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.

Whoa...

Can you elaborate on your surprise?

The code is not "unpredictable", the defaults are backwards compatible

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)

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

@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.

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.

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?

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

liquidpele · 2019-05-01T23:36:55Z

tox.ini

@@ -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


Why are you ignoring the test you added?

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.

liquidpele · 2019-05-01T23:37:50Z

tox.ini

@@ -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


You shouldn't need to exclude things like this.

…exes. Add readme Add test settings Remove index related changes

…s; pep8

staled

auvipy

thinking about the consequences of the current migrations after this

auvipy · 2019-05-06T18:50:00Z

django_celery_beat/migrations/0001_initial.py

+                    help_text='Useful description',
+                    max_length=getattr(
+                        settings,
+                        'DJANGO_CELERY_BEAT_NAME_MAX_LENGTH',


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 · 2019-05-27T00:01:20Z

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.

auvipy · 2021-01-20T17:53:59Z

upgrading minimal MySQL version should let us not make this change

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

auvipy requested changes Dec 4, 2018

View reviewed changes

milind-shakya-sp force-pushed the fix_max_length_mysql_issue branch from 2184d6d to 510da36 Compare December 4, 2018 15:11

tuky mentioned this pull request Feb 11, 2019

Messed up migration files #217

Closed

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 previously approved these changes Mar 12, 2019

View reviewed changes

milind-shakya-sp force-pushed the fix_max_length_mysql_issue branch from 30ed551 to ed131d0 Compare April 23, 2019 15:21

liquidpele suggested changes May 1, 2019

View reviewed changes

liquidpele added the Changes Needed label May 2, 2019

dilin-sp and others added 3 commits May 6, 2019 14:37

Set PeriodicTask.name max_length = 191 to avoid issues with MySQL ind…

b8095c9

…exes. Add readme Add test settings Remove index related changes

test that PeriodicTask.name.max_length defaults to 200

d482429

test models with DJANGO_CELERY_BEAT_NAME_MAX_LENGTH=191 in own proces…

1dc843b

…s; pep8

milind-shakya-sp force-pushed the fix_max_length_mysql_issue branch from ed131d0 to 506dada Compare May 6, 2019 18:40

fix tox.ini

46dac8e

milind-shakya-sp force-pushed the fix_max_length_mysql_issue branch from 506dada to 46dac8e Compare May 6, 2019 18:42

auvipy reviewed May 6, 2019

View reviewed changes

auvipy self-assigned this Jan 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix max_length issue for mysql #202

Fix max_length issue for mysql #202

milind-shakya-sp commented Dec 3, 2018 •

edited

Loading

auvipy left a comment

milind-shakya-sp commented Dec 4, 2018

auvipy commented Dec 5, 2018

milind-shakya-sp commented Dec 5, 2018

auvipy commented Dec 7, 2018

tinylambda commented Dec 10, 2018 •

edited

Loading

tinylambda commented Dec 10, 2018

milind-shakya-sp commented Dec 10, 2018

tinylambda commented Dec 11, 2018 •

edited

Loading

milind-shakya-sp commented Dec 11, 2018

tinylambda commented Dec 11, 2018

tinylambda commented Dec 12, 2018

milind-shakya-sp commented Dec 12, 2018

tinylambda commented Dec 13, 2018

milind-shakya-sp commented Dec 13, 2018

tuky commented Feb 11, 2019 •

edited

Loading

tuky commented Feb 15, 2019

auvipy commented Apr 23, 2019

tuky commented Apr 23, 2019

milind-shakya-sp commented Apr 23, 2019

liquidpele May 1, 2019

tuky May 2, 2019 •

edited

Loading

milind-shakya-sp May 6, 2019

auvipy May 6, 2019

liquidpele May 6, 2019

auvipy May 6, 2019

liquidpele May 1, 2019

tuky May 2, 2019

liquidpele May 1, 2019

auvipy left a comment

auvipy May 6, 2019

liquidpele commented May 27, 2019

auvipy commented Jan 20, 2021

Fix max_length issue for mysql #202

Are you sure you want to change the base?

Fix max_length issue for mysql #202

Conversation

milind-shakya-sp commented Dec 3, 2018 • edited Loading

auvipy left a comment

Choose a reason for hiding this comment

milind-shakya-sp commented Dec 4, 2018

auvipy commented Dec 5, 2018

milind-shakya-sp commented Dec 5, 2018

auvipy commented Dec 7, 2018

tinylambda commented Dec 10, 2018 • edited Loading

tinylambda commented Dec 10, 2018

milind-shakya-sp commented Dec 10, 2018

tinylambda commented Dec 11, 2018 • edited Loading

milind-shakya-sp commented Dec 11, 2018

tinylambda commented Dec 11, 2018

tinylambda commented Dec 12, 2018

milind-shakya-sp commented Dec 12, 2018

tinylambda commented Dec 13, 2018

milind-shakya-sp commented Dec 13, 2018

tuky commented Feb 11, 2019 • edited Loading

tuky commented Feb 15, 2019

auvipy commented Apr 23, 2019

tuky commented Apr 23, 2019

milind-shakya-sp commented Apr 23, 2019

Choose a reason for hiding this comment

tuky May 2, 2019 • edited Loading

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

auvipy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liquidpele commented May 27, 2019

auvipy commented Jan 20, 2021

milind-shakya-sp commented Dec 3, 2018 •

edited

Loading

tinylambda commented Dec 10, 2018 •

edited

Loading

tinylambda commented Dec 11, 2018 •

edited

Loading

tuky commented Feb 11, 2019 •

edited

Loading

tuky May 2, 2019 •

edited

Loading