Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fixed #16047 -- Restore autocommit state correctly on psycopg2

When the postgresql_psycopg2 backend was used with DB-level autocommit
mode enabled, after entering transaction management and then leaving
it, the isolation level was never set back to autocommit mode.

Thanks brodie for report and working on this issue.
  • Loading branch information...
commit f572ee0c65ec5eac9edb0cb3e35c96ec86d89ffb 1 parent da573fb
@akaariai akaariai authored
View
14 django/db/backends/__init__.py
@@ -109,16 +109,18 @@ def leave_transaction_management(self):
over to the surrounding block, as a commit will commit all changes, even
those from outside. (Commits are on connection level.)
"""
- self._leave_transaction_management(self.is_managed())
if self.transaction_state:
del self.transaction_state[-1]
else:
- raise TransactionManagementError("This code isn't under transaction "
- "management")
+ raise TransactionManagementError(
+ "This code isn't under transaction management")
+ # We will pass the next status (after leaving the previous state
+ # behind) to subclass hook.
+ self._leave_transaction_management(self.is_managed())
if self._dirty:
self.rollback()
- raise TransactionManagementError("Transaction managed block ended with "
- "pending COMMIT/ROLLBACK")
+ raise TransactionManagementError(
+ "Transaction managed block ended with pending COMMIT/ROLLBACK")
self._dirty = False
def validate_thread_sharing(self):
@@ -176,6 +178,8 @@ def is_managed(self):
"""
if self.transaction_state:
return self.transaction_state[-1]
+ # Note that this setting isn't documented, and is only used here, and
+ # in enter_transaction_management()
return settings.TRANSACTIONS_MANAGED
def managed(self, flag=True):
View
9 docs/releases/1.5.txt
@@ -168,6 +168,15 @@ number was inside the existing page range.
It does check it now and raises an :exc:`InvalidPage` exception when the number
is either too low or too high.
+Behavior of autocommit database option on PostgreSQL changed
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+PostgreSQL's autocommit option didn't work as advertised previously. It did
+work for single transaction block, but after the first block was left the
+autocommit behavior was never restored. This bug is now fixed in 1.5. While
+this is only a bug fix, it is worth checking your applications behavior if
+you are using PostgreSQL together with the autocommit option.
+
Features deprecated in 1.5
==========================
View
60 tests/regressiontests/transactions_regress/tests.py
@@ -1,11 +1,11 @@
from __future__ import absolute_import
from django.core.exceptions import ImproperlyConfigured
-from django.db import connection, transaction
+from django.db import connection, connections, transaction, DEFAULT_DB_ALIAS
from django.db.transaction import commit_on_success, commit_manually, TransactionManagementError
from django.test import TransactionTestCase, skipUnlessDBFeature
from django.test.utils import override_settings
-from django.utils.unittest import skipIf
+from django.utils.unittest import skipIf, skipUnless
from .models import Mod, M2mA, M2mB
@@ -175,6 +175,62 @@ def test_failing_query_transaction_closed_debug(self):
self.test_failing_query_transaction_closed()
+class TestPostgresAutocommit(TransactionTestCase):
+ """
+ Tests to make sure psycopg2's autocommit mode is restored after entering
+ and leaving transaction management. Refs #16047.
+ """
+ def setUp(self):
+ from psycopg2.extensions import (ISOLATION_LEVEL_AUTOCOMMIT,
+ ISOLATION_LEVEL_READ_COMMITTED)
+ self._autocommit = ISOLATION_LEVEL_AUTOCOMMIT
+ self._read_committed = ISOLATION_LEVEL_READ_COMMITTED
+
+ # We want a clean backend with autocommit = True, so
+ # first we need to do a bit of work to have that.
+ self._old_backend = connections[DEFAULT_DB_ALIAS]
+ settings = self._old_backend.settings_dict.copy()
+ opts = settings['OPTIONS'].copy()
+ opts['autocommit'] = True
+ settings['OPTIONS'] = opts
+ new_backend = self._old_backend.__class__(settings, DEFAULT_DB_ALIAS)
+ connections[DEFAULT_DB_ALIAS] = new_backend
+
+ def test_initial_autocommit_state(self):
+ self.assertTrue(connection.features.uses_autocommit)
+ self.assertEqual(connection.isolation_level, self._autocommit)
+
+ def test_transaction_management(self):
+ transaction.enter_transaction_management()
+ transaction.managed(True)
+ self.assertEqual(connection.isolation_level, self._read_committed)
+
+ transaction.leave_transaction_management()
+ self.assertEqual(connection.isolation_level, self._autocommit)
+
+ def test_transaction_stacking(self):
+ transaction.enter_transaction_management()
+ transaction.managed(True)
+ self.assertEqual(connection.isolation_level, self._read_committed)
+
+ transaction.enter_transaction_management()
+ self.assertEqual(connection.isolation_level, self._read_committed)
+
+ transaction.leave_transaction_management()
+ self.assertEqual(connection.isolation_level, self._read_committed)
+
+ transaction.leave_transaction_management()
+ self.assertEqual(connection.isolation_level, self._autocommit)
+
+ def tearDown(self):
+ connections[DEFAULT_DB_ALIAS] = self._old_backend
+
+TestPostgresAutocommit = skipUnless(connection.vendor == 'postgresql',
+ "This test only valid for PostgreSQL")(TestPostgresAutocommit)
+TestPostgresAutoCommit = skipUnlessDBFeature('supports_transactions')(
+ TestPostgresAutocommit)
+
+
class TestManyToManyAddTransaction(TransactionTestCase):
def test_manyrelated_add_commit(self):
"Test for https://code.djangoproject.com/ticket/16818"

4 comments on commit f572ee0

@claudep
Collaborator

I think that more modern syntax would use skipUnless and skipUnlessDBFeature as decorators. Other bikeshedding: generally, the tearDown method is always following setUp in Django tests.

@akaariai
Collaborator

For some reason moving the skips to decorators will skip the tests in all cases.

The diff is this

+@skipUnless(connection.vendor == 'postgresql',
+            "This test only valid for PostgreSQL")
+@skipUnlessDBFeature('supports_transactions')
 class TestPostgresAutocommit(TransactionTestCase):
     """
     Tests to make sure psycopg2's autocommit mode is restored after entering
@@ -225,11 +226,6 @@ class TestPostgresAutocommit(TransactionTestCase):
     def tearDown(self):
         connections[DEFAULT_DB_ALIAS] = self._old_backend

-TestPostgresAutocommit = skipUnless(connection.vendor == 'postgresql',
-    "This test only valid for PostgreSQL")(TestPostgresAutocommit)
-TestPostgresAutoCommit = skipUnlessDBFeature('supports_transactions')(
-    TestPostgresAutocommit)

With the change when running under PostgreSQL:
Ran 11 tests in 4.925s
With master:
Ran 14 tests in 6.307s

The problem seems to be the check for transaction_support. Luckily this isn't needed at all, AFAICS postgresql always supports transactions.

So, maybe I should remove the transaction_support check, move the vendor check to decorator and move the tearDown directly below setUp?

@claudep
Collaborator

Reading django/test/testcases.py, I suspect skipIfDBFeature/skipUnlessDBFeature cannot be used as class decorators (ticket #18551 created).
+1 to your last suggestion.

@akaariai
Collaborator

I committed the above changes in 925a693.

Please sign in to comment.
Something went wrong with that request. Please try again.