Opened 6 months ago

Closed 4 months ago

Last modified 4 months ago

#34949 closed Cleanup/optimization (fixed)

Clarify when unique constraints are ignored.

Reported by: Roman Odaisky Owned by: Jordan Bae
Component: Documentation Version: dev
Severity: Normal Keywords:
Cc: Jordan Bae 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

Consider the following code:

class SomeModel(models.Model):
    a = models.IntegerField()
    b = models.IntegerField()
    c = models.IntegerField()
    d = models.IntegerField()
    e = models.IntegerField()
    f = models.IntegerField()
    g = models.IntegerField(null=True)

    class Meta:
        indexes = [
            models.Index(fields=["a"], name="index_a"),
            models.Index(fields=["b"], include=["c"], name="index_b_include_c"),
        ]
        constraints = [
            models.UniqueConstraint(fields=["d"], name="unique_d"),
            models.UniqueConstraint(fields=["e"], include=["f"], name="unique_e_include_f"),
            models.UniqueConstraint(fields=["g"], nulls_distinct=True, name="unique_g_nulls_distinct"),
        ]

Expected Behavior

From the documentation:

  • include is ignored for databases besides PostgreSQL.
  • nulls_distinct is ignored for databases besides PostgreSQL 15+.

One would imagine that if we run this on SQLite we’ll end up with five indexes on one field each as though the unsupported parameters were not present.

Actual Behavior

Instead we get:

  • index_a on a
  • index_b_include_c on b
  • unique_d on d
  • and no other constraints!

The documentation and the behavior are clearly at odds, and the behavior is very confusing. The intention behind indexes like unique_e_include_f is to enforce a constraint and to enable an optimization on top of that, Django is unable to provide the optimization so it doesn’t enforce the constraint either?

Workflows Affected

It seems to me there are two important workflows, none of which Django currently supports:

  1. A project for which the DB has been chosen long ago, and a migration to a different one is very unlikely. The developer would like to get the most out of the DB and is willing to use DB-specific features. In this case Django should raise errors if the specific features requested are unavailable.
  2. A DB-agnostic app intended to be included in other projects. The developer would like Django to create indexes that best match the performance requirements as appropriate for whatever DB the app may end up running on.

For example, in the second case, anticipating a lot of SomeModel.objects.filter(b__range=...).values("b", "c") the developer would like to create an Index(fields=["b"], include=["c"]) but with a fallback to Index(fields=["b", "c"]) if covering indexes aren’t supported. The interface to create custom indexes is in theory documented but very sparsely (it doesn’t even say what the arguments to create_sql are) so expecting app developers to use this route is unrealistic.

Note that while in many other cases failing to perform an optimization is considered a graceful fallback, indexes are explicitly created for purposes of optimization and failing to achieve that is an error that should be signaled.

Suggested Resolution

  1. Under no circumstances should Django quietly decline to create a unique constraint.
  2. Index and UniqueConstraint classes should have extra parameters to specify what to fall back to if requested features are unavailable. Perhaps something along the lines of
    models.Index(
        fields=["b"],
        include=["c"],
        fallback=models.Index(fields=["b", "c"]),
        # where does the name go though?
    )
    models.UniqueConstraint(
        fields=["b"],
        include=["c"],
        fallback=[
            models.UniqueConstraint(fields=["b"]),
            models.Index(fields=["b", "c"]),
        ],
    )
    models.UniqueConstraint(
        fields=["x"],
        nulls_distinct=True,
        fallback=models.UniqueConstraint(Coalesce(F("x"), 0)),
    )
    
  3. There should be a setting to make it a warning or an error if Django encounters an index that it can’t create exactly and that has no user-specified fallback.
  4. A perfect solution would also provide a way of upgrading indexes if a later version of the DB adds support for the features that were previously missing.

Here’s my attempt to approximate a solution:

class FallbackIndex(models.Index):
    def __init__(self, *a, requires=None, fallback=None, **k):
        super().__init__(*a, **k)
        self.requires = requires
        self.fallback = fallback

    def create_sql(self, model, schema_editor, using="", **kwargs):
        return (
            super()
            if getattr(schema_editor.connection.features, self.requires)
            else self.fallback
        ).create_sql(model, schema_editor, using=using, **kwargs)

    def deconstruct(self):
        path, expressions, kwargs = super().deconstruct()
        kwargs["requires"] = self.requires
        kwargs["fallback"] = self.fallback
        return path, expressions, kwargs

indexes = [
    FallbackIndex(
        fields=["b"],
        include=["c"],
        name="index_b_include_c",
        requires="supports_covering_indexes",
        fallback=models.Index(fields=["b", "c"], name="index_b_include_c"),
    ),
]

except it should be possible to determine the required features automatically.

Change History (19)

comment:1 by David Sanders, 6 months ago

Triage Stage: UnreviewedAccepted

Hey thanks for the report!

Accepting though I don't know whether this will end up being a documentation change or behaviour change. I agree the behaviour is inconsistent between indexes & constraints, though there may be a few factors to consider here and other team members will weigh in on whether making the behaviour consistent is a desirable outcome. (If we do change behaviour this ticket may need to be split in 2 as nulls_distinct is a new Django 5.0 feature.)

comment:2 by David Sanders, 6 months ago

Here's the forum discussion I raised: https://forum.djangoproject.com/t/request-for-comments-about-a-ticket-index-vs-uniqueconstraint-inconsistency/25097

Only got 1 response but I agree with them so that's 2 for changing the behaviour 🤷‍♂️

Roman, would you like to prepare a patch?

comment:3 by Roman Odaisky, 6 months ago

A patch that would do exactly what? By what means do we make it possible for UniqueConstraint(fields=["a"], include=["b"]) to fall back to UniqueConstraint(fields=["a"]) + Index(fields=["a", "b"])?

comment:4 by Jordan Bae, 4 months ago

I think he intended that models.UniqueConstraint(fields=e, include=f, name="unique_e_include_f") makes 'unique_e_include_f' on 'e' like models.Index. and UniqueConstraint with nulls_distinct act ignored like deferrable so need to update documentation. If you are okay, can i handle this?

comment:5 by Jordan Bae, 4 months ago

Owner: changed from nobody to Jordan Bae
Status: newassigned

comment:6 by Roman Odaisky, 4 months ago

Perhaps the easiest way of implementing this would be something along the lines of

    class Meta:
        constraints = [
            models.UniqueConstraint(fields=["e"], include=["f"], if_has_features=["supports_covering_indexes"]),
            models.UniqueConstraint(fields=["e"], unless_has_features=["supports_covering_indexes"]),
        ]
        indexes = [
            models.Index(fields=["e", "f"], unless_has_features=["supports_covering_indexes"]),
        ]

The only question that would remain would be, what if the DB gains support for a feature it previously lacked.

comment:7 by Jordan Bae, 4 months ago

Thanks for sharing ur thought. Currently I am trying to update two changes.
when i check 4options related this issue with below code

class Ticket34949(models.Model):
    a = models.IntegerField()
    b = models.IntegerField()
    c = models.IntegerField()
    d = models.IntegerField()
    e = models.IntegerField()
    f = models.IntegerField()

    class Meta:
        constraints = [
            models.UniqueConstraint(fields=['a'], name='unique_a'),
            models.UniqueConstraint(fields=['b'], name='unique_b_c', include=['c']),
            models.UniqueConstraint(fields=['d'], name='unique_d_e', deferrable=models.Deferrable.DEFERRED),
            models.UniqueConstraint(fields=['e'], name='unique_e', nulls_distinct=False),
            models.UniqueConstraint(fields=['f'], name='unique_f', opclasses=['varchar_pattern_ops'])
        ]

Django makes only a,f unique constraints in MySQL.
I think opclasses, include are option for optimization. so it needs to make unique constraints as David said.
And nulls_distinct, deferrable are related to unique constraints behavior. so, Django shouldn't make unique constraints. so, my actions are below two items.
1) fix includes
2) update documents for distinguishing between opclasses, include and nulls_distinct, deferrable.

comment:8 by Jordan Bae, 4 months ago

Cc: Jordan Bae added
Has patch: set

comment:9 by Mariusz Felisiak, 4 months ago

I'm not really sure what do we want to achieve in this ticket.

Under no circumstances should Django quietly decline to create a unique constraint.

It doesn't. Django has system checks for all unsupported options models.W036 -> models.W040 so none of them is silently omitted.

comment:10 by Mariusz Felisiak, 4 months ago

Patch needs improvement: set

comment:11 by Jordan Bae, 4 months ago

Patch needs improvement: unset

comment:12 by Roman Odaisky, 4 months ago

I'm not really sure what do we want to achieve in this ticket.

Suppose I want to write a reusable Django app that others will include in their projects. A model in the app wants an index that’s best described as UniqueConstraint(fields=["a", "b"], include=["c", "d"]). How do I make Django create constraints and/or indexes that best approximate this on whatever DB the user will run their project?

comment:13 by Mariusz Felisiak, 4 months ago

If you want to create a cross-database constraint you should use options support by all databases.

comment:14 by Mariusz Felisiak, 4 months ago

Patch needs improvement: set

comment:15 by Jordan Bae, 4 months ago

Patch needs improvement: unset

comment:16 by Mariusz Felisiak, 4 months ago

Component: Database layer (models, ORM)Documentation
Type: BugCleanup/optimization

comment:17 by Mariusz Felisiak, 4 months ago

Summary: Fails to create unique constraintsClarify when unique constraints are ignored.
Triage Stage: AcceptedReady for checkin

comment:18 by Mariusz Felisiak <felisiak.mariusz@…>, 4 months ago

Resolution: fixed
Status: assignedclosed

In 4fec1d2:

Fixed #34949 -- Clarified when UniqueConstraints with include/nulls_distinct are not created.

comment:19 by Mariusz Felisiak <felisiak.mariusz@…>, 4 months ago

In dd2d768:

[5.0.x] Fixed #34949 -- Clarified when UniqueConstraints with include/nulls_distinct are not created.

Backport of 4fec1d2ce37241fb8fa001971c441d360ed2a196 from main

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