Re: [Django] #6785: QuerySet.get(), nit-pick / optimize
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Type: Uncategorized | Status: closed
Component: Uncategorized | Version: queryset-
Severity: Normal | refactor
Keywords: qs-rf | Resolution: wontfix
Has patch: 1 | Triage Stage:
Needs tests: 0 | Unreviewed
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by wim@…):
* ui_ux: => 0
* type: => Uncategorized
* severity: => Normal
* easy: => 0
Comment:
Strange though it may seem: would it be possible to explicitly state the
number of objects returned and restrain that number to 1, 2, 3, 4, 5, more
than 5? In my experience, the number of objects is helpful when debugging.
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Cleanup/optimization | Version: master
Component: Database layer | Resolution: wontfix
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* cc: timograham@… (added)
* needs_better_patch: 0 => 1
* component: Uncategorized => Database layer (models, ORM)
* version: queryset-refactor => master
* keywords: qs-rf =>
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
Seems like there's consensus to re-open this:
https://groups.google.com/d/topic/django-developers/PkzS9Wv6hIU/discussion
Pull request (which needs improvement):
https://github.com/django/django/pull/1139
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
Severity: Normal | Needs documentation: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* status: closed => new
* needs_better_patch: 1 => 0
* resolution: wontfix =>
Comment:
Updated pull request based on django-developers discussion:
https://github.com/django/django/pull/1320
I'm not sure it needs docs or a mention in the release notes, but happy to
add them if necessary.
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Cleanup/optimization | Version: master
Component: Database layer | Resolution: fixed
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"da79ccca1d34f427952cce4555e598a700adb8de"]:
{{{
#!CommitTicketReference repository=""
revision="da79ccca1d34f427952cce4555e598a700adb8de"
Fixed #6785 -- Made QuerySet.get() fetch a limited number of rows.
Thanks Patryk Zawadzki.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: shai (added)
* status: closed => new
* resolution: fixed =>
Comment:
As noted in #23061 the current implementation is problematic on Oracle. It
also does more work than necessary elsewhere: Slicing a query clones it,
which is an expensive operation, and potentially makes the database work
harder.
It is better to implement this as hinted by the ticket's summary -- by
limiting the fetches, not the select.
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
Limiting the fetches doesn't work that well on most core databases. If I
recall correctly sqlite, postgresql and mysql all do transfer all of the
rows of the query when accessing the results (for PostgreSQL I know this
is the case). So, while limiting the fetches would result in less model
instances being generated, the overhead of generating all the rows in the
database and transferring the results would still be there.
To get rid of clone() overhead we could just call query.set_limits()
manually.
With this all being said... I guess we are optimizing the wrong case here.
Successful .get() (just one row returned) should be optimized, not the
error case. I am not sure how much overhead LIMIT (or the nested selects
on Oracle) cost.
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by shai):
Replying to [comment:8 akaariai]:
> Limiting the fetches doesn't work that well on most core databases.
[...] [T]he overhead of generating all the rows in the database and
transferring the results would still be there.
>
> To get rid of clone() overhead we could just call query.set_limits()
manually.
>
> With this all being said... I guess we are optimizing the wrong case
here. Successful .get() (just one row returned) should be optimized, not
the error case. I am not sure how much overhead LIMIT (or the nested
selects on Oracle) cost.
The intent of re-opening is exactly to optimize the successful case; the
issue is that the current implementation imposes an overhead on all
`get()` calls for better handling of the error case. Doing it with fetches
alone will remove this overhead -- for the successful case, asking for 21
results or for all results would be the same. I am not sure about the
added performance costs either, except that (as noted) the nested selects
on Oracle cost us in functionality.
On a side note, if more than one row is returned from the database, no
model instance at all needs to be created.
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 1 => 0
Comment:
Yes, optimizing the successful case is important. The .get() method can be
used in try-except workflows, but I don't know of any realistic use case
where the try-except is interested in multiple objects returned case.
The question is how much overhead the LIMIT 21 clause adds for get() on
databases that support it. If it doesn't add overhead, then keeping the
current behavior on databases that support LIMIT seems OK to me. If it
adds overhead, then lets just get rid of the LIMIT clause and close this
ticket as wontfix. As said before we can get rid of the clone overhead
easily, so that isn't a factor in deciding what to do here.
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jafula):
Any chance of revisiting this for 1.8? In Oracle, the get query SQL is
wrapped in an additional select and is just noise when debugging (no known
performance complaints in Production so far).
We have our own Oracle backend that inherits from the one that ships with
Django that we have added Oracle DRCP (connection pooling) support to it.
So some sort of hook, parameter or over-ridable function that would let us
turn this feature off in our own backend would be wonderful.
Michael
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
Of course, if someone writes a patch we'll look at it.
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(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 akaariai):
I'm beginning to think committing this was a mistake. Correctly working
code shouldn't face this issue, so why optimize it. Removing this from 1.8
is ok for me.
One possibility is to use the iterator() method, so we don't turn all the
rows to model instances.
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"293fd5da5b8c7b79bd34ef793ab45c1bb8ac69ea"]:
{{{
#!CommitTicketReference repository=""
revision="293fd5da5b8c7b79bd34ef793ab45c1bb8ac69ea"
Reverted "Fixed #6785 -- Made QuerySet.get() fetch a limited number of
rows."
This reverts commit da79ccca1d34f427952cce4555e598a700adb8de.
This optimized the unsuccessful case at the expense of the successful one.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
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 Tim Graham <timograham@…>):
In [changeset:"7060ef71581c740bcc28ed405225537a411c36b5"]:
{{{
#!CommitTicketReference repository=""
revision="7060ef71581c740bcc28ed405225537a411c36b5"
[1.8.x] Reverted "Fixed #6785 -- Made QuerySet.get() fetch a limited
number of rows."
This reverts commit da79ccca1d34f427952cce4555e598a700adb8de.
This optimized the unsuccessful case at the expense of the successful one.
Backport of 293fd5da5b8c7b79bd34ef793ab45c1bb8ac69ea from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: deadwisdom | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
I've reverted the original patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:16>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(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 Shai Berger):
So, a solution was added then reverted, because it did two things: Added
significant overhead in the general case, and broke things on Oracle.
I believe we can try again now -- I think Oracle has added `OFFSET/LIMIT`
support in later versions and the versions which lack it are no longer
supported; and Anssi has suggested in comment:8 a way to avoid the
overhead.
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:17>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Amir Hadi
* status: new => assigned
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/11215 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Brantley | Owner: Amir Hadi
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
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:"330638b89f14e1fb06e9d313ccc9768ae167c53f" 330638b8]:
{{{
#!CommitTicketReference repository=""
revision="330638b89f14e1fb06e9d313ccc9768ae167c53f"
Fixed #6785 -- Made QuerySet.get() fetch a limited number of rows.
Co-authored-by: Tim Graham <timog...@gmail.com>
Co-authored-by: Patryk Zawadzki <pat...@room-303.com>
Co-Authored-By: Mariusz Felisiak <felisiak...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/6785#comment:19>