[Django] #34964: Reversing the order of Q objects in a CheckConstraint generates a migration
Django
-------------------------------------+-------------------------------------
Reporter: Jacob | Owner: Jacob Walls
Walls |
Type: | Status: assigned
Cleanup/optimization |
Component: | Version: dev
Migrations |
Severity: Normal | Keywords: noop
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changing `Q(A) & Q(B)` to `Q(B) & Q(A)` (or `Q(B, A)`) generates a
migration that drops and recreates the exact same constraint.
Suggesting to build on the work in #34744 to adjust the identity property
for Q objects to use a deterministic sort, given that `^`, `&`, and `|`
operations are commutative.
[https://github.com/django/django/pull/17466 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34964>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: noop | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
The only thing that slightly worries me is that I've seen Postgres be very
finicky about the order of components in complex constraint when dealing
with conditional of functional indices and in these rare cases the order
matters even they are representing the same condition.
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: noop | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: David Sanders (added)
Comment:
To extend Simon's concerns; I think this needs more investigation as
commutative doesn't always mean that the system will behave in an
equivalent way 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: noop | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jacob Walls):
Simon's suggestion on the PR was to sort Q objects in the constructor of
`CheckConstraint` so that we wouldn't even get into a situation where the
autodetector would be remaining quiet even with SQL changes. I see that
this would come at the cost of causing migrations to be emitted for
existing projects, though, so we might want a release note.
The case that originally prompted me to open a ticket was for a migration
dropping and recreating a constraint with the exact same SQL, e.g.
changing `Q(A) & Q(B)` to `Q(A, B)` . I just added a regression test for
this case.
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: noop | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
The creation of new migrations could be avoided entirely by performing the
sorting only during `__eq__`
{{{#!diff
diff --git a/django/db/models/constraints.py
b/django/db/models/constraints.py
index 56d547e6b0..e221bebd8b 100644
--- a/django/db/models/constraints.py
+++ b/django/db/models/constraints.py
@@ -152,14 +152,22 @@ def __repr__(self):
)
def __eq__(self, other):
- if isinstance(other, CheckConstraint):
- return (
- self.name == other.name
- and self.check == other.check
- and self.violation_error_code ==
other.violation_error_code
- and self.violation_error_message ==
other.violation_error_message
- )
- return super().__eq__(other)
+ if not isinstance(other, CheckConstraint):
+ return super().__eq__(other)
+ # Relax check equality to equivalence comparisons to allow
+ # re-ordering of components.
+ if isinstance(self.check, Q):
+ if not isinstance(other.check, Q) or not
self.check.equivalent_to(
+ other.check
+ ):
+ return False
+ elif self.check != other.check:
+ return False
+ return (
+ self.name == other.name
+ and self.violation_error_code == other.violation_error_code
+ and self.violation_error_message ==
other.violation_error_message
+ )
def deconstruct(self):
path, args, kwargs = super().deconstruct()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: noop | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jacob Walls):
Thanks Simon. I just checked more carefully and found that I wasn't able
to come up with a test case that creates a migration for an existing
project. (Sorry!)
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: noop | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: David Wobrock (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: noop | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
I have doubts, in my experience database optimizers are sometimes very
picky and the order of statements may be significant even for commutative
operators.
Is it worth changing?
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: noop | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jacob Walls):
Thanks for the extra context around those edge cases. I'd love to know
more about them so I can look for them in my own projects.
I notice we've been sorting the arguments to the Q constructor since
#29125. So if someone did need to change the order to work around a
database edge case, they'd ''want'' a change from `Q(A, B)` to `Q(A) &
Q(B)` to generate a migration, since if B sorts before A, it can only be
expressed in Django as the latter, and this PR would take away the
migration to make it.
The argument in favor of this change (which is very slight, I know) is
that refactoring the code for consistency/readability with the surrounding
constraints might generate a migration with a table rebuild. That's
something you might miss. In other words, you need to know that it's not
worth paying that cost when you're editing/reviewing the code. Until I
knew about these edge cases, I thought that was a potential DX win to
reduce cognitive burden.
Finally, I guess if we still wanted this change we might ought to block it
on a resolution of #25245. Should we Someday/Maybe this ticket, then?
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: noop | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
I have a gut feeling that it's not worth doing, as there is a risk of
potential regressions. If you modify `check` of `condition` you should be
ready to deal with consequences of recreating the index, even if they
theoretically have the same meaning 🤷 I'm not strongly against this
patch, this is just a friendly warning ;)
> ... with a table rebuild. That's something you might miss.
I know how migrations work on SQLite ;)
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: noop | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Sanders):
Advice from #postgresql IRC channel is that an expression's operand order
_does_ matter, but only for check constraints.
> in most contexts ANDed and ORed terms are sliced-and-diced as the
planner sees fit. but there is one case that might matter:
> the check constraint obviously gets evaluated to check inserted rows,
and I don't believe (but would have to check) that it gets reordered there
> in which case CHECK (foo OR slowfunc(bar)) might be faster than CHECK
(slowfunc(bar) OR foo)
>
> let me check the code. sec.
> looks like check expressions are not reordered by the planner for
performance
> inside queries, lists of ANDed quals evaluated at a single node are
reordered when possible to put expensive ones last (subject to security
restrictions)
> but that doesn't apply to checking constraints on insert/update
> so I would expect CHECK (foo OR slowfunc(bar)) to be noticably faster
than CHECK (slowfunc(bar) OR foo) in the case where "foo" is usually true
>
> tested it and it looks like i am correct
Advice courtesy of RhodiumToad 💚
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Component: Migrations | Version: dev
Severity: Normal | Resolution: wontfix
Keywords: noop | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
* stage: Accepted => Unreviewed
Comment:
It looks like changing the order is not an option, at least not for
`CheckConstraint`s.
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: closed
Component: Migrations | Version: dev
Severity: Normal | Resolution: wontfix
Keywords: noop | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
In [changeset:"7714ccfeae969aca52ad46c1d69a13fac4086c08" 7714ccfe]:
{{{#!CommitTicketReference repository=""
revision="7714ccfeae969aca52ad46c1d69a13fac4086c08"
Refs #34964 -- Doc'd that Q expression order is preserved.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: closed
Component: Migrations | Version: dev
Severity: Normal | Resolution: wontfix
Keywords: noop | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
{{{#!CommitTicketReference repository=""
revision="a8de04f8db470d1c43f945f28a00fb49b1f2ca80"
[5.0.x] Refs #34964 -- Doc'd that Q expression order is preserved.
Backport of 7714ccfeae969aca52ad46c1d69a13fac4086c08 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34964#comment:14>