Opened 3 weeks ago

Closed 2 weeks ago

#36165 closed Bug (fixed)

Postgres schema editor throws away custom index deletion SQL

Reported by: Daniel Finch Owned by: Natalia Bidart
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Mariusz Felisiak Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If I wanted to create a Postgres database index type that used custom SQL for both creation and deletion, the custom SQL would only apply in the create case.

Here's some example code:

from django.db.models import Index

class UniqueIndex(Index):
    # Adjusted version of DatabaseSchemaEditor.sql_create_index that works with materialized views
    sql_create_index = "CREATE UNIQUE INDEX %(name)s ON %(table)s%(using)s (%(columns)s);"
    sql_delete_index = "DROP INDEX IF EXISTS %(name)s;"

    def create_sql(self, model, schema_editor, using="", **kwargs):
        kwargs.setdefault("sql", self.sql_create_index)
        return super().create_sql(model, schema_editor, using, **kwargs)

    def remove_sql(self, model, schema_editor, **kwargs):
        kwargs.setdefault("sql", self.sql_delete_index)
        return super().remove_sql(model, schema_editor, **kwargs)

The call to create_sql would do the right thing, and use the SQL supplied - as seen in the underlying code:

        sql = sql or (
            self.sql_create_index
            if not concurrently
            else self.sql_create_index_concurrently
        )

https://github.com/django/django/blob/5f30fd2358fd60a514bdba31594bfc8122f30167/django/db/backends/postgresql/schema.py#L349-L353

However, the same cannot be said for the delete code, which simply ignores the provided SQL value:

        sql = (
            self.sql_delete_index_concurrently
            if concurrently
            else self.sql_delete_index
        )

https://github.com/django/django/blob/5f30fd2358fd60a514bdba31594bfc8122f30167/django/db/backends/postgresql/schema.py#L324-L330

I believe this constitutes a bug and should have an obvious and simple fix.

Change History (4)

comment:1 by Natalia Bidart, 2 weeks ago

Component: CSRFDatabase layer (models, ORM)
Has patch: set
Owner: set to Natalia Bidart
Status: newassigned
Triage Stage: UnreviewedAccepted
Version: 5.1dev

Hello Daniel, thank you for your report. I have spent some time reviewing code and docs for Index. First of all, the docs do not document the methods create_sql or remove_sql, so overriding these is not necessarily supported. But, after further investigation I found:

  • revno bd366ca2aeffa869b7dbc0b0aa01caea75e6dc31 added the support for the custom sql in create index
  • the rationale from Mariusz is still valid "this may be helpful for 3rd-party database backends that subclass django.db.backends.postgresql. I think it's worth adding".

Given the above, I'm accepting this ticket and I'm pushing a branch with fix and regression test for both methods.

comment:2 by Natalia Bidart, 2 weeks ago

Cc: Mariusz Felisiak added

comment:3 by Natalia Bidart, 2 weeks ago

Triage Stage: AcceptedReady for checkin

comment:4 by nessita <124304+nessita@…>, 2 weeks ago

Resolution: fixed
Status: assignedclosed

In 1f33f21f:

Fixed #36165 -- Made PostgreSQL's SchemaEditor._delete_index_sql() respect the "sql" argument.

This is a follow up of bd366ca2aeffa869b7dbc0b0aa01caea75e6dc31.

Thank you Daniel Finch for the report.

Note: See TracTickets for help on using tickets.
Back to Top