Opened 9 years ago

Closed 6 years ago

#5415 closed (wontfix)

Every SQL query should send a signal

Reported by: Adrian Holovaty Owned by: George Vilches
Component: Database layer (models, ORM) Version: master
Severity: Keywords: feature_request
Cc: kenneth.arnold@…, Gonzalo Saavedra Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description (last modified by Adrian Holovaty)

Every SQL query should send a signal, using django.core.signals. This will enable all sorts of interesting and useful things, such as logging and debugging functionality.

This should happen at the database backend level, not at the ORM level.

Once this change has been made, django.db.connection.queries should either be removed or refactored to use the signal.

One concern: Are signals fast enough? See #4561.

Attachments (5)

5415.diff (1.2 KB) - added by czhang 9 years ago.
I tried to come up this simple patch, but I have no idea how to test this properly.
sql.py (2.0 KB) - added by George Vilches 9 years ago.
Example middleware demonstrating use of DB signals, against r6702.
db_signal_r6702.2.patch (8.7 KB) - added by George Vilches 9 years ago.
db_signal_r6702.patch (9.1 KB) - added by George Vilches 9 years ago.
Adds optional enable of signal classes.
db_signal_r6979.patch (7.7 KB) - added by George Vilches 9 years ago.
Changes to DB signals against r6979 in response to Malcolm's requests.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 9 years ago by Adrian Holovaty

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 9 years ago by Adrian Holovaty

Description: modified (diff)

comment:3 Changed 9 years ago by Jacob

\*gulp\*

I don't think signals are (currently) fast enough for this, but in theory at least this could be a neat feature.

comment:4 Changed 9 years ago by Philippe Raoult

Keywords: feature_request added

Changed 9 years ago by czhang

Attachment: 5415.diff added

I tried to come up this simple patch, but I have no idea how to test this properly.

comment:5 Changed 9 years ago by George Vilches

Has patch: set

I've done a patch for this, which covers the new signal, a cursor wrapper for every query for handling signal dispatching, and updating the unit tests so they all pass again. Patch is again r6702.

Also, I've attached a middleware example of how you might use this signal for tracking and debugging. It's not great, but it's an example.

Changed 9 years ago by George Vilches

Attachment: sql.py added

Example middleware demonstrating use of DB signals, against r6702.

Changed 9 years ago by George Vilches

Attachment: db_signal_r6702.2.patch added

comment:6 Changed 9 years ago by George Vilches

Needs documentation: set

An aside: docs/faq.txt refers to this ticket, as it explains how to use connection.queries. Is it preferable to update that documentation to show how to use the signal, or if the middleware provided above is accepted, just explain how to use that, and maybe a hand-waving reference at the signal dispatch?

comment:7 Changed 9 years ago by Malcolm Tredinnick

Triage Stage: AcceptedDesign decision needed

I'm a big -1 on this. The extra performance overhead is way too much and we shouldn't be discouraging people from running database queries. At a minimum, I would think it would have to be entirely optional and off by default, perhaps controlled in a fashion similar to manual transactions.

comment:8 Changed 9 years ago by George Vilches

I agree with the potential performance concern, I just filled out Adrian's request. :)

Two possibilities that spring to mind easily:

1) We could make it so the CursorSignalWrapper is only used if settings.DEBUG is on, or preferably by a settings.DATABASE_DEBUG flag, so query signals can be controlled separately? Possible concern: this isn't easy to change at runtime, since the cursor is already defined as soon as the DB connection is instantiated, so it would be rare that this could be called early enough. Fine for simple uses (always on, always off).

2) A mechanism, something like django.db.connection.enable_signals() which wraps the cursor on the connection at that moment. Easier to change at runtime, and can still use (1) to decide whether to trigger it by default.

Unrelated: if #4561 is fixed (specifically the part "signals are sent regardless of whether or not something is listening"), would your concern still stand in any significant way? Is it strictly the dispatcher load, or is it just that CursorSignalWrapper is another object. If it's CursorSignalWrapper, how is this different than the old CursorDebugWrapper? They perform much the same role as far as load goes (the only difference being the dispatcher, so back to the first part of the question).

comment:9 Changed 9 years ago by Adrian Holovaty

Patch needs improvement: set
Triage Stage: Design decision neededAccepted

Malcolm -- yes, this would be entirely optional and off by default. I'm wanting this feature purely so that #5416 can be done. We shouldn't use the signals/dispatching framework if that's too slow (which it looks like it is).

Changed 9 years ago by George Vilches

Attachment: db_signal_r6702.patch added

Adds optional enable of signal classes.

comment:10 Changed 9 years ago by George Vilches

The current patch now makes CursorSignalWrapper optional in the way of #2. It works if settings.DEBUG is on, or can be activated at runtime by doing the following:

>>> from django import db
>>> db.connection.use_signal_cursor = True

Does that cover the concerns about performance and being off by default?

comment:11 Changed 9 years ago by George Vilches

Owner: changed from nobody to George Vilches
Patch needs improvement: unset
Status: newassigned
Triage Stage: AcceptedReady for checkin

Needs a final review, bumping status to "Ready for Checkin" per Malcolm's request.

comment:12 Changed 9 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

I have concerns. This seem to remove the current default DebugCursor behaviour, or am I missing something? Where are the queries now logged and times stored? Also, you seem to have arbitrarily renamed the cursor class, which is backwards-incompatible. Let's try to make this as minimally intrusive as possible.

comment:13 Changed 9 years ago by George Vilches

This does replace the DebugCursorWrapper with SignalCursorWrapper and the sql.py middleware referenced above, since SignalCursorWrapper's functionality somewhat supercedes DebugCursorWrapper. The reasoning was that this still leaves us with only one built-in debugging mechanism (SignalCursorWrapper is *always* enabled when DEBUG is True, so we can rely on it for that situation), which is less maintenance and less method overhead (we don't have the problem of calling through two CursorWrapper classes). If someone wants different debugging behavior, then they use the signal their own way and just ignore our built-in middleware. The debugging element doesn't have to be a middleware, but at the middleware point post_response was the latest point that we could do the work, so if any other middlewares before this one run DB queries, we get that data as well (unlike most of the other uses of django.db.connection.queries embedded in templates rendered through the view, which will lead to missing any queries executed after that point).

If removing CursorDebugWrapper is too big a backwards-incompatible change (that's a pretty deep internal, and for part of the ticket we're already removing django.db.connection.queries since Adrian requested it above (refactor was an option as well, but storing the queries in the database connector itself doesn't seem like a great idea). So, if we want to prevent backwards-incompatibility, we'll have to add a signal into the backends and have that populate the queries list like before, but that sort of defeats a good portion of the reason for trying to separate the functionality, we're still cluttering the backend with this data.

However, if backwards-compatibility is absolutely necessary in this case, I can definitely generate the patch to maintain both Cursor wrappers, and it will just call through the stack if both are enabled. They're both safe wrappers, they could stack easily enough if we have to.

comment:14 Changed 9 years ago by Malcolm Tredinnick

Since we're only using these signals for debugging, why not simply add them to the existing debug cursor? Let's not over-engineer this.

comment:15 Changed 9 years ago by George Vilches

So keep DebugCursorWrapper just like it is, and not refactor django.db.connection.queries to use the signal? From adrian's original comment in the ticket:

"Once this change has been made, django.db.connection.queries should either be removed or refactored to use the signal."

Should I just ignore this? If I should, then I'll make the patch based on that now. (There's no reason to do this if we keep using DebugCursorWrapper as it is and just stack the signals on top of it). I just want to make sure that I don't ignore adrian's request unless another committer tells me it's okay to do so. :)

comment:16 Changed 9 years ago by Malcolm Tredinnick

*sigh* I don't know. Right now you've renamed a class that is used in a lot of places and removed some functionality that is used all over the place (logging the queries, etc). They're my two problems. Whatever else you and Adrian decide to do to implement this feature that I don't care about is fine by me, but let's try to avoid breaking existing code, please.

Changed 9 years ago by George Vilches

Attachment: db_signal_r6979.patch added

Changes to DB signals against r6979 in response to Malcolm's requests.

comment:17 Changed 9 years ago by George Vilches

This should be 100% backwards-compatible with the current solution, tested against r6979. CursorDebugWrapper is exactly what it used to, as is connection.queries. All the additional signal functionality exists and still functions as described above, including the ability to control it via db.connection.use_signal_cursor = True. Does this alleviate your concerns?

The tests have been updated and show both the django.db.connection.queries log and a record maintained through use of the new signal, verifying that the recorded information is the same. All unit tests pass here.

Waiting for confirmation that this is acceptable, and then I will add usage docs to allow for checkin. Malcolm? :)

comment:18 Changed 8 years ago by Kenneth Arnold

Are the newly refactored signals fast enough for this? I have a more useful query time tracker in the works, and it would be nice to not have to patch Django to hook it in.

comment:19 Changed 8 years ago by Kenneth Arnold

Cc: kenneth.arnold@… added

comment:20 Changed 7 years ago by Gonzalo Saavedra

Cc: Gonzalo Saavedra added

comment:21 Changed 7 years ago by tonyarkles

Any thoughts on this? I'm working on a project that could really use this functionality, and I'd be willing to work on the patch and shepherd it through.

comment:22 Changed 7 years ago by George Vilches

Patch needs improvement: unset

I can bring my last patch up to current, but we need a committer to weigh in. I satisfied all of Malcolm's listed requests with the last patch. Obviously this won't go in till at least the 1.3 cycle, but we need someone to champion it anyway.

comment:23 Changed 6 years ago by Alex Gaynor

Resolution: wontfix
Status: assignedclosed

Wontfixing on the grounds that between connection.queries and the new logging stuff there's nothing that a signal really adds.

Note: See TracTickets for help on using tickets.
Back to Top