postgresql_psycopg2 never restores autocommit mode when leaving transaction management
|Reported by:||Brodie Rao||Owned by:||nobody|
|Component:||Database layer (models, ORM)||Version:||1.3|
|Severity:||Normal||Keywords:||psycopg2 autocommit transactions|
|Cc:||dick@…, ionel.mc@…, anssi.kaariainen@…, kmike84@…||Triage Stage:||Accepted|
|Has patch:||yes||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
When using the postgresql_psycopg2 backend with DB-level autocommit mode enabled, after entering transaction management and then leaving it, the isolation level on the psycopg2 connection is never set back to autocommit mode.
And to make matters worse, the isolation level on the DatabaseWrapper object stays set to READ COMMITTED, which means all new database connections made in the same process won't be using the AUTOCOMMIT isolation level.
This is particularly problematic with pgbouncer, which will essentially stop acting like a connection pool if you disconnect from the server mid-transaction. When using psycopg2 with the default READ COMMITTED isolation level, the database driver will implicitly open a transaction for each new connection. If the last query in the connection isn't .save() or .update(), Django won't have issued COMMIT or ROLLBACK. If Django closes the connection without cleaning up the transaction, pgbouncer can't safely reuse that connection. Thus it's forced to close the server connection, and you get an error stating "closing connection: unclean server".
Turning on autocommit mode and explicitly using transactions when they're needed is one way of mitigating that problem, but this bug prevents that workaround from being viable. I think the issue with not cleaning up implicitly opened transactions is probably worth a separate bug report, which I'd be happy to create if my description makes sense to others.
I'm attaching three patches to this ticket related to the original autocommit mode problem described above. The first patch adds two tests to modeltests.transactions.tests that confirm autocommit mode is initialized correctly and that it's restored correctly after leaving transaction management. The second patch corrects the DB feature tests for transaction support when using psycopg2's autocommit mode.
The third patch fixes the bug by correcting what seems to me to be a nonsensical conditional in DatabaseWrapper._leave_transaction_management(). Specifically, it only sets the isolation level back to autocommit mode if three conditions are satisfied:
- We've asked for DB-level autocommit mode to be enabled.
- We're not in managed mode (i.e., someone has called transaction.managed(False)).
- The isolation level isn't already set to AUTOCOMMIT.
The second check is what prevents the isolation level from ever being restored; neither the transaction decorators nor the transaction middleware ever call transaction.managed(False) before calling transaction.leave_transaction_management(). It's also likely that other people writing custom transaction decorators don't do this either, so it seems reasonable to remove the check. I can't think of any reason you'd want to keep the isolation level at READ COMMITTED when you've asked to leave transaction management and you've also asked for DB-level autocommit mode.
Also, I believe this bug has been present in the psycopg2 wrapper since autocommit mode was introduced. I also believe the second bug I mentioned has been present for many releases.