Code

Opened 7 years ago

Closed 4 years ago

#5415 closed (wontfix)

Every SQL query should send a signal

Reported by: adrian Owned by: gav
Component: Database layer (models, ORM) Version: master
Severity: Keywords: feature_request
Cc: kenneth.arnold@…, gonz 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)

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 7 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 gav 7 years ago.
Example middleware demonstrating use of DB signals, against r6702.
db_signal_r6702.2.patch (8.7 KB) - added by gav 7 years ago.
db_signal_r6702.patch (9.1 KB) - added by gav 7 years ago.
Adds optional enable of signal classes.
db_signal_r6979.patch (7.7 KB) - added by gav 7 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 7 years ago by adrian

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 7 years ago by adrian

  • Description modified (diff)

comment:3 Changed 7 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 7 years ago by PhiR

  • Keywords feature_request added

Changed 7 years ago by czhang

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

comment:5 Changed 7 years ago by gav

  • 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 7 years ago by gav

Example middleware demonstrating use of DB signals, against r6702.

Changed 7 years ago by gav

comment:6 Changed 7 years ago by gav

  • 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 7 years ago by mtredinnick

  • Triage Stage changed from Accepted to Design 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 7 years ago by gav

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 7 years ago by adrian

  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

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 7 years ago by gav

Adds optional enable of signal classes.

comment:10 Changed 7 years ago by gav

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 7 years ago by gav

  • Owner changed from nobody to gav
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Accepted to Ready for checkin

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

comment:12 Changed 7 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 7 years ago by gav

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 7 years ago by mtredinnick

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 7 years ago by gav

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 7 years ago by mtredinnick

*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 7 years ago by gav

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

comment:17 Changed 7 years ago by gav

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 6 years ago by kcarnold

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 6 years ago by kcarnold

  • Cc kenneth.arnold@… added

comment:20 Changed 5 years ago by gonz

  • Cc gonz added

comment:21 Changed 4 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 4 years ago by gav

  • 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 4 years ago by Alex

  • Resolution set to wontfix
  • Status changed from assigned to closed

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.