Re: [Django] #12581: connection.queries should use a ring buffer to avoid large memory consumption under runserver
Django
consumption under runserver
-------------------------------------+-------------------------------------
Reporter: robhudson | 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: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by bmispelon):
* cc: bmispelon@… (added)
* needs_better_patch: 1 => 0
* version: => master
* needs_tests: 0 => 1
Comment:
The situation has changed a bit since the original patch was written.
`collections.deque` now takes a `maxlen` argument in its constructor,
making the need of a custom `RingBuffer` implementation obsolete.
The patch becomes much simpler too: only two lines need to be changed:
https://github.com/bmispelon/django/compare/master...ticket-12581
Regarding the potential problems with django debug toolbar, I haven't
looked into it very thoroughly but it seemed to work on my first attempt.
The original patch had tests for the `RingBuffer` implementation which are
not needed anymore since we rely 100% on python's stdlib now.
We should still test the new feature somehow but I'm not quite sure how to
achieve this.
I'll look into this a bit deeper but if anyone has ideas on how to
implement the tests, or some test failures with django-debug-toolbar, I'd
love some feedback.
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:11>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
consumption under runserver
-------------------------------------+-------------------------------------
Reporter: robhudson | 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: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by bmispelon):
I had to tweak `django.test.utils.CapturedQueriesContext` in order to get
the test suite to run but after that, all the tests pass without further
modification.
I also took a closer look at how this affects DDT and it really seems to
run without any problem. It's not actually affected by the change since it
has its own logic for counting queries (I tried setting the `maxlen` to 1
and DDT would still show the correct number of queries).
One thing I'm concerned about is hardcoding this `maxlen` value to 100,
but I don't know how to do it in a more flexible manner (creating another
setting is likely out of question).
I'll look into adding some tests and maybe some documentation in the
upcoming days.
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:12>
Django
consumption under runserver
-------------------------------------+-------------------------------------
Reporter: robhudson | 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: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by charettes):
A `maxlen` of 100 looks like sensible default to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:13>
Django
cast integer to text
-------------------------------------+-------------------------------------
Reporter: robhudson | 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: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
At least since magic-removal (which is the horizon of events as far as I
am concerned), `connections.queries` is reset at the end of each request-
response cycle by `django.db.reset_queries`. I fail to see how it could
grow indefinitely. I must have missed something; could someone explain
what problem this ticket is trying to solve and how to reproduce it?
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:14>
Django
consumption under runserver
Reporter: robhudson | 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: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
Restore the summary...
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:15>
Django
consumption under runserver
-------------------------------------+-------------------------------------
Reporter: robhudson | 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: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by carljm):
It can grow indefinitely when the Django ORM is used outside the
request/response cycle, in long-running data-processing tasks.
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:16>
Django
consumption under runserver
-------------------------------------+-------------------------------------
Reporter: robhudson | 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: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
I understand that, however, the summary talks of runserver which doesn't
exhibit this behavior.
If we want to protect against memory exhaustion only in long running
processes, can we put a limit that's outside the realm of plausibility for
a single request eg. 10 000?
I'm not wild about changing the type of `connection.queries`, but as long
as the DDT works...
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:17>
Django
consumption under runserver
-------------------------------------+-------------------------------------
Reporter: robhudson | 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: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by claudep):
#21245 has been marked as a duplicate (and includes a valid use case).
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:18>
Django
consumption under runserver
-------------------------------------+-------------------------------------
Reporter: robhudson | 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: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: pztrick44@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:19>
Django
consumption under runserver
-------------------------------------+-------------------------------------
Reporter: robhudson | 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: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
The `self.queries = deque(maxlen=100)` doesn't survive when the query log
is reset with `connection.queries = []`. The bulk_create tests do that.
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:20>
Django
consumption under runserver
-------------------------------------+-------------------------------------
Reporter: robhudson | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
Severity: Normal | checkin
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
I'm going to complete this patch. However, an important aspect hasn't been
considered yet. `connection.queries` is documented in faq/models.txt as "a
list of dictionaries in order of query execution". We cannot change its
type.
To preserve it, we can use a deque internally and provide a `queries`
property that turns the deque into a list. The property should also emit a
warning when the limit is reached and queries are truncated.
This requires making `connection.queries` read-only. I suspect some people
use `connection.queries = []` to reset queries, even though the
documentation only talks about `db.reset_queries()`. But since there's a
documented API, I don't feel too bad about breaking undocumented usage.
I've set a high limit to minimize chances that people will hit it -- if
it's over 9000, you're really in trouble -- and I've made it a class
attribute to make it possible to change.
Pull request: https://github.com/django/django/pull/2772
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:21>
Django
consumption under runserver
-------------------------------------+-------------------------------------
Reporter: robhudson | Owner: nobody
Cleanup/optimization | Version: master
Component: Database layer | Resolution: fixed
(models, ORM) | Triage Stage: Ready for
Severity: Normal | checkin
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"cfcca7ccce3dc527d16757ff6dc978e50c4a2e61"]:
{{{
#!CommitTicketReference repository=""
revision="cfcca7ccce3dc527d16757ff6dc978e50c4a2e61"
Fixed #3711, #6734, #12581 -- Bounded connection.queries.
Prevented unlimited memory consumption when running background tasks
with DEBUG=True.
Thanks Rob, Alex, Baptiste, and others.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:22>
Django
consumption under runserver
-------------------------------------+-------------------------------------
Reporter: robhudson | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Database layer | Resolution: fixed
(models, ORM) | Triage Stage: Ready for
Severity: Normal | checkin
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"fc31319752945d4d39f70c8278ecd42af240bc64"]:
{{{
#!CommitTicketReference repository=""
revision="fc31319752945d4d39f70c8278ecd42af240bc64"
Fixed test again. Refs #12581.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:23>
Django
consumption under runserver
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
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
-------------------------------------+-------------------------------------
Comment (by nessita <124304+nessita@…>):
In [changeset:"5e81a4e7900f105971f332efd1702b5dd7f628ac" 5e81a4e]:
{{{#!CommitTicketReference repository=""
revision="5e81a4e7900f105971f332efd1702b5dd7f628ac"
Refs #12581 -- Adjusted warning stacklevel in queries ring buffer.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/12581#comment:24>