[Django] #31881: Raise Index name limit for postgres
Django
-------------------------------------+-------------------------------------
Reporter: Jarek | Owner: nobody
Glowacki |
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: master
layer (models, ORM) | Keywords: postgres index
Severity: Normal | max_name_length
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
[https://github.com/django/django/blob/18d4eac7fce6dfab6149ec7aba72f3c765b5722c/django/db/backends/postgresql/operations.py#L193-L204
/ 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. :)
--
Ticket URL: <https://code.djangoproject.com/ticket/31881>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: postgres index | Triage Stage:
max_name_length | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by felixxm):
> And change the code to just:
> {{{
> class PostgresIndex(Index):
>
> @cached_property
> def max_name_length(self):
> return 63
> }}}
This will break backwards compatibility for `PostgresIndex` subclasses
with long suffixes, because currently we allow suffixes of any length.
> 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?
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` and `max_name_length` attributes
in [https://docs.djangoproject.com/en/3.1/ref/models/indexes/ Model index
reference]. What do you think?
--
Ticket URL: <https://code.djangoproject.com/ticket/31881#comment:1>
Django
-------------------------------------+-------------------------------------
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Keywords: postgres index | Triage Stage:
max_name_length | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => needsinfo
--
Ticket URL: <https://code.djangoproject.com/ticket/31881#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: postgres index | Triage Stage:
max_name_length | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jarek Glowacki):
Hey sorry for the slow response.
Replying to [comment:1 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
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31881#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Severity: Normal | Resolution: needsinfo
Keywords: postgres index | Triage Stage:
max_name_length | Unreviewed
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Florian Demmer (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/31881#comment:4>