From 50328f0a618674b7143d86acaa7016c5293e9774 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anssi=20K=C3=A4=C3=A4ri=C3=A4inen?= Date: Wed, 20 Feb 2013 03:11:54 +0200 Subject: [PATCH] Fixed #19861 -- Transaction ._dirty flag improvement There were a couple of errors in ._dirty flag handling: * It started as None, but was never reset to None. * The _dirty flag was sometimes used to indicate if the connection was inside transaction management, but this was not done consistently. This also meant the flag had three separate values. * The None value had a special meaning, causing for example inability to commit() on new connection unless enter/leave tx management was done. * The _dirty was tracking "connection in transaction" state, but only in managed transactions. * Some tests never reset the transaction state of the used connection. * And some additional less important changes. This commit has some potential for regressions, but as the above list shows, the current situation isn't perfect either. --- django/db/backends/__init__.py | 43 +++---- django/db/backends/creation.py | 2 +- .../db/backends/postgresql_psycopg2/base.py | 9 ++ .../backends/postgresql_psycopg2/creation.py | 2 + django/db/backends/util.py | 10 +- django/db/models/sql/compiler.py | 5 - docs/ref/models/querysets.txt | 5 - tests/delete_regress/tests.py | 4 +- tests/middleware/tests.py | 13 +- tests/select_for_update/tests.py | 23 ++-- tests/transactions/tests.py | 2 - tests/transactions_regress/tests.py | 112 +++++++++++++++++- 12 files changed, 161 insertions(+), 69 deletions(-) diff --git a/django/db/backends/__init__.py b/django/db/backends/__init__.py index b77455e6e1d..46db1910f9e 100644 --- a/django/db/backends/__init__.py +++ b/django/db/backends/__init__.py @@ -41,7 +41,10 @@ class BaseDatabaseWrapper(object): # Transaction related attributes self.transaction_state = [] self.savepoint_state = 0 - self._dirty = None + # Tracks if the connection is believed to be in transaction. This is + # set somewhat aggressively, as the DBAPI doesn't make it easy to + # deduce if the connection is in transaction or not. + self._dirty = False self._thread_ident = thread.get_ident() self.allow_thread_sharing = allow_thread_sharing @@ -118,8 +121,7 @@ class BaseDatabaseWrapper(object): stack. """ if self._dirty: - self._rollback() - self._dirty = False + self.rollback() while self.transaction_state: self.leave_transaction_management() @@ -137,9 +139,6 @@ class BaseDatabaseWrapper(object): self.transaction_state.append(self.transaction_state[-1]) else: self.transaction_state.append(settings.TRANSACTIONS_MANAGED) - - if self._dirty is None: - self._dirty = False self._enter_transaction_management(managed) def leave_transaction_management(self): @@ -153,14 +152,16 @@ class BaseDatabaseWrapper(object): else: raise TransactionManagementError( "This code isn't under transaction management") + # The _leave_transaction_management hook can change the dirty flag, + # so memoize it. + dirty = self._dirty # 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: + if dirty: self.rollback() raise TransactionManagementError( "Transaction managed block ended with pending COMMIT/ROLLBACK") - self._dirty = False def validate_thread_sharing(self): """ @@ -190,11 +191,7 @@ class BaseDatabaseWrapper(object): to decide in a managed block of code to decide whether there are open changes waiting for commit. """ - if self._dirty is not None: - self._dirty = True - else: - raise TransactionManagementError("This code isn't under transaction " - "management") + self._dirty = True def set_clean(self): """ @@ -202,10 +199,7 @@ class BaseDatabaseWrapper(object): to decide in a managed block of code to decide whether a commit or rollback should happen. """ - if self._dirty is not None: - self._dirty = False - else: - raise TransactionManagementError("This code isn't under transaction management") + self._dirty = False self.clean_savepoints() def clean_savepoints(self): @@ -233,8 +227,7 @@ class BaseDatabaseWrapper(object): if top: top[-1] = flag if not flag and self.is_dirty(): - self._commit() - self.set_clean() + self.commit() else: raise TransactionManagementError("This code isn't under transaction " "management") @@ -245,7 +238,7 @@ class BaseDatabaseWrapper(object): """ self.validate_thread_sharing() if not self.is_managed(): - self._commit() + self.commit() self.clean_savepoints() else: self.set_dirty() @@ -256,7 +249,7 @@ class BaseDatabaseWrapper(object): """ self.validate_thread_sharing() if not self.is_managed(): - self._rollback() + self.rollback() else: self.set_dirty() @@ -343,6 +336,7 @@ class BaseDatabaseWrapper(object): if self.connection is not None: self.connection.close() self.connection = None + self.set_clean() def cursor(self): self.validate_thread_sharing() @@ -485,14 +479,13 @@ class BaseDatabaseFeatures(object): self.connection.managed(True) cursor = self.connection.cursor() cursor.execute('CREATE TABLE ROLLBACK_TEST (X INT)') - self.connection._commit() + self.connection.commit() cursor.execute('INSERT INTO ROLLBACK_TEST (X) VALUES (8)') - self.connection._rollback() + self.connection.rollback() cursor.execute('SELECT COUNT(X) FROM ROLLBACK_TEST') count, = cursor.fetchone() cursor.execute('DROP TABLE ROLLBACK_TEST') - self.connection._commit() - self.connection._dirty = False + self.connection.commit() finally: self.connection.leave_transaction_management() return count == 0 diff --git a/django/db/backends/creation.py b/django/db/backends/creation.py index 77c9e6c9e63..70c24bc820a 100644 --- a/django/db/backends/creation.py +++ b/django/db/backends/creation.py @@ -385,8 +385,8 @@ class BaseDatabaseCreation(object): # Create the test database and connect to it. We need to autocommit # if the database supports it because PostgreSQL doesn't allow # CREATE/DROP DATABASE statements within transactions. - cursor = self.connection.cursor() self._prepare_for_test_db_ddl() + cursor = self.connection.cursor() try: cursor.execute( "CREATE DATABASE %s %s" % (qn(test_database_name), suffix)) diff --git a/django/db/backends/postgresql_psycopg2/base.py b/django/db/backends/postgresql_psycopg2/base.py index 85a0991402c..bf129c07585 100644 --- a/django/db/backends/postgresql_psycopg2/base.py +++ b/django/db/backends/postgresql_psycopg2/base.py @@ -149,6 +149,8 @@ class DatabaseWrapper(BaseDatabaseWrapper): exc_info=sys.exc_info() ) raise + finally: + self.set_clean() @cached_property def pg_version(self): @@ -233,10 +235,17 @@ class DatabaseWrapper(BaseDatabaseWrapper): try: if self.connection is not None: self.connection.set_isolation_level(level) + if level == psycopg2.extensions.ISOLATION_LEVEL_AUTOCOMMIT: + self.set_clean() finally: self.isolation_level = level self.features.uses_savepoints = bool(level) + def set_dirty(self): + if ((self.transaction_state and self.transaction_state[-1]) or + not self.features.uses_autocommit): + super(DatabaseWrapper, self).set_dirty() + def _commit(self): if self.connection is not None: try: diff --git a/django/db/backends/postgresql_psycopg2/creation.py b/django/db/backends/postgresql_psycopg2/creation.py index 88afd5f52f2..d977939f410 100644 --- a/django/db/backends/postgresql_psycopg2/creation.py +++ b/django/db/backends/postgresql_psycopg2/creation.py @@ -82,6 +82,8 @@ class DatabaseCreation(BaseDatabaseCreation): def _prepare_for_test_db_ddl(self): """Rollback and close the active transaction.""" + # Make sure there is an open connection. + self.connection.cursor() self.connection.connection.rollback() self.connection.connection.set_isolation_level( psycopg2.extensions.ISOLATION_LEVEL_AUTOCOMMIT) diff --git a/django/db/backends/util.py b/django/db/backends/util.py index 1ba23060e07..ebab982a049 100644 --- a/django/db/backends/util.py +++ b/django/db/backends/util.py @@ -19,13 +19,9 @@ class CursorWrapper(object): self.cursor = cursor self.db = db - def set_dirty(self): - if self.db.is_managed(): - self.db.set_dirty() - def __getattr__(self, attr): if attr in ('execute', 'executemany', 'callproc'): - self.set_dirty() + self.db.set_dirty() return getattr(self.cursor, attr) def __iter__(self): @@ -35,7 +31,7 @@ class CursorWrapper(object): class CursorDebugWrapper(CursorWrapper): def execute(self, sql, params=()): - self.set_dirty() + self.db.set_dirty() start = time() try: return self.cursor.execute(sql, params) @@ -52,7 +48,7 @@ class CursorDebugWrapper(CursorWrapper): ) def executemany(self, sql, param_list): - self.set_dirty() + self.db.set_dirty() start = time() try: return self.cursor.executemany(sql, param_list) diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index 22d025ecb2e..b9d25d98a17 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -687,11 +687,6 @@ class SQLCompiler(object): resolve_columns = hasattr(self, 'resolve_columns') fields = None has_aggregate_select = bool(self.query.aggregate_select) - # Set transaction dirty if we're using SELECT FOR UPDATE to ensure - # a subsequent commit/rollback is executed, so any database locks - # are released. - if self.query.select_for_update and transaction.is_managed(self.using): - transaction.set_dirty(self.using) for rows in self.execute_sql(MULTI): for row in rows: if resolve_columns: diff --git a/docs/ref/models/querysets.txt b/docs/ref/models/querysets.txt index f77f87dd8e9..0fa8b8e361b 100644 --- a/docs/ref/models/querysets.txt +++ b/docs/ref/models/querysets.txt @@ -1249,11 +1249,6 @@ make the call non-blocking. If a conflicting lock is already acquired by another transaction, :exc:`~django.db.DatabaseError` will be raised when the queryset is evaluated. -Note that using ``select_for_update()`` will cause the current transaction to be -considered dirty, if under transaction management. This is to ensure that -Django issues a ``COMMIT`` or ``ROLLBACK``, releasing any locks held by the -``SELECT FOR UPDATE``. - Currently, the ``postgresql_psycopg2``, ``oracle``, and ``mysql`` database backends support ``select_for_update()``. However, MySQL has no support for the ``nowait`` argument. Obviously, users of external third-party backends should diff --git a/tests/delete_regress/tests.py b/tests/delete_regress/tests.py index e007ebdd76a..9fcc19ba715 100644 --- a/tests/delete_regress/tests.py +++ b/tests/delete_regress/tests.py @@ -23,11 +23,13 @@ class DeleteLockingTest(TransactionTestCase): # Put both DB connections into managed transaction mode transaction.enter_transaction_management() transaction.managed(True) - self.conn2._enter_transaction_management(True) + self.conn2.enter_transaction_management() + self.conn2.managed(True) def tearDown(self): # Close down the second connection. transaction.leave_transaction_management() + self.conn2.abort() self.conn2.close() @skipUnlessDBFeature('test_db_allows_multiple_connections') diff --git a/tests/middleware/tests.py b/tests/middleware/tests.py index e6512e4aceb..4e5fd8ea6b6 100644 --- a/tests/middleware/tests.py +++ b/tests/middleware/tests.py @@ -683,6 +683,9 @@ class TransactionMiddlewareTest(TransactionTestCase): self.response = HttpResponse() self.response.status_code = 200 + def tearDown(self): + transaction.abort() + def test_request(self): TransactionMiddleware().process_request(self.request) self.assertTrue(transaction.is_managed()) @@ -697,10 +700,14 @@ class TransactionMiddlewareTest(TransactionTestCase): self.assertEqual(Band.objects.count(), 1) def test_unmanaged_response(self): + transaction.enter_transaction_management() transaction.managed(False) + self.assertEqual(Band.objects.count(), 0) TransactionMiddleware().process_response(self.request, self.response) self.assertFalse(transaction.is_managed()) - self.assertFalse(transaction.is_dirty()) + # The transaction middleware doesn't commit/rollback if management + # has been disabled. + self.assertTrue(transaction.is_dirty()) def test_exception(self): transaction.enter_transaction_management() @@ -708,8 +715,8 @@ class TransactionMiddlewareTest(TransactionTestCase): Band.objects.create(name='The Beatles') self.assertTrue(transaction.is_dirty()) TransactionMiddleware().process_exception(self.request, None) - self.assertEqual(Band.objects.count(), 0) self.assertFalse(transaction.is_dirty()) + self.assertEqual(Band.objects.count(), 0) def test_failing_commit(self): # It is possible that connection.commit() fails. Check that @@ -724,8 +731,8 @@ class TransactionMiddlewareTest(TransactionTestCase): self.assertTrue(transaction.is_dirty()) with self.assertRaises(IntegrityError): TransactionMiddleware().process_response(self.request, None) - self.assertEqual(Band.objects.count(), 0) self.assertFalse(transaction.is_dirty()) + self.assertEqual(Band.objects.count(), 0) self.assertFalse(transaction.is_managed()) finally: del connections[DEFAULT_DB_ALIAS].commit diff --git a/tests/select_for_update/tests.py b/tests/select_for_update/tests.py index e3e4d9e7e28..c5a04881c91 100644 --- a/tests/select_for_update/tests.py +++ b/tests/select_for_update/tests.py @@ -24,7 +24,7 @@ requires_threading = unittest.skipUnless(threading, 'requires threading') class SelectForUpdateTests(TransactionTestCase): def setUp(self): - transaction.enter_transaction_management(True) + transaction.enter_transaction_management() transaction.managed(True) self.person = Person.objects.create(name='Reinhardt') @@ -48,9 +48,8 @@ class SelectForUpdateTests(TransactionTestCase): try: # We don't really care if this fails - some of the tests will set # this in the course of their run. - transaction.managed(False) - transaction.leave_transaction_management() - self.new_connection.leave_transaction_management() + transaction.abort() + self.new_connection.abort() except transaction.TransactionManagementError: pass self.new_connection.close() @@ -73,7 +72,7 @@ class SelectForUpdateTests(TransactionTestCase): def end_blocking_transaction(self): # Roll back the blocking transaction. - self.new_connection._rollback() + self.new_connection.rollback() def has_for_update_sql(self, tested_connection, nowait=False): # Examine the SQL that was executed to determine whether it @@ -119,6 +118,7 @@ class SelectForUpdateTests(TransactionTestCase): """ self.start_blocking_transaction() status = [] + thread = threading.Thread( target=self.run_select_for_update, args=(status,), @@ -164,7 +164,7 @@ class SelectForUpdateTests(TransactionTestCase): try: # We need to enter transaction management again, as this is done on # per-thread basis - transaction.enter_transaction_management(True) + transaction.enter_transaction_management() transaction.managed(True) people = list( Person.objects.all().select_for_update(nowait=nowait) @@ -177,6 +177,7 @@ class SelectForUpdateTests(TransactionTestCase): finally: # This method is run in a separate thread. It uses its own # database connection. Close it without waiting for the GC. + transaction.abort() connection.close() @requires_threading @@ -271,13 +272,3 @@ class SelectForUpdateTests(TransactionTestCase): """ people = list(Person.objects.select_for_update()) self.assertTrue(transaction.is_dirty()) - - @skipUnlessDBFeature('has_select_for_update') - def test_transaction_not_dirty_unmanaged(self): - """ If we're not under txn management, the txn will never be - marked as dirty. - """ - transaction.managed(False) - transaction.leave_transaction_management() - people = list(Person.objects.select_for_update()) - self.assertFalse(transaction.is_dirty()) diff --git a/tests/transactions/tests.py b/tests/transactions/tests.py index f3246eef2af..a1edf53fcb7 100644 --- a/tests/transactions/tests.py +++ b/tests/transactions/tests.py @@ -165,7 +165,6 @@ class TransactionRollbackTests(TransactionTestCase): def execute_bad_sql(self): cursor = connection.cursor() cursor.execute("INSERT INTO transactions_reporter (first_name, last_name) VALUES ('Douglas', 'Adams');") - transaction.set_dirty() @skipUnlessDBFeature('requires_rollback_on_dirty_transaction') def test_bad_sql(self): @@ -306,5 +305,4 @@ class TransactionContextManagerTests(TransactionTestCase): with transaction.commit_on_success(): cursor = connection.cursor() cursor.execute("INSERT INTO transactions_reporter (first_name, last_name) VALUES ('Douglas', 'Adams');") - transaction.set_dirty() transaction.rollback() diff --git a/tests/transactions_regress/tests.py b/tests/transactions_regress/tests.py index e76208d72ca..8cc68b7e0d6 100644 --- a/tests/transactions_regress/tests.py +++ b/tests/transactions_regress/tests.py @@ -138,7 +138,8 @@ class TestTransactionClosing(TransactionTestCase): @transaction.commit_on_success def create_system_user(): "Create a user in a transaction" - user = User.objects.create_user(username='system', password='iamr00t', email='root@SITENAME.com') + user = User.objects.create_user(username='system', password='iamr00t', + email='root@SITENAME.com') # Redundant, just makes sure the user id was read back from DB Mod.objects.create(fld=user.pk) @@ -161,6 +162,83 @@ class TestTransactionClosing(TransactionTestCase): """ self.test_failing_query_transaction_closed() +@skipIf(connection.vendor == 'sqlite' and + (connection.settings_dict['NAME'] == ':memory:' or + not connection.settings_dict['NAME']), + 'Test uses multiple connections, but in-memory sqlite does not support this') +class TestNewConnection(TransactionTestCase): + """ + Check that new connections don't have special behaviour. + """ + def setUp(self): + self._old_backend = connections[DEFAULT_DB_ALIAS] + settings = self._old_backend.settings_dict.copy() + opts = settings['OPTIONS'].copy() + if 'autocommit' in opts: + opts['autocommit'] = False + settings['OPTIONS'] = opts + new_backend = self._old_backend.__class__(settings, DEFAULT_DB_ALIAS) + connections[DEFAULT_DB_ALIAS] = new_backend + + def tearDown(self): + try: + connections[DEFAULT_DB_ALIAS].abort() + except Exception: + import ipdb; ipdb.set_trace() + finally: + connections[DEFAULT_DB_ALIAS].close() + connections[DEFAULT_DB_ALIAS] = self._old_backend + + def test_commit(self): + """ + Users are allowed to commit and rollback connections. + """ + # The starting value is False, not None. + self.assertIs(connection._dirty, False) + list(Mod.objects.all()) + self.assertTrue(connection.is_dirty()) + connection.commit() + self.assertFalse(connection.is_dirty()) + list(Mod.objects.all()) + self.assertTrue(connection.is_dirty()) + connection.rollback() + self.assertFalse(connection.is_dirty()) + + def test_enter_exit_management(self): + orig_dirty = connection._dirty + connection.enter_transaction_management() + connection.leave_transaction_management() + self.assertEqual(orig_dirty, connection._dirty) + + def test_commit_unless_managed(self): + cursor = connection.cursor() + cursor.execute("INSERT into transactions_regress_mod (fld) values (2)") + connection.commit_unless_managed() + self.assertFalse(connection.is_dirty()) + self.assertEqual(len(Mod.objects.all()), 1) + self.assertTrue(connection.is_dirty()) + connection.commit_unless_managed() + self.assertFalse(connection.is_dirty()) + + def test_commit_unless_managed_in_managed(self): + cursor = connection.cursor() + connection.enter_transaction_management() + transaction.managed(True) + cursor.execute("INSERT into transactions_regress_mod (fld) values (2)") + connection.commit_unless_managed() + self.assertTrue(connection.is_dirty()) + connection.rollback() + self.assertFalse(connection.is_dirty()) + self.assertEqual(len(Mod.objects.all()), 0) + connection.commit() + connection.leave_transaction_management() + self.assertFalse(connection.is_dirty()) + self.assertEqual(len(Mod.objects.all()), 0) + self.assertTrue(connection.is_dirty()) + connection.commit_unless_managed() + self.assertFalse(connection.is_dirty()) + self.assertEqual(len(Mod.objects.all()), 0) + @skipUnless(connection.vendor == 'postgresql', "This test only valid for PostgreSQL") @@ -171,9 +249,11 @@ class TestPostgresAutocommit(TransactionTestCase): """ def setUp(self): from psycopg2.extensions import (ISOLATION_LEVEL_AUTOCOMMIT, - ISOLATION_LEVEL_READ_COMMITTED) + ISOLATION_LEVEL_READ_COMMITTED, + TRANSACTION_STATUS_IDLE) self._autocommit = ISOLATION_LEVEL_AUTOCOMMIT self._read_committed = ISOLATION_LEVEL_READ_COMMITTED + self._idle = TRANSACTION_STATUS_IDLE # We want a clean backend with autocommit = True, so # first we need to do a bit of work to have that. @@ -186,7 +266,11 @@ class TestPostgresAutocommit(TransactionTestCase): connections[DEFAULT_DB_ALIAS] = new_backend def tearDown(self): - connections[DEFAULT_DB_ALIAS] = self._old_backend + try: + connections[DEFAULT_DB_ALIAS].abort() + finally: + connections[DEFAULT_DB_ALIAS].close() + connections[DEFAULT_DB_ALIAS] = self._old_backend def test_initial_autocommit_state(self): self.assertTrue(connection.features.uses_autocommit) @@ -214,6 +298,26 @@ class TestPostgresAutocommit(TransactionTestCase): transaction.leave_transaction_management() self.assertEqual(connection.isolation_level, self._autocommit) + def test_enter_autocommit(self): + transaction.enter_transaction_management() + transaction.managed(True) + self.assertEqual(connection.isolation_level, self._read_committed) + list(Mod.objects.all()) + self.assertTrue(transaction.is_dirty()) + # Enter autocommit mode again. + transaction.enter_transaction_management(False) + transaction.managed(False) + self.assertFalse(transaction.is_dirty()) + self.assertEqual( + connection.connection.get_transaction_status(), + self._idle) + list(Mod.objects.all()) + self.assertFalse(transaction.is_dirty()) + transaction.leave_transaction_management() + self.assertEqual(connection.isolation_level, self._read_committed) + transaction.leave_transaction_management() + self.assertEqual(connection.isolation_level, self._autocommit) + class TestManyToManyAddTransaction(TransactionTestCase): def test_manyrelated_add_commit(self): @@ -247,7 +351,7 @@ class SavepointTest(TransactionTestCase): work() - @skipIf(connection.vendor == 'mysql' and \ + @skipIf(connection.vendor == 'mysql' and connection.features._mysql_storage_engine == 'MyISAM', "MyISAM MySQL storage engine doesn't support savepoints") @skipUnlessDBFeature('uses_savepoints')