Every SQL query should send a signal

\*gulp\*

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

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

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.

Example middleware demonstrating use of DB signals, against r6702.

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?

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.

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

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

Adds optional enable of signal classes.

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?

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

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.

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.

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.

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

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

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

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