Get PostgreSQL transaction tracking working #1619
Conversation
There was a problem hiding this comment.
Thank you for the well written pull request. There's a lot of good information in the individual commits. I think summarizing some of that into comments would help future devs understand what things are doing and why, but I'm content without them for the most part.
living180
force-pushed
the
sql_transaction_tracking
branch
2 times, most recently
from
9eb9790 to
ada10e1
Compare
|
Thanks so much for the review. I hope I have adequately addressed the points you raised. I added comments to the parts that I felt were non-obvious from the code. If there is anything still unclear, either from the comments that I added or that I have not yet commented, please let me know. |
living180
force-pushed
the
sql_transaction_tracking
branch
from
ada10e1 to
90880e2
Compare
living180
force-pushed
the
sql_transaction_tracking
branch
from
90880e2 to
684f0e2
Compare
dj32 was inadvertently removed from the -postgresql testenv section.
The _djdt_chunked_cursor attribute wasn't being cleaned up in the unwrap_cursor() method.
If the cursor()/chunked_cursor() methods of a connection had already been monkey patched by some other code before wrap_cursor() was called, unwrap_cursor() would undo the previous monkey patch as well as the one performed by wrap_cursor(). This can occur when testing if multiple databases are defined but only some of them are allowed to be used by the tests. [1] Django's SimpleTestCase wraps the connections for any disallowed databases to raise an exception if they are accessed. [2] Without this commit, unwrap_cursor() was undoing Django's monkey patch, resulting in an exception when Django tried to undo its monkey patch which was no longer there. Update unwrap_cursor() to preserve a previous monkey patch if present. [1] https://docs.djangoproject.com/en/stable/topics/testing/tools/#django.test.SimpleTestCase.databases [2] https://github.com/django/django/blob/ce586ed6931092d3a5f06df9031cdeb891793ddb/django/test/testcases.py#L350
Ensure that queries made to different databases get recorded with the correct alias.
BaseDatabaseWrapper always has an alias attribute. [1] [1] https://github.com/django/django/blob/aa28c392b9491f02330905cb73b7078b1cd18c60/django/db/backends/base/base.py#L70
The vendor attribute does not exist on the underlying DB connection.
Previously, the logic for determining when to switch transaction IDs
was contained in SQLPanel.get_transaction_id(). However, this was not
sufficient, as it only looked at the transaction status after the query
was finished. If two queries were issued consecutively but in separate
transactions, such as in the following:
with transaction.atomic():
list(User.objects.all())
with transaction.atomic():
list(Group.objects.all())
both queries would be given the same transaction ID since they would
both have the same transaction status after being executed
(TRANSACTION_STATUS_INTRANS). Instead, move the logic to
NormalCursorWrapper._record() and compare the transaction status before
the query to the transaction status after the query to determine if a
new transaction was started.
Also, use the conn.status attribute for determining transaction status
instead of conn.get_transaction_status(), since the former is a local
connection variable, while the latter requires a call to the server.
The logic in SQLPanel.generate_stats() did not properly account for the DB alias when marking transactions as ended. It would always mark the previous query as ending the transaction even if the previous query was from a different DB alias. Update the code to track the last query by alias so that the correct query can be marked as ending the transaction.
living180
force-pushed
the
sql_transaction_tracking
branch
from
684f0e2 to
8269153
Compare
There was a problem hiding this comment.
It's a lot to review. I think the code looks good (better than before), tests pass and I've tested it a bit in a local environment.
I am fine with merging this but I'll wait for a moment to see if @tim-schilling has some reservations.
|
Thanks @matthiask and @tim-schilling for the reviews! One question for future reference - would it be helpful if I included relevant change log entries with future PRs? |
Sure! This would be helpful. |
There were bugs at multiple levels that prevented PostgreSQL transactions from being tracked properly. This PR hopefully fixes all of those, plus adds a test for this functionality.