Fixes #1239

[saemideluxe]

This is just PoC. I wouldn't know how to test this properly.

[matthiask]

Oh wow, that's all it took? Do you have any idea how many times the cursor got wrapped? "Only" twice or more?

[codecov]

Codecov Report

Merging #1475 (7e2c161) into main (ebd4ae3) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1475      +/-   ##
==========================================
- Coverage   86.34%   86.26%   -0.09%     
==========================================
  Files          35       35              
  Lines        1853     1856       +3     
  Branches      260      261       +1     
==========================================
+ Hits         1600     1601       +1     
- Misses        181      182       +1     
- Partials       72       73       +1     

Impacted Files	Coverage Δ
debug_toolbar/panels/sql/tracking.py	88.03% <100.00%> (+0.31%)	⬆️
debug_toolbar/panels/profiling.py	86.60% <0.00%> (-1.79%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebd4ae3...7e2c161. Read the comment docs.

[carltongibson]

FWIW This is ≈ how I fixed it when I hit this issue.

I got stumped on how to write a test for this, and never had the time to get past that, but...

[matthiask]

@carltongibson Thanks :)

@saemideluxe We get some test failures on PostgreSQL; the chunked cursor test fails. Do you have the time to look into this?

[saemideluxe]

@matthiask I will look into over the weekend.

[saemideluxe]

The test for the __wrapper__ attribute was a bit shortsighted ;) Testing the type of the cursor instance makes more sense and the tests do pass.

What I do not understand is why with postgresql we get do not get a wrapped cursor but with other databases we get a wrapped cursor... Maybe this is related to the fact that test_recording_chunked_cursor is only executed for postgresql connections? I saw that there are a few special cases for connection.vendor == "postgresql"

[matthiask]

The test for the __wrapper__ attribute was a bit shortsighted ;) Testing the type of the cursor instance makes more sense and the tests do pass.

Nice, thanks! Is there a reason why you're checking for NormalCursorWrapper and not state.Wrapper? Maybe it doesn't matter much but using the same type in the isinstance call as below would be less confusing for me.

What I do not understand is why with postgresql we get do not get a wrapped cursor but with other databases we get a wrapped cursor... Maybe this is related to the fact that test_recording_chunked_cursor is only executed for postgresql connections? I saw that there are a few special cases for connection.vendor == "postgresql"

I'm not sure and didn't dig into it. My stance is that the code is perfect if tests pass (well not really but it's somewhat true)

[saemideluxe]

Right, just changed that to state.Wrapper.

[matthiask]

Thanks!

[jayaddison]

Is there any chance we'd want a similar fix for the non-chunked cursor equivalent, or does #1239 only affect chunked fetches for some reason?

[saemideluxe]

Is there any chance we'd want a similar fix for the non-chunked cursor equivalent, or does #1239 only affect chunked fetches for some reason?

According to the conversation in #1239 it only seems to affect chunked cursors (also my experience). The initial author investigated the behaviour somewhat it seems. But I couldn't rule it out for sure because I did not have the time to dig into the cause of having wrapped cursors in the first place.

[jayaddison]

Ahh, ok, thanks @saemideluxe - yep, this is starting to make more sense to me now. I'm repeating a bit of the discussion I suppose, but hopefully it's worthwhile to understand.

If I understand it now, it's the fact that -- for some database providers -- a method call to chunked_cursor calls the cursor method, and so both of those methods add a level of wrapping at call-time, without the fix.

To put it another way: for the problem to appear, both cursor methods have to be called -- usually accidentally -- in the context of the same database query, and in the same call stack.

The fix uses the fact chunked_cursor may be implemented in terms of the standard cursor -- in other words, chunked_cursor is a bit like an extension or override of the default, always-available cursor implementation. The reverse situation (a standard cursor being implemented in terms of chunked_cursor) is much less likely to occur.

Supporting the approach, certainly Django since way back considers database support for chunked reads to be an optional ability, and the introduction of chunked_cursor provides cursor as the default implementation for chunked_cursor.

[saemideluxe]

Thanks for the elaborations and the links @jayaddison
👍

[jayaddison]

No problem, thank you for the fix :) It'll make SQL performance debugging much less confusing for a lot of Django users.

There is a possible way to test this in #1478, although I think that's slightly incompatible at the moment.

I have another question during writing the tests: is isinstance(cursor, state.Wrapper) safe to use as a check? (cc @matthiask) That's a @property method, and could evaluate to either a NormalCursorWrapper or a ExceptionCursorWrapper based on the thread state. Might it make sense to have a base CursorWrapper class that both of them inherit from, and then use isinstance on that?

[matthiask]

That's a @Property method, and could evaluate to either a NormalCursorWrapper or a ExceptionCursorWrapper based on the thread state. Might it make sense to have a base CursorWrapper class that both of them inherit from, and then use isinstance on that?

Since there's no way for this attribute to change between the isinstance Check and the wrapping (except when running in a preemptive multitasking environment (threading), which Django doesn't support anyway) this should be safe. At least I cannot imagine a scenario where it wouldn't be.

The common base class may still be a good idea for other reasons but I'm not sure it's necessary.

[jayaddison]

Makes sense, thanks @matthiask. I've opened #1479 with that possible refactor.