Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

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

comment:1 by Mariusz Felisiak, 4 years ago

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 Model index reference. What do you think?

comment:2 by Mariusz Felisiak, 4 years ago

Resolution: needsinfo
Status: newclosed

in reply to:  1 comment:3 by Jarek Glowacki, 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:

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.

Version 1, edited 4 years ago by Jarek Glowacki (previous) (next) (diff)
Note: See TracTickets for help on using tickets.
Back to Top