Fixed #22343 -- Disallowed select_for_update in autocommit mode #2533

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Member

shaib commented Apr 9, 2014

The ticket was originally about two failing tests, which are
fixed by putting their queries in transactions.

Thanks Tim Graham for the report and Simon Charette for review.

@shaib shaib Fixed #22343 -- Disallowed select_for_update in autocommit mode
The ticket was originally about two failing tests, which are
fixed by putting their queries in transactions.

Thanks Tim Graham for the report and Simon Charette for review.
0a773e5

@timgraham timgraham commented on an outdated diff Apr 9, 2014

docs/ref/models/querysets.txt
@@ -1365,9 +1365,19 @@ do not support ``nowait``, such as MySQL, will cause a
:exc:`~django.db.DatabaseError` to be raised. This is in order to prevent code
unexpectedly blocking.
+Executing a queryset with ``select_for_update`` in autocommit mode is
+an error, because the rows are then not locked. If allowed, this would
@timgraham

timgraham Apr 9, 2014

Owner

no comma after error

@timgraham timgraham commented on an outdated diff Apr 9, 2014

docs/releases/1.6.3.txt
@@ -5,7 +5,27 @@ Django 1.6.3 release notes
*Under development*
This is Django 1.6.3, a bugfix release for Django 1.6. Django 1.6.3 fixes
-several bugs in 1.6.2:
+several bugs in 1.6.2, and makes one backwards-incompatible change:

@timgraham timgraham commented on an outdated diff Apr 9, 2014

docs/releases/1.6.3.txt
@@ -5,7 +5,27 @@ Django 1.6.3 release notes
*Under development*
This is Django 1.6.3, a bugfix release for Django 1.6. Django 1.6.3 fixes
-several bugs in 1.6.2:
+several bugs in 1.6.2, and makes one backwards-incompatible change:
+
+``select_for_update()`` requires a transaction
+==============================================
+
+Historically, Queries that use
@timgraham

timgraham Apr 9, 2014

Owner

queries (lower case)

@timgraham timgraham commented on the diff Apr 9, 2014

docs/releases/1.7.txt
@@ -1086,6 +1086,23 @@ Note also that the admin login form has been updated to not contain the
``this_is_the_login_form`` field (now unused) and the ``ValidationError`` code
has been set to the more regular ``invalid_login`` key.
+``select_for_update()`` requires a transaction
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
@timgraham

timgraham Apr 9, 2014

Owner

A note that this was also fixed in Django 1.6.3 would be helpful.

@timgraham timgraham commented on the diff Apr 9, 2014

docs/ref/models/querysets.txt
@@ -1365,9 +1365,19 @@ do not support ``nowait``, such as MySQL, will cause a
:exc:`~django.db.DatabaseError` to be raised. This is in order to prevent code
unexpectedly blocking.
+Executing a queryset with ``select_for_update`` in autocommit mode is
@timgraham

timgraham Apr 9, 2014

Owner

I would either say "Executing a query" or "Evaluating a queryset". If there are other places where we say "Executing a queryset" feel free to leave it, just seems a bit odd to me.

@timgraham timgraham commented on the diff Apr 9, 2014

docs/releases/1.6.3.txt
@@ -5,7 +5,33 @@ Django 1.6.3 release notes
*Under development*
This is Django 1.6.3, a bugfix release for Django 1.6. Django 1.6.3 fixes
-several bugs in 1.6.2:
+several bugs in 1.6.2 and makes one backwards-incompatible change:
+
+``select_for_update()`` requires a transaction
+==============================================
+
+Historically, queries that use
+:meth:`~django.db.models.query.QuerySet.select_for_update()` could be
+executed in autocommit mode, outside of a transaction. Before Django
+1.6, Django's automatic transactions mode allowed this to be used to
+lock records until the next write operation. Django 1.6 introduced
+database-level autocommit; since then, execution in such a context
+voids the effect of ``select_for_update()``. It is, therefore, assumed
+now to be an error, and raises an exception.
@timgraham

timgraham Apr 9, 2014

Owner

remove comma (since "raises an exception" cannot be a sentence on its own)

@timgraham timgraham commented on the diff Apr 9, 2014

docs/releases/1.6.3.txt
+
+Historically, queries that use
+:meth:`~django.db.models.query.QuerySet.select_for_update()` could be
+executed in autocommit mode, outside of a transaction. Before Django
+1.6, Django's automatic transactions mode allowed this to be used to
+lock records until the next write operation. Django 1.6 introduced
+database-level autocommit; since then, execution in such a context
+voids the effect of ``select_for_update()``. It is, therefore, assumed
+now to be an error, and raises an exception.
+
+This change may cause test failures if you use ``select_for_update()``
+in a test class which is a subclass of
+:class:`~django.test.TransactionTestCase` rather than
+:class:`~django.test.TestCase`.
+
+This change was made because such errors can be caused by including an
@timgraham

timgraham Apr 9, 2014

Owner

I would move this above the previous paragraph since you refer to "such errors" which is from the 1st paragraph

@timgraham timgraham commented on the diff Apr 9, 2014

docs/releases/1.6.3.txt
+:meth:`~django.db.models.query.QuerySet.select_for_update()` could be
+executed in autocommit mode, outside of a transaction. Before Django
+1.6, Django's automatic transactions mode allowed this to be used to
+lock records until the next write operation. Django 1.6 introduced
+database-level autocommit; since then, execution in such a context
+voids the effect of ``select_for_update()``. It is, therefore, assumed
+now to be an error, and raises an exception.
+
+This change may cause test failures if you use ``select_for_update()``
+in a test class which is a subclass of
+:class:`~django.test.TransactionTestCase` rather than
+:class:`~django.test.TestCase`.
+
+This change was made because such errors can be caused by including an
+app which expects global transactions (e.g. :setting:`ATOMIC_REQUESTS
+<DATABASE-ATOMIC_REQUESTS>` set to True), or Django's old autocommit
@timgraham

timgraham Apr 9, 2014

Owner

`` around True

Member

shaib commented Apr 10, 2014

Pushed manually, and then applied and pushed Tim's last comments.

shaib closed this Apr 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment