Code

Opened 4 years ago

Last modified 6 months ago

#12581 new Cleanup/optimization

connection.queries should use a ring buffer to avoid large memory consumption under runserver

Reported by: robhudson Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: rob@…, bmispelon@…, pztrick44@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, when running using runserver, each query is appended to a list. For long running runserver processes, this list can grow quite large.

Attachments (1)

12581.diff (3.0 KB) - added by robhudson 4 years ago.
RingBuffer, based on connections.deque (Python 2.4+)

Download all attachments as: .zip

Change History (20)

Changed 4 years ago by robhudson

RingBuffer, based on connections.deque (Python 2.4+)

comment:1 Changed 4 years ago by robhudson

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago by robhudson

FYI: Original patch by Alex Gaynor, tests and ticket added by Rob Hudson.

comment:3 Changed 4 years ago by russellm

  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Accepted
  • Version 1.1 deleted

comment:4 Changed 4 years ago by ubernostrum

  • milestone 1.2 deleted

1.2 is feature-frozen, moving this feature request off the milestone.

comment:5 Changed 4 years ago by adurdin

Note that the django debug toolbar’s sql panel does not work with this patch.

comment:6 Changed 3 years ago by aaugustin

This is a duplicate of #3711 (closed as wontfix) and #6734 (closed as duplicate).

Since the ticket was accepted by a core dev, I won't close it, though.

comment:7 Changed 3 years ago by mattmcc

  • Severity set to Normal
  • Type set to New feature

comment:8 Changed 3 years ago by julien

  • Patch needs improvement set
  • Type changed from New feature to Cleanup/optimization

The tests would need to be rewritten using unittests since this is now Django's preferred way. Also, the datastructures tests seem to have been moved to source:django/trunk/tests/regressiontests/utils/datastructures.py

comment:9 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:11 Changed 12 months ago by bmispelon

  • Cc bmispelon@… added
  • Needs tests set
  • Patch needs improvement unset
  • Version set to master

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.

comment:12 Changed 12 months ago 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.

comment:13 Changed 12 months ago by charettes

A maxlen of 100 looks like sensible default to me.

comment:14 Changed 12 months ago by aaugustin

  • Summary changed from connection.queries should use a ring buffer to avoid large memory consumption under runserver to connection.queries should use a ring buffer to avoid large memory consumption under runserverontrib.comments inline on Postgres 8.3 fails to cast integer to text

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?

comment:15 Changed 12 months ago by aaugustin

  • Summary changed from connection.queries should use a ring buffer to avoid large memory consumption under runserverontrib.comments inline on Postgres 8.3 fails to cast integer to text to connection.queries should use a ring buffer to avoid large memory consumption under runserver

Restore the summary...

comment:16 Changed 12 months ago by carljm

It can grow indefinitely when the Django ORM is used outside the request/response cycle, in long-running data-processing tasks.

comment:17 Changed 12 months ago 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...

comment:18 Changed 6 months ago by claudep

#21245 has been marked as a duplicate (and includes a valid use case).

comment:19 Changed 6 months ago by pztrick

  • Cc pztrick44@… added

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.