#31881 closed Cleanup/optimization (needsinfo)
Raise Index name limit for postgres
Reported by: | Jarek Glowacki | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | postgres index max_name_length |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Hi!
This line here: https://docs.djangoproject.com/en/3.1/ref/models/indexes/#django.db.models.Index.name
Index.name
... For compatibility with different databases, index names cannot be longer than 30 characters and shouldn’t start with a number (0-9) or underscore (_).
Okay, but why is this restriction maintained when using a specific backend's db index class, namely PostgresIndex
?
https://github.com/django/django/blob/8c7992f658e1125f8dc20b19206e8bbbe4187372/django/contrib/postgres/indexes.py#L11-L19
This issue has become more noticeable with django 3.0, now that interpolated names are allowed for index definitions. Nice feature, until it falls over because one of your models has a longer name than the rest. :(
I'd like to propose we change the docs to read:
Index.name
... For compatibility with different databases, index names cannot be longer than 30 characters and shouldn’t start with a number (0-9) or underscore (_). To get around this restriction, you can directly import and use the Index class for your database backend (eg
PostgresIndex
), or write your own.
And change the code to just:
class PostgresIndex(Index): @cached_property def max_name_length(self): return 63 ...
Notes:
- This _should_ be backwards compatible, though I'm not entirely sure how django creates indexes automatically and applies name truncation in these cases, if it were to somewhy use this class, we'd start getting different truncation to what we had in the past and the user would need to make migrations to recreate some of their indexes with new names.. surely not?
- I'm not sure whether this change should be limited to indexes only. I had a brief look and found / this, which already does what I'm suggesting elsewhere. We should pull the 63 out into a
POSTGRES_IDENTIFIER_MAX_NAME_LENGTH
constant and apply it more broadly?
Given the goahead, I'm happy to turn the above into a PR.
Thanks for your time. :)
Change History (3)
follow-up: 3 comment:1 by , 4 years ago
comment:2 by , 4 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
comment:3 by , 4 years ago
Hey sorry for the slow response.
Replying to felixxm:
This will break backwards compatibility for
PostgresIndex
subclasses with long suffixes, because currently we allow suffixes of any length.
I had a deeper dig and I don't see how that's the case.. max_name_length
is only interrogated when:
- an index name has been automatically constructed: https://github.com/django/django/blob/8c7992f658e1125f8dc20b19206e8bbbe4187372/django/db/models/indexes.py#L118-L121
- system checks run: https://github.com/django/django/blob/8c7992f658e1125f8dc20b19206e8bbbe4187372/django/db/models/base.py#L1604-L1612
The code that automatically generates indexes would remain untouched, and unaffected:
https://github.com/django/django/blob/8c7992f658e1125f8dc20b19206e8bbbe4187372/django/db/models/indexes.py#L110-L117
It could potentially be relaxed to pad out the hash up to MyIndex.max_name_length
, but I'm not suggesting we change this, and I don't think doing so would provide any benefit.
I've gone ahead and implemented the change here: https://github.com/django/django/compare/master...jarekwg:ticket_31881?expand=1
Tests all pass.
Please note that PostgresIndex
isn't used by default for Postgres backends, it's only used when inherited by those special Postgres-specific Indexes. So the solution I'm suggesting would only help in cases where users explicitly specify PostgresIndex
when defining their custom Indexes. Users with custom overrides would still be overriding all this, so I don't see how they'd run into regressions.
The only hardcoded shenanigans i could find that would have trouble if suffix length were to change is the one here, but it's oracle-specific code, so wouldn't be affected by our work anyway: https://github.com/django/django/blob/8c7992f658e1125f8dc20b19206e8bbbe4187372/django/db/backends/oracle/operations.py#L596
All that said, I agree that documenting suffix
and max_name_length
would also be quite helpful. I wouldn't've opened this ticket had there been a note in the docs, instructing me to create my own index class with these attributes. I'm happy to offer a PR for that too.
This will break backwards compatibility for
PostgresIndex
subclasses with long suffixes, because currently we allow suffixes of any length.No, because it will break backwards compatibility for backend subclasses (per the comment, "This implementation returns 63, but can be overridden by a custom database backend that inherits most of its behavior from this one."). There are people doing this, e.g. #25428.
I think it's enough to document
suffix
andmax_name_length
attributes in Model index reference. What do you think?