--
Ticket URL: <https://code.djangoproject.com/ticket/21134>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* status: new => assigned
* needs_better_patch: => 0
* component: Documentation => Database layer (models, ORM)
* needs_tests: => 0
* version: 1.5 => 1.6-beta-1
* owner: nobody => aaugustin
* needs_docs: => 0
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
I added a warning to the docs earlier today, see [4db2752e] and
[0ad178c4].
Anssi also suggested that Django should raise an error when attempting to
make database queries while a broken transaction is in progress. This
would improve cross-database consistency, normalizing to PostgreSQL's
behavior.
Here's a patch implementing this idea:
https://github.com/RichardOfWard/django/commit/cb46c75db275db59b54511c090286255bd9cc46d
Full discussion: https://groups.google.com/d/topic/django-developers
/ACLQRF-71s8/discussion
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:1>
* status: assigned => closed
* resolution: => fixed
* component: Database layer (models, ORM) => Documentation
Comment:
I attempted to write a patch for the behavior described above and in fact
it can't be made to work reliable, because not every SQL query is wrapped
is `transaction.atomic(savepoint=False)`. I'm attaching the patch at the
point I gave up, for future reference.
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:2>
Comment (by akaariai):
The committed documentation changes aren't about what I have been
complaining about. I have said numerous times that *this is not about
DatabaseErrors*. This is about any error that bubbles out of
atomic(savepoint=False) block.
The problem is this:
{{{
r1 = Reporter.objects.create(first_name='foo', last_name='bar')
with transaction.atomic():
r2 = Reporter(first_name='foo', last_name='bar2', id=r1.id)
try:
r2.save(force_insert=True)
except IntegrityError:
r2.save(force_update=True)
self.assertEqual(Reporter.objects.get(pk=r1.pk).last_name, 'bar2')
}}}
The last assert fails. This is unexpected for anybody using a database
where this pattern is allowed (as far as I know any db other than
PostgreSQL). Note that there isn't any explicit use of
atomic(savepoint=False) blocks. Also, the exception *does not need to be
DatabaseError subclass at all*.
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:3>
Comment (by aaugustin):
In practice there isn't any difference between "DatabaseErrors" and
"errors that bubble out of atomic(savepoint=False)". The only non-fatal
exceptions expected inside an atomic(savepoint=False) block in Django are
DatabaseErrors.
After taking into account the warning I added, your example becomes:
{{{
r1 = Reporter.objects.create(first_name='foo', last_name='bar')
with transaction.atomic():
r2 = Reporter(first_name='foo', last_name='bar2', id=r1.id)
try:
with transaction.atomic():
r2.save(force_insert=True)
except IntegrityError:
r2.save(force_update=True)
self.assertEqual(Reporter.objects.get(pk=r1.pk).last_name, 'bar2')
}}}
which behaves correctly.
But it's unrealistic to hope that everyone will do that without
complaining, especially people used to databases with a loose (non-atomic)
transaction implementation.
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:4>
* status: closed => new
* has_patch: 0 => 1
* resolution: fixed =>
Comment:
PR: https://github.com/django/django/pull/1659
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:5>
Comment (by aaugustin):
This pull request prevents running queries in `atomic` blocks that are
going to end with a rollback.
Since `atomic` is a new API, this change looks like it's backwards
compatible. Unfortunately, it isn't, because some features already take
advantage of `atomic` internally.
Specifically `django.test.TestCase` wraps each test in an `atomic` block.
As a consequence, tests that ran queries after database errors will
require modifications, even if the query is a rollback to a previous
savepoint. That said:
- If it's a test for transactional behavior, it should be in a
`TransactionTestCase`. Quite obviously high-level and low-level APIs
cannot be mixed arbitrarily.
- If the savepoint is used to recover from a database error, it's much
easier to recover with an `atomic` block. This is how I fixed the few
tests that were broken by this change in Django's test suite.
If we decide to go with this approach, I'll add this to the transactions
docs or the release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:6>
* status: new => assigned
* needs_better_patch: 0 => 1
Comment:
This patch breaks two tests under PostgreSQL when running the full test
suite — not in isolation:
{{{
======================================================================
ERROR: test_alter_unique_together
(migrations.test_operations.OperationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/Users/myk/Documents/dev/django/tests/migrations/test_operations.py",
line 256, in test_alter_unique_together
operation.database_backwards("test_alunto", editor, new_state,
project_state)
File
"/Users/myk/Documents/dev/django/django/db/migrations/operations/models.py",
line 121, in database_backwards
return self.database_forwards(app_label, schema_editor, from_state,
to_state)
File
"/Users/myk/Documents/dev/django/django/db/migrations/operations/models.py",
line 117, in database_forwards
getattr(new_model._meta, "unique_together", set()),
File "/Users/myk/Documents/dev/django/django/db/backends/schema.py",
line 271, in alter_unique_together
", ".join(columns),
ValueError: Found wrong number (0) of constraints for
test_alunto_pony(pink, weight)
======================================================================
ERROR: test_unique_together (schema.tests.SchemaTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/myk/Documents/dev/django/tests/schema/tests.py", line 430,
in test_unique_together
[],
File "/Users/myk/Documents/dev/django/django/db/backends/schema.py",
line 271, in alter_unique_together
", ".join(columns),
ValueError: Found wrong number (0) of constraints for
schema_uniquetest(year, slug)
----------------------------------------------------------------------
}}}
It also breaks one test on MySQL, because it's the only database that
can't defer constraints checks automatically:
{{{
======================================================================
ERROR: test_loaddata_error_message (fixtures.tests.FixtureLoadingTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/myk/Documents/dev/django/tests/fixtures/tests.py", line
324, in test_loaddata_error_message
management.call_command('loaddata', 'invalid.json', verbosity=0)
File
"/Users/myk/Documents/dev/django/django/core/management/__init__.py", line
159, in call_command
return klass.execute(*args, **defaults)
File "/Users/myk/Documents/dev/django/django/core/management/base.py",
line 289, in execute
output = self.handle(*args, **options)
File
"/Users/myk/Documents/dev/django/django/core/management/commands/loaddata.py",
line 55, in handle
self.loaddata(fixture_labels)
File
"/Users/myk/Documents/dev/django/django/core/management/commands/loaddata.py",
line 84, in loaddata
self.load_label(fixture_label)
File
"/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py",
line 35, in __exit__
self.gen.throw(type, value, traceback)
File "/Users/myk/Documents/dev/django/django/db/backends/__init__.py",
line 417, in constraint_checks_disabled
self.enable_constraint_checking()
File "/Users/myk/Documents/dev/django/django/db/backends/mysql/base.py",
line 490, in enable_constraint_checking
self.cursor().execute('SET foreign_key_checks=1')
File "/Users/myk/Documents/dev/django/django/db/backends/utils.py", line
47, in execute
self.db.validate_no_broken_transaction()
File "/Users/myk/Documents/dev/django/django/db/backends/__init__.py",
line 367, in validate_no_broken_transaction
"An error occurred in the current transaction. You can't "
TransactionManagementError: An error occurred in the current transaction.
You can't execute queries until the end of the 'atomic' block.
----------------------------------------------------------------------
}}}
Under Oracle it appears to be fine (I'm not getting a clean run because my
Oracle test setup is weird, but all the failures I see are explained by
other factors).
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:7>
Comment (by apollo13):
Neither bisect nor pair for the migrations tests triggered an error here;
bisecting schema is currently hard due to other errors.
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:8>
Comment (by aaugustin):
I added a commit to the PR to fix loaddata on MySQL. It isn't great;
better ideas welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:9>
Comment (by andrewgodwin):
I just pulled the branch and tested against the most recent master and got
no test failures. I just committed some test fixes to schema/migrations,
so that might be helping.
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:10>
Comment (by aaugustin):
This is getting interesting: I can reproduce the above failures with
PostgreSQL on one machine but not on another. Both are Mac Book Pros with
the latest OS X, Python and Postgres from MacPorts.
:(
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:11>
Comment (by Harm Geerts <hgeerts@…>):
I receive the same failure on master with postgres-9.3.0
The test fails because the constraint columns from introspection are
returned in reversed order so it doesn't match the existing constraint in
_constraint_names()
https://github.com/django/django/blob/master/django/db/backends/schema.py#L720
e.g. [year, slug] == [slug, year] fails.
Even when using a new connection on the test database the order remains
reversed.
However, when using an explicit collate it will return in the expected
order.
The database was created with encoding/collation nl_NL.UTF-8
First query is equal to what introspection.get_constraints() uses.
The second uses COLLATE "C" and the last uses COLLATE "nk_NL".
So somewhere in the test suite postgres gets confused and switches
collation to something odd?
{{{
test_django_default=# SELECT
kc.constraint_name,
kc.column_name,
c.constraint_type,
array(SELECT table_name::text || '.' || column_name::text
FROM information_schema.constraint_column_usage WHERE constraint_name =
kc.constraint_name)
FROM information_schema.key_column_usage AS kc
JOIN information_schema.table_constraints AS c ON
kc.table_schema = c.table_schema AND
kc.table_name = c.table_name AND
kc.constraint_name = c.constraint_name
WHERE
kc.table_schema = 'public' AND
kc.table_name = 'schema_uniquetest';
constraint_name | column_name | constraint_type |
array
---------------------------------+-------------+-----------------+-------------------------------------------------
schema_uniquetest_year_slug_key | slug | UNIQUE |
{schema_uniquetest.year,schema_uniquetest.slug}
schema_uniquetest_year_slug_key | year | UNIQUE |
{schema_uniquetest.year,schema_uniquetest.slug}
schema_uniquetest_pkey | id | PRIMARY KEY |
{schema_uniquetest.id}
(3 rows)
test_django_default=# SELECT
kc.constraint_name,
kc.column_name,
c.constraint_type,
array(SELECT table_name::text || '.' || column_name::text
FROM information_schema.constraint_column_usage WHERE constraint_name =
kc.constraint_name)
FROM information_schema.key_column_usage AS kc
JOIN information_schema.table_constraints AS c ON
kc.table_schema = c.table_schema AND
kc.table_name = c.table_name AND
kc.constraint_name = c.constraint_name
WHERE
kc.table_schema = 'public' AND
kc.table_name = 'schema_uniquetest' COLLATE "C";
constraint_name | column_name | constraint_type |
array
---------------------------------+-------------+-----------------+-------------------------------------------------
schema_uniquetest_pkey | id | PRIMARY KEY |
{schema_uniquetest.id}
schema_uniquetest_year_slug_key | year | UNIQUE |
{schema_uniquetest.year,schema_uniquetest.slug}
schema_uniquetest_year_slug_key | slug | UNIQUE |
{schema_uniquetest.year,schema_uniquetest.slug}
(3 rows)
test_django_default=# SELECT
kc.constraint_name,
kc.column_name,
c.constraint_type,
array(SELECT table_name::text || '.' || column_name::text
FROM information_schema.constraint_column_usage WHERE constraint_name =
kc.constraint_name)
FROM information_schema.key_column_usage AS kc
JOIN information_schema.table_constraints AS c ON
kc.table_schema = c.table_schema AND
kc.table_name = c.table_name AND
kc.constraint_name = c.constraint_name
WHERE
kc.table_schema = 'public' AND
kc.table_name = 'schema_uniquetest' COLLATE "nl_NL";
constraint_name | column_name | constraint_type |
array
---------------------------------+-------------+-----------------+-------------------------------------------------
schema_uniquetest_pkey | id | PRIMARY KEY |
{schema_uniquetest.id}
schema_uniquetest_year_slug_key | year | UNIQUE |
{schema_uniquetest.year,schema_uniquetest.slug}
schema_uniquetest_year_slug_key | slug | UNIQUE |
{schema_uniquetest.year,schema_uniquetest.slug}
(3 rows)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:12>
Comment (by akaariai):
The query is missing ordering - the db is free to return the rows in any
order it wishes. Collation changing result ordering is a bit surprising,
but that doesn't mean PostgreSQL is doing anything wrong here.
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:13>
Comment (by Harm Geerts <hgeerts@…>):
I got the exact same response on #postgresql
[00:33:51] <urth> I'm getting an odd ordering problem with
postgresql-9.3.0, unless I explicitly collate the query the rows are
returned in reversed order http://pgsql.privatepaste.com/c3e1e066ea
[00:34:58] <RhodiumToad> urth: I see no ORDER BY there?
[00:35:28] <RhodiumToad> urth: without ORDER BY, the result is returned in
whatever order suits the planner's whim, and will often change between
queries
[00:39:08] <urth> RhodiumToad: the query is part of a testcase for django,
if that test is run on it's own it succeeds, but with the full test suite
it fails for some. can other queries on the database influence the
ordering used by the planner?
[00:39:25] <RhodiumToad> yes
[00:39:50] <urth> ok, thanks
[00:40:47] <RhodiumToad> the rules are quite clear: without an order by at
the top query level, the order of the result of any query or subquery is
indeterminate
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:14>
Comment (by andrewgodwin):
There should be a column in that table that we can use for ORDER BY to get
the columns returned in the correct order. I'll take a look.
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:15>
Comment (by Andrew Godwin <andrew@…>):
In [changeset:"59582a811913466784f90506619882a0555e022e"]:
{{{
#!CommitTicketReference repository=""
revision="59582a811913466784f90506619882a0555e022e"
Enforce ordering on PostgreSQL get_constraints cols (refs #21134)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:16>
* component: Documentation => Database layer (models, ORM)
* severity: Normal => Release blocker
Comment:
Changing flags since we're heading towards code changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:17>
* needs_docs: 0 => 1
* needs_better_patch: 1 => 0
Comment:
I rebased the pull request on top of Andrew's fixes and I confirmed it
passes the test suite on SQLite, PostgreSQL and MySQL. I have no reason to
suspect it no longer passes it on Oracle.
The code itself is RFC; this ticket still needs:
- a message on django-developers to explain the tradeoffs,
- additional explanations in the documentation.
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:18>
* needs_docs: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
This looks good to me.
I wonder if there should be an example of the probably most common case of
actually needing set_rollback(), that is `try: obj.save() except
IntegrityError: # do something else` on MySQL or Oracle for example. But
this can be added later on. I have a feeling we are going to get some
feedback on this anyways...
From the options available to us, marking the outer block for rollback and
forbidding queries seems to be the best one (it is not great, but neither
are any of the other options). So, marking as RFC.
For reference, my understanding is that here are the options:
- make atomic(savepoint=False) a no-op when inside transaction - leads to
it being non-atomic.
- always use savepoints - heavy cost for some operations (mostly multiple
obj.save() in a row)
- mark outer block for rollback, allow queries. This works great for
read-only queries, but once you write in the marked for rollback block it
seems things work OK, but your modifications are silently rolled back.
- mark outer block for rollback, forbid data modification queries -
django has no way to know which queries modify data
- mark outer block for rollback, forbid all queries - forces users to
explicitly continue the transaction if that is needed. Doing so might be
dangerous. But it is their explicit decision to do so. It might be
annoying for those who are accustomed to MySQL/Oracle way of continuing
transaction after IntegrityError.
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:19>
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"0d74bdaf0c39feb8ec303dbbdbcadba70e46eecb"]:
{{{
#!CommitTicketReference repository=""
revision="0d74bdaf0c39feb8ec303dbbdbcadba70e46eecb"
Fixed #21134 -- Prevented queries in broken transactions.
Backport of 728548e4 from master.
Squashed commit of the following:
commit 63ddb271a44df389b2c302e421fc17b7f0529755
Author: Aymeric Augustin <aymeric....@m4x.org>
Date: Sun Sep 29 22:51:00 2013 +0200
Clarified interactions between atomic and exceptions.
commit 2899ec299228217c876ba3aa4024e523a41c8504
Author: Aymeric Augustin <aymeric....@m4x.org>
Date: Sun Sep 22 22:45:32 2013 +0200
Fixed TransactionManagementError in tests.
Previous commit introduced an additional check to prevent running
queries in transactions that will be rolled back, which triggered a
few
failures in the tests. In practice using transaction.atomic instead of
the low-level savepoint APIs was enough to fix the problems.
commit 4a639b059ea80aeb78f7f160a7d4b9f609b9c238
Author: Aymeric Augustin <aymeric....@m4x.org>
Date: Tue Sep 24 22:24:17 2013 +0200
Allowed nesting constraint_checks_disabled inside atomic.
Since MySQL handles transactions loosely, this isn't a problem.
commit 2a4ab1cb6e83391ff7e25d08479e230ca564bfef
Author: Aymeric Augustin <aymeric....@m4x.org>
Date: Sat Sep 21 18:43:12 2013 +0200
Prevented running queries in transactions that will be rolled back.
This avoids a counter-intuitive behavior in an edge case on databases
with non-atomic transaction semantics.
It prevents using savepoint_rollback() inside an atomic block without
calling set_rollback(False) first, which is backwards-incompatible in
tests.
Refs #21134.
commit 8e3db393853c7ac64a445b66e57f3620a3fde7b0
Author: Aymeric Augustin <aymeric....@m4x.org>
Date: Sun Sep 22 22:14:17 2013 +0200
Replaced manual savepoints by atomic blocks.
This ensures the rollback flag is handled consistently in internal
APIs.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21134#comment:20>