Every SQL query should send a signal

[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.

[Jacob]

\*gulp\*

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

[czhang]

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

[George Vilches]

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.

[George Vilches]

Example middleware demonstrating use of DB signals, against r6702.

[George Vilches]

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?

[Malcolm Tredinnick]

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.

[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).

[Adrian Holovaty]

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).

[George Vilches]

Adds optional enable of signal classes.

[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?

[George Vilches]

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

[Malcolm Tredinnick]

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.

[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.

[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.

[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. :)

[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.

[George Vilches]

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

[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? :)

[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.

[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.

[George Vilches]

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.

[Alex Gaynor]

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