Re: [Django] #33759: Using subquery to filter a model delete generates a sub-optimal SQL
Django
-------------------------------------+-------------------------------------
Reporter: Christofer Bertonha | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* cc: Simon Charette (added)
* type: Bug => Cleanup/optimization
Comment:
This behavior was changed in 4074f38e1dcc93b859bbbfd6abd8441c3bca36b3 to
prevent:
{{{
django.db.utils.NotSupportedError: (1235, "This version of MySQL doesn't
yet support 'LIMIT & IN/ALL/ANY/SOME subquery'")
}}}
on MySQL. I have no idea for an alternative fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/33759#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Christofer Bertonha | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* stage: Unreviewed => Accepted
Comment:
We could add a new feature flag to add an extra subquery only on MySQL:
{{{#!diff
diff --git a/django/db/backends/base/features.py
b/django/db/backends/base/features.py
index c54d30cf73..e8737aadc2 100644
--- a/django/db/backends/base/features.py
+++ b/django/db/backends/base/features.py
@@ -12,6 +12,9 @@ class BaseDatabaseFeatures:
allows_group_by_selected_pks = False
empty_fetchmany_value = []
update_can_self_select = True
+ # Does the backend support self-reference subqueries in the DELETE
+ # statement?
+ delete_can_self_reference_subquery = True
# Does the backend distinguish between '' and None?
interprets_empty_strings_as_nulls = False
diff --git a/django/db/backends/mysql/features.py
b/django/db/backends/mysql/features.py
index 3ea3deeae3..848d891a84 100644
--- a/django/db/backends/mysql/features.py
+++ b/django/db/backends/mysql/features.py
@@ -25,6 +25,7 @@ class DatabaseFeatures(BaseDatabaseFeatures):
supports_slicing_ordering_in_compound = True
supports_index_on_text_field = False
supports_update_conflicts = True
+ delete_can_self_reference_subquery = False
create_test_procedure_without_params_sql = """
CREATE PROCEDURE test_procedure ()
BEGIN
diff --git a/django/db/models/sql/compiler.py
b/django/db/models/sql/compiler.py
index 9c7bd8ea1a..e57a43ac47 100644
--- a/django/db/models/sql/compiler.py
+++ b/django/db/models/sql/compiler.py
@@ -1721,7 +1721,10 @@ class SQLDeleteCompiler(SQLCompiler):
Create the SQL for this query. Return the SQL string and list of
parameters.
"""
- if self.single_alias and not
self.contains_self_reference_subquery:
+ if self.single_alias and (
+ self.connection.features.delete_can_self_reference_subquery
or
+ not self.contains_self_reference_subquery
+ ):
return self._as_sql(self.query)
innerq = self.query.clone()
innerq.__class__ = Query
}}}
Another issue is that `QuerySet.delete()` from the ticket description:
{{{
subquery = Comment.objects.filter(created_at__lt=datetime(2022, 6,
1))[:1000]
Comment.objects.filter(id__in=subquery).delete()
...
File "site-packages/MySQLdb/connections.py", line 254, in query
_mysql.connection.query(self, query)
django.db.utils.NotSupportedError: (1235, "This version of MySQL doesn't
yet support 'LIMIT & IN/ALL/ANY/SOME subquery'")
}}}
crashes on MySQL even with 4074f38e1dcc93b859bbbfd6abd8441c3bca36b3.
--
Ticket URL: <https://code.djangoproject.com/ticket/33759#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Christofer Bertonha | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Mariusz I think that it makes sense to only enable the query wrapping when
necessary through a feature flag as you've suggested.
As for the MySQL limited subquery issue that's a different can of worms.
The unnecessary query wrapping should be testable on backends with the
feature disabled by counting the number of `SELECT`.
Thanks for the ping and sorry for my late answer!
--
Ticket URL: <https://code.djangoproject.com/ticket/33759#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Roy
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Alokik Roy
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33759#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Alokik Roy):
* owner: Alokik Roy => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/33759#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Pandey
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => Aman Pandey
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33759#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Christofer Bertonha | Owner: Aman
Type: | Pandey
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Aman Pandey):
* has_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
Draft PR up https://github.com/django/django/pull/16809
--
Ticket URL: <https://code.djangoproject.com/ticket/33759#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Christofer Bertonha | Owner: Aman
Type: | Pandey
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Aman Pandey):
* needs_tests: 1 => 0
Comment:
PR ready.
--
Ticket URL: <https://code.djangoproject.com/ticket/33759#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Christofer Bertonha | Owner: Aman
Type: | Pandey
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33759#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Christofer Bertonha | Owner: Aman
Type: | Pandey
Component: Database layer | Version: 4.0
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"0b0998dc151feb77068e2387c34cc50ef6b356ae" 0b0998dc]:
{{{
#!CommitTicketReference repository=""
revision="0b0998dc151feb77068e2387c34cc50ef6b356ae"
Fixed #33759 -- Avoided unnecessary subquery in QuerySet.delete() with
self-referential subqueries if supported.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33759#comment:13>