-
-
Notifications
You must be signed in to change notification settings - Fork 340
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
Failed to install index for silk.Request model: (1071, 'Specified key was too long; max key length is 767 bytes') #38
Comments
Interesting, what database are you using? |
mysql5.6
|
… 767 bytes' when creating the Request table in MySQL
… 767 bytes' when creating the Request table in MySQL
http://dev.mysql.com/doc/refman/5.6/en/create-index.html says:
this means the length of the varchar column is too long to apply index on so we must decrease the size of the column or size of the index to 255. I tried changing the table definition to: class Request(models.Model):
path = CharField(max_length=255, db_index=True)
...
view_name = CharField(max_length=255, db_index=True, blank=True, default='') and it worked. but i don't know if changing the length of these 2 fields is good or bad for the system to work. |
@utkuyaman, the only issue I see with that is if a path is longer than 255 characters it's going to break when we try to save down to the db. Not sure how rare that would be. I totally support making this change though, but I think if you do make a pull request for this you should add code that truncates any paths that are >255 |
Is there any update on this? I can take a stab at making a pull request to truncate the path if that's the way to go. |
I changed the table definition as i described in my reply. and it worked for me. |
I just tried changing the length of the two fields and it did indeed work for me. |
@prismic @utkuyaman I def think truncating is the way to go. It would be nice if there was a way to make the |
If changing maxlength is not desirable, what about the ability to turn off indexing on those fields via settings? I imagine running on a dev box indexing would not be a huge performance issue, although in production it would be. Would a warning message in the docs about turning off indexing suffice? |
@mtford90 To create a pull request that simply truncates a Request.path that is larger than
Both have pro's and cons. Whichever you lean on I can probably develop, write a test and make a pull request. Both solutions put the development at the model level. This simplifies the responsibility to one location that is very visible to discover and understand. Thoughts? |
@utkuyaman @prismic @mtford90 hard-coding the By default, MySQL uses a character encoding it calls The upshot of this is that the maximum index length is actually 191 characters (767/4) instead of 255 characters (767/3). For more details on how the Django team already ran into this issue, see https://code.djangoproject.com/ticket/18392 (in particular the comment at https://code.djangoproject.com/ticket/18392#comment:16 is enlightening). They haven't fully resolved this yet. Also, to be clear: it only makes sense to both change What was the rationale for the original |
@smaccona I believe the attempt was to ensure the ability to capture long URL's that django-silk could be stored. Further up in the thread you will read that indexing a varchar column longer than 255 is not possible in MySQL but it is PostgreSQL. That difference is at the heart of this issue. |
@avelis what I meant was that the current Also, indexing a The point of my post above was that even reducing the |
@avelis sorry for not getting back to you sooner! Have been rather overworked recently ;) @smaccona there was no rationale behind 300 as far as I remember... It was probably arbitrary ¯_(ツ)_/¯. Interesting analysis though... I've never really used MySQL before but seems to have some pretty baffling design decisions. I really doubt I'll have time to dig into this myself (and would be a poor use of my time I think with lack of mysql experience). In fact I'm looking for contributors to silk anyway having just moved it into it's own org - @avelis & @smaccona if you two are able to think of a solution (or at least a mitigation given that the django team haven't solved this either) would be happy to add you both as contributors so that my shitty response time doesn't hold back a fix any longer. |
@mtford90 agreed on the baffling MySQL design decisions ... I don't think the Django team are going to have an easy fix for this given it's a limitation in one of their dependencies and not really something they control. I have no doubt it's stuff like this that makes them highly recommend PostgreSQL over MySQL. I propose that we simply reduce
In that case our maximum length for an indexable For the secondary strategy, here are my thoughts and proposal:
The If this is acceptable to everyone, I'll make a PR for it. |
@smaccona Your precision strategy and solution works for me. I was previously not aware of those settings for MySQL so I do appreciate you enlightening me with that information. @mtford90 I am open to contribution access. Would enjoy seeing this project gain more traction among the open source community. |
@smaccona Reckon it's possible to attempt to write the view name, detect the error & fall back to 190 chars? Not sure how viable or accurate detection of this specific error could be though... @avelis @smaccona, gave you both access to the repo, thanks again for the help & let me know if either of you have any questions on the source code etc. Look forward to the PR ;) |
@mtford90 we could certainly trap an error on save (and I believe MySQL has a specific error for a column's value being too long) but it won't help because we won't get as far as
Django doesn't support different field lengths for different database engines, and even if it did, I think that's unneeded complexity. Nor does it support indexing a field when using one database engine but not another. I think the bottom line is that to support MySQL and keep the index, we have to limit these two fields to 190 characters. To be honest, I don't think 190 characters is that bad a limitation - with 16-character directory, file and view names you would have to have a hierarchy 11-deep to run into a situation where we truncate (querystrings etc. don't count). Here's what one of those paths would look like:
I don't know about you guys but I have nothing that long in any of my apps! |
@smaccona haha very good point... Ok I would say go for the 190 char limit! |
what is the status on this issue? |
@erback this is not as straightforward to fix as I originally thought because migrations for both Django and South will need to be supported for existing installations. For now, I suggest you go with the steps outlined in the second paragraph of my comment above at #38 (comment) to enable longer indexes in MySQL. This can be done without downtime. |
@smaccona thanks for the swift response. Somehow I got it to work anyway by just running syncdb again after it failed. Is there something that I'm missing, because everythin seems to be working fine now? |
@erback I'm pretty sure your models have been created but the two indexes affected by this bug are missing. This will mean that Silk will still work, but will perform very slowly once you have more than a small amount of data. You can confirm this by examining the |
@smaccona aha now I get it hadn't read through the issue well enough. Thanks for the input! |
@smaccona One path forward I see is doing a backwards incompatible change to just reset the initial migration file to the desired new max length. I realize this sort of cleaves current users from the project but could have the desired effect of laying this issue to rest. |
This has been hanging around for a long time now and I haven't had time to come up with a good fix which takes migrations into account properly (there's no strictly non-destructive way to truncate the data). We could do as @avelis suggested and just do a backwards incompatible change and reset the length. Another option is to just more fully document in the installation instructions the procedure for making MySQL support longer indexes - how does everyone feel about that? |
@smaccona For developer ease of use, I don't mind going forward with a fully backwards incompatible change. While making MySQL support longer indexes is possible that might not work for everyone as easily. Not sure what is the best transition strategy for the backwards incompatible change. Open to ideas on that effort. |
I think it's fairly customary to communicate backwards-incompatible changes by bumping the major version number |
@teeberg I agree and given my recent experience no one seems to surprised when that happens. I can look into changing this. I am also open to a PR for this I can cut a release with a major version bump. |
@avelis which fields need adjustments? I will be happy to send the suggested changes |
Adds support for `utf8` and `utf8mb4` character sets in MySQL's default configuration. Closes #38.
MySQL index restrictions prevent us from having path or view_name be longer than 190 characters (see jazzband#38), but there is no logic in the Request object to enforce this limit when saving, so long URLs cause a 500 error. Closes jazzband#179.
I get this warning when run "python manage.py syncdb".
The text was updated successfully, but these errors were encountered: