Re: [Django] #13724: Calling QuerySet.delete() through a relation the DB router is ignored.
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner: nobody
<chris@…> | Status: new
Type: Bug | Version: 1.3
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: multidb | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by paulcollins):
* status: closed => new
* severity: => Normal
* cc: paul.collins.iii@… (added)
* resolution: invalid =>
* version: 1.2 => 1.3
* easy: => 0
* ui_ux: => 0
* type: => Bug
Comment:
So we're not using the django-multidb-router, and we're seeing this kind
of odd behavior as well. Allow_relation is not being called, which makes
sense but what doesn't make sense is
```
.all().delete()
```
The Router is being called for the .all() (db_for_read), but then the
QuerySet is being changed to a DELETE. Since db_for_write is never
called, and our router automatically picks a read-only slave for read-only
queries, thus the expected explosion happens.
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner: nobody
<chris@…> | Status: new
Type: Bug | Version: 1.3
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: multidb | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
@paulcollins, could you provide a test for Django's test suite that
demonstrates the problem?
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner: nobody
<chris@…> | Status: new
Type: Bug | Version: 1.3
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: multidb | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by barnardo):
Attached is a test case for 1.3.7. I was not able to duplicate. Please
verify.
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner:
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: multidb | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => paulcollins
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
PR created with unit tests showing several cases where this bug manifests
https://github.com/django/django/pull/1597
Marking ticket as accepted per conversations with Jacob Kaplan-Moss and
Russell Keith-Magee
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner:
<chris@…> | paulcollins
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) | Resolution:
Keywords: multidb | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by paulcollins):
* stage: Accepted => Ready for checkin
Comment:
yes badform I know but the code is in a good state per reviews with
Russell in person. The docs I think are okay but a second set of eyes is
always appreciated. Flagging this as ready for checkin just to get a core
devs attention =)
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner:
<chris@…> | paulcollins
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: multidb | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by russellm):
To back up @paulcollins here -- I'm completely OK with this being bumped
to RFC. There's still a need for a final review, but I've been reviewing
the patch as it was being developed, and I'm happy with the direction it's
been heading.
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner:
<chris@…> | paulcollins
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) | Resolution:
Keywords: multidb | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
I added a question to the PR about how db_for_write should work in related
manager methods. For example in someobj.related_set.add()
(fields/related.py:L418 in current master), should there be one call to
db_for_write, and then all objects are saved to that DB? This is how m2m
field's add works for example. And, if so, should this be tested. As is,
each time save() is called it will individually call db_for_write() and
this could lead splitting the save into different databases.
Maybe the related manager write routing doesn't need to be addressed in
this ticket, and maybe I just don't understand this issue fully (I don't
know multidb routing that well). Still, I will mark "patch needs
improvement" until this question is answered.
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner:
<chris@…> | paulcollins
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: multidb | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by paulcollins):
@akaariai
That's a good point, the save could end up splitting it across multiple
databases. Depending on how complex the router is maybe that's desired?
In any this case, the point of this ticket was the related manager write
routing. For example
`A.objects.get(pk=something).related_set.filter(name__in=[str(range(12))]).delete()`
A ... get is fine.
related_set.filter says get db_for_read
.delete should call db_for_write, but since a db has already been set on
the QuerySet object it doesn't. At that point it's locked into trying to
send the delete to the read database. In the case of sharding db_for_read
and db_for_write are probably the same thing, but in the case of master /
slave (which is where I ran into this issue) they're not. In a master
slave setup, trying to do a delete on db_for_read will (one hopes) fail.
In the case of things being split up because of multiple calls to get
db_for_write I believe the hint object is passed down the chain so the
implementer of the router could make a decision based on that hint. I'll
add a check in the tests for the hint and report back.
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner:
<chris@…> | paulcollins
Component: Database layer | Version: 1.3
Severity: Normal | Triage Stage: Accepted
Keywords: multidb | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"9595183d03cfd0d94ae2dd506a3d2b86cf5c74a7"]:
{{{
#!CommitTicketReference repository=""
revision="9595183d03cfd0d94ae2dd506a3d2b86cf5c74a7"
Fixed #13724: Corrected routing of write queries involving managers.
Previously, if a database request spanned a related object manager, the
first manager encountered would cause a request to the router, and this
would bind all subsequent queries to the same database returned by the
router. Unfortunately, the first router query would be performed using
a read request to the router, resulting in bad routing information being
used if the subsequent query was actually a write.
This change defers the call to the router until the final query is
acutally
made.
It includes a small *BACKWARDS INCOMPATIBILITY* on an edge case - see the
release notes for details.
Thanks to Paul Collins (@paulcollinsiii) for the excellent debugging
work and patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner:
<chris@…> | paulcollins
Type: Bug | Status: closed
Component: Database layer | Version: 1.3
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: multidb | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by russellm):
@akaariai - FYI - I implemented your suggested change in the test suite;
the router tests now validate that the hints and model passed to the
router are valid.
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner:
<chris@…> | paulcollins
Type: Bug | Status: closed
Component: Database layer | Version: 1.3
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: multidb | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"f93896344abfdfd288e0d8249b37e2b799fd7d53"]:
{{{
#!CommitTicketReference repository=""
revision="f93896344abfdfd288e0d8249b37e2b799fd7d53"
Fixed Python 3.2 syntax errors; refs #13724.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner:
<chris@…> | paulcollins
Type: Bug | Status: closed
Component: Database layer | Version: 1.3
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: multidb | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"4745ea1d27a154a44ed2cacb66b01d5de1f9e3d3"]:
{{{
#!CommitTicketReference repository=""
revision="4745ea1d27a154a44ed2cacb66b01d5de1f9e3d3"
Added hints argument to GeoQuerySet; refs #13724.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:13>
Django
-------------------------------------+-------------------------------------
<chris@…> | Collins
Type: Bug | Status: closed
Component: Database layer | Version: 1.3
(models, ORM) |
Keywords: multidb | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"6c23b43655f3710cfb1ecc57236405d11a544247" 6c23b436]:
{{{
#!CommitTicketReference repository=""
revision="6c23b43655f3710cfb1ecc57236405d11a544247"
Refs #13724 -- Corrected QuerySet signature in docs.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner: Paul
<chris@…> | Collins
Type: Bug | Status: closed
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: multidb | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"213a39b4df1edb36b042e347a6c344b3fcd27bc0" 213a39b4]:
{{{
#!CommitTicketReference repository=""
revision="213a39b4df1edb36b042e347a6c344b3fcd27bc0"
[3.0.x] Refs #13724 -- Corrected QuerySet signature in docs.
Backport of 6c23b43655f3710cfb1ecc57236405d11a544247 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Targett | Owner: Paul
<chris@…> | Collins
Type: Bug | Status: closed
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: multidb | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"43bc59ceb624d6a1ac58aa7a282254e60c9c4025" 43bc59ce]:
{{{
#!CommitTicketReference repository=""
revision="43bc59ceb624d6a1ac58aa7a282254e60c9c4025"
[2.2.x] Refs #13724 -- Corrected QuerySet signature in docs.
Backport of 6c23b43655f3710cfb1ecc57236405d11a544247 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/13724#comment:16>