[patch] Add optional FOR UPDATE clause to QuerySets

[Hawkeye]

SQL supports the "FOR UPDATE" clause for SELECT statements to allow for pre-emptive locking. The addition of for_update() for use with get() database operations would greatly enhance Django's transaction support.

As articulated by Michael Radziej on django-users:

   something = models.Something.get(id=333).for_update() 
   # --> django issues: SELECT * FROM something FOR UPDATE 
   something.count +=1 
   something.save() 
   # --> django issues: UPDATE SOMETHING SET ... 

Suggesting EITHER:

• for_update method to extend all 'get'-like statements
• get_for_update to supplement other 'get'-like statements

[mir@…]

My original proposal was grossly defect (it just meant to illustrate the idea).

What I really meant was adding for_update() as a method to QuerySet,
returning a QuerySet (like filter, distinct, etc.)

So, the example should of course be the other way around:

something = models.Something.objects.for_update().get(id=333)

or, easier to understand,

something = models.Something.objects.filter(id=333).for_update().get()

[Hawkeye]

I'm attaching a patch that provides the functionality:

something = models.Something.objects.filter(id=333).for_update().get()

Obviously, it could appear at different locations in the chain.

Comments welcome!

[Collin Grady <cgrady@…>]

improvement on same idea, supports backend-specific syntax

[Collin Grady <cgrady@…>]

I had this same idea last night, and wrote up a patch similar to this, but with support for db-specific syntax - for example MS SQL uses 'WITH (UPDLOCK, HOLDLOCK)' from what I could see, whereas sqlite doesn't support it at all, so it's ignored there to prevent errors (though that could be changed if it's the wrong way to go about it)

[Malcolm Tredinnick]

This isn't a bad feature, although I'm not sure I like the API without being able to put my finger on the exact reason -- nevermind, though, that can be sorted out later and it may be that I just haven't become used to it yet.

Wanted to drop in a note to say "yes, it's worth implementing", but "stop posting patches", because this will go in after the QuerySet refactorisation.

Moving to "design decision needed" so that we remember to think about the API at the right moment.

[KBS]

Here is an updated patch which I got working with r7188 of trunk.

It solved this problem I was having with concurrently incrementing a model field:
​http://dpaste.com/37397/

Now I simply do to lock row and retreive data in mysql.
vote = Votes.object.select_for_update().get_or_create(name='xx')[0]

Note: you could get deadlock if you do not save and commit transaction, and on mysql, this only works on innodb tables, and maybe clusters.

[KBS]

updated patch that works against r7188 of trunk

[anonymous]

bump, i realy need this feature, could someone say what is this feature status?

[James Bennett]

Replying to anonymous:

bump, i realy need this feature, could someone say what is this feature status?

Please don't do this; posting a comment for no other reason than to try to "bump" a ticket is rude, and your question was already answered by Malcolm's comment about what needs to happen with this patch.

[sakyamuni]

patch against 7477

[sakyamuni]

I just updated KBS'es patch to work against the trunk.

Thanks.

[sakyamuni]

Updated patch to include test / documentation, please review as I have not submitted tests/documentation before

[sakyamuni]

Corrected bug in test, modified documentation

[sakyamuni]

minor documentation changes

[kbs]

should work now with sqlite

[sakyamuni]

Added NOWAIT support (raises exception with mysql), included KBS'es fix to get sqlite to work

[sakyamuni <x@…>]

Also note that with for_update_7513.2.patch, I improved the tests such that it tries to commit after locking with for_update()

[Sebastian Bauer]

patch against r8031

[wallenfe]

Will this patch be included in the 1.0 release?

[Russell Keith-Magee]

Replying to wallenfe:

Will this patch be included in the 1.0 release?

No. The canonical list of v1.0 features is the ​Version One features wiki page. This ticket is not mentioned there, so it won't be included.

[Sebastian Bauer]

hmm its small patch without any backwards-incompatible changes and its working, so maybe worth to be commited?

[mrts]

As of now, targeting to post 1.0. Could go into 1.0-maybe as well.

[jdemoor]

Updated patch for Django 1.0, several fixes, tested with Postgres and SQLite.

[spaetz]

tested on mysql. django 1.0 plus patch for_update_1.0.patch posted in comment above:

Exception Value: type object 'BaseDatabaseWrapper' has no attribute 'for_update_sql'
Exception Location: django-1.0_for_update/django/db/backends/mysql/base.py in for_update_sql, line 146

[jdemoor]

Fixed MySQL bug.

[jdemoor]

Patch against Django 1.0.1

[jdemoor]

I attached a new patch with these changes:

• backend-agnostic exceptions for deadlocks and locking failures
• a decorator to retry deadlocked views
• select_for_update() does nothing when the backend doesn't support it; the same goes for the nowait argument with MySQL (there used to be a NotImplementedError)
• updated documentation
• comprehensive tests

The patch is against Django 1.0.1 and is tested on Python 2.5.

[Ramiro Morales]

Latest patch (for_update-1.0.1.diff) has this in django/db/models/sql/query.py:30

_all__ = ['Query', 'BaseQuery', 'LockNotAvailable']

a leading "_" is missing.

[Sebastian Bauer]

with all

[(none)]

Milestone post-1.0 deleted

[Sebastian Bauer]

Could anyone specify what is need to add this patch to trunk?

[Ian Kelly]

For starters, somebody needs to update the patch so that it applies cleanly to trunk. A patch against 1.0.1 is not useful, since this feature will never be included in any 1.0.X release.

We also need to add support for the Oracle backend...actually, I think I'll work on that now.

[Ian Kelly]

Added support for oracle backend

[Ian Kelly]

I made a small change to handle_deadlocks so that the transaction is automatically rolled back when a deadlock is detected and no retries are left. The reason for this is that in Oracle, the second thread in the test was still waiting for the first thread to rollback even after the first thread had been joined and presumably had its resources cleaned up. The alternative would be to leave it as is and recommend the user to always rollback whenever a DeadlockError is raised, but I'm not thrilled with that: one might expect to be able to ignore the DeadlockError and have the deadlock be fully resolved.

I'll also be checking on the cx_Oracle mailing list whether this behavior is intended.

On a related note, I'm not sure that having handle_deadlocks retry the view should be the default behavior -- anything done in middleware and not already committed would be rolled back as well and not retried.

[liangent]

it seems there's a problem...
if i used .select_for_update(), but didn't do UPDATE
django doesn't know it needs to commit when automatic commit is required, for example,
.managed(False) or something in middleware/transaction.py (when a view returns)

maybe, it should be marked as dirty when invoking .select_for_update()?

[liangent]

another problem...

                transaction.managed(True)
                transaction.set_dirty() # see comment above
                s = S.objects.select_for_update().filter(s='Q').order_by('pk')
                try:
                    t = s[0]
                except IndexError:
                    transaction.managed(False)
                    time.sleep(2000)
                    continue # there's a 'for' outside
                t.s = 'A'
                t.g = G.objects.get(pk=1)
                t.save(force_update=True) # mark
                transaction.managed(False)

in that marked line, if i don't specify force_update=True, it will do an INSERT, which will cause an IntegrityError. IntegrityError: (1062, "Duplicate entry '11' for key 1")

[Sebastian Bauer]

for update patch with transaction.commit_unless_managed() inside select_for_update()

[Sebastian Bauer]

Replying to liangent:

in that marked line, if i don't specify force_update=True, it will do an INSERT, which will cause an IntegrityError. IntegrityError: (1062, "Duplicate entry '11' for key 1")

i try to reproduce this problem but without success, could you paste model file and separate some test?

i send patch to trunk version with transaction.commit_unless_managed() inside select_for_update() (django/db/models/query.py)

i dont have oracle to make some tests, but im using this patch with postgresql for about 1/2 year

[Ian Kelly]

Replying to ikelly:

I made a small change to handle_deadlocks so that the transaction is automatically rolled back when a deadlock is detected and no retries are left. The reason for this is that in Oracle, the second thread in the test was still waiting for the first thread to rollback even after the first thread had been joined and presumably had its resources cleaned up. The alternative would be to leave it as is and recommend the user to always rollback whenever a DeadlockError is raised, but I'm not thrilled with that: one might expect to be able to ignore the DeadlockError and have the deadlock be fully resolved.

I'll also be checking on the cx_Oracle mailing list whether this behavior is intended.

Update: I'm having a heck of a time nailing down exactly what's going on with this hang issue. It reliably occurs when using the oracle backend, but outside of Django I can't reproduce it at all; the connection does get properly cleaned up when the deadlocked thread exits. About the only thing I've managed to determine is that if I run del connection.connection when exiting the test threads, then the issue does not occur and the tests pass. What I can't figure out is why this is necessary.

[Sebastian Bauer]

in last patch i forget include new files

[Sebastian Bauer]

Replying to ikelly:

Update: I'm having a heck of a time nailing down exactly what's going on with this hang issue. It reliably occurs when using the oracle backend, but outside of Django I can't reproduce it at all; the connection does get properly cleaned up when the deadlocked thread exits. About the only thing I've managed to determine is that if I run del connection.connection when exiting the test threads, then the issue does not occur and the tests pass. What I can't figure out is why this is necessary.

maybe problem is related with that select_for_update didnt set transaction dirty? new patch have this corected(i think)

[Ian Kelly]

I've found the problem -- turns out it has nothing to do with Oracle. It's ​Python bug 1868 combined with setting DEBUG=True in the settings file; I had that set in my oracle test settings file, but not in my postgres or mysql files. With DEBUG=True set, the cursor gets wrapped in a CursorDebugWrapper, which has a finally clause in its execute method that accesses an attribute of the ConnectionWrapper object, triggering the bug.

I don't think this is likely to be a major issue outside of the test suite, so I wouldn't object to moving the rollback in handle_deadlock back to its original location, as long as we do something in the test to make sure it doesn't hang.

Also, I think select_for_update is the wrong place to set the transaction dirty. That needs to happen when the query is run, not when the queryset is constructed, which is not necessarily the same time.

[liangent]

Replying to anih:

Replying to liangent:

in that marked line, if i don't specify force_update=True, it will do an INSERT, which will cause an IntegrityError. IntegrityError: (1062, "Duplicate entry '11' for key 1")

i try to reproduce this problem but without success, could you paste model file and separate some test?

i send patch to trunk version with transaction.commit_unless_managed() inside select_for_update() (django/db/models/query.py)

i dont have oracle to make some tests, but im using this patch with postgresql for about 1/2 year

(note: 1. 'key 1' is the primary key; 2. i'm using mysql: 5.0.67-0ubuntu6)

i think it has nothing to do with the models.

as i know, django try to SELECT a row when we're going to .save() to know if the row exists. however, because i used SELECT FOR UPDATE, mysql locked the row, and django cannot find it with SELECT, so django decides to INSERT. since the field 'id' in object 't' has been set and value is assigned to an existing row ('t' was got from a SELECT FOR UPDATE), an IntegrityError is raised.

[liangent]

Replying to ikelly:

I've found the problem -- turns out it has nothing to do with Oracle. It's ​Python bug 1868 combined with setting DEBUG=True in the settings file; I had that set in my oracle test settings file, but not in my postgres or mysql files. With DEBUG=True set, the cursor gets wrapped in a CursorDebugWrapper, which has a finally clause in its execute method that accesses an attribute of the ConnectionWrapper object, triggering the bug.

I don't think this is likely to be a major issue outside of the test suite, so I wouldn't object to moving the rollback in handle_deadlock back to its original location, as long as we do something in the test to make sure it doesn't hang.

Also, I think select_for_update is the wrong place to set the transaction dirty. That needs to happen when the query is run, not when the queryset is constructed, which is not necessarily the same time.

anyhow, transaction should be set dirty after SELECT FOR UPDATE was executed.

[Sebastian Bauer]

Replying to liangent:

Replying to anih:
(note: 1. 'key 1' is the primary key; 2. i'm using mysql: 5.0.67-0ubuntu6)

i think it has nothing to do with the models.

as i know, django try to SELECT a row when we're going to .save() to know if the row exists. however, because i used SELECT FOR UPDATE, mysql locked the row, and django cannot find it with SELECT, so django decides to INSERT. since the field 'id' in object 't' has been set and value is assigned to an existing row ('t' was got from a SELECT FOR UPDATE), an IntegrityError is raised.

problem is how mysql treats rows select with for update statment:
​http://www.mysqlperformanceblog.com/2006/08/06/select-lock-in-share-mode-and-for-update/

[liangent]

Replying to anih:

Replying to liangent:

Replying to anih:
(note: 1. 'key 1' is the primary key; 2. i'm using mysql: 5.0.67-0ubuntu6)

i think it has nothing to do with the models.

as i know, django try to SELECT a row when we're going to .save() to know if the row exists. however, because i used SELECT FOR UPDATE, mysql locked the row, and django cannot find it with SELECT, so django decides to INSERT. since the field 'id' in object 't' has been set and value is assigned to an existing row ('t' was got from a SELECT FOR UPDATE), an IntegrityError is raised.

problem is how mysql treats rows select with for update statment:
​http://www.mysqlperformanceblog.com/2006/08/06/select-lock-in-share-mode-and-for-update/

i think django should deal with it and have an identical invoking interface between different DBs.

[Sebastian Bauer]

i see two solutions:

1. in documentation put information about this problem and workaround with force_update=True

2. we can propagate information about using select_for_update to models object and when we check if row exist add for update to this "select count()"

i dont now if second solution is ok

maybe we need set stage "design decision needed" for this ticket?

[liangent]

1. will SELECT COUNT() FOR UPDATE lock more rows? i'm not sure

2. or we can save whether an instance was selected by .select_for_update() in that instance?

[jdemoor]

Fixed bug in tests.

[jdemoor]

The tests were failing because of

view1 = handle_deadlocks(max_retries=max_retries)(transaction.commit_manually(view1))

which is now

view1 = transaction.commit_manually(handle_deadlocks(max_retries=max_retries)(view1))

This will have to go in the documentation.

[xmm]

Sorry, some options switched off. Maybe it is important.
PS: Has added myself in the CC-field since I consider important this patch.

[Sebastian Bauer]

for update patch do 1.1

[physicsnick]

Hey guys, just a warning here: the latest patch, for_update_11366, adds a print statement to db/models/base.py which causes mod_wsgi to panic because access to stdout is restricted. Seems to work well otherwise.

[Craig de Stigter]

remove unnecessary/broken changes to django/db/models/base.py

[Craig de Stigter]

new patch solves a bunch of test failures and gets rid of the offending print statement.

[Forest Bond]

I believe that transaction.set_dirty should be called when acquiring the row lock. The transaction must be rolled back or committed for the lock to be released. TransactionMiddleware won't do this unless the transaction is marked as dirty.

[FTMO]

based on the file for_update_11366_cdestigter.diff

[brunobraga]

We have worked with [ftmo] to update previous patches according to newest releases, but I see it is pointless to keep doing that every time... Instead, we should push this to the main stream, but I don't know what still needs to be done for it to be properly accepted.

I would like to flag this for review by the committers (forward to "ready for checkin"), and if there is anything missing, let us know, so we can work on it and finalize this.

For reference, we are using the latest 1.2.0 patch in production environment for almost 6 months now, without any problems.

Thanks in advance,

[brunobraga]

Additionally, the newest django 1.2.3 (which has been put to the official Ubuntu repository 10.10) requires modification in the patch in order to work properly. As soon as we receive a positive message from committers, we can work on updating it to add to the main stream code.

Thank you,

[German M. Bravo]

Replying to brunobraga:

Additionally, the newest django 1.2.3 (which has been put to the official Ubuntu repository 10.10) requires modification in the patch in order to work properly. As soon as we receive a positive message from committers, we can work on updating it to add to the main stream code.

Thank you,

brunobraga, please, what modifications does it need? because the patch applies fine on django tagged 1.2.3

[Dan Fairs]

Update of for_update_1.2.0-final.patch.diff to apply cleanly to trunk r14751

[Dan Fairs]

Ugh - ignore that last patch, please - I somehow missed the tests!

[Dan Fairs]

Updated version of patch with tests - really this time! (passed on MySQL, PostgreSQL, sqlite)

[Dan Fairs]

OK - I've updated the patch. This now has tests, fixes a missing import on the MySQL backend and an AttributeError in the psycopg2_postgresql backend. The tests pass for me on PostgreSQL, MySQL (MyISAM) and SQLite. I'm about to try to get Oracle up and running to give it a spin there (wish me luck).

Any feedback on the test implementation is welcomed - I rarely do programming with threads, so any advice is welcome. In particular, guidance on handling the tests on systems without threading support would be welcomed. Also, I have yet to test any deadlocking behaviour alluded to in previous comments; and feedback there on the patch itself is good too. Note that I've not altered the core code of the patch, aside from fixing broken imports and

I think the following showstopper issues remain:

• LockNotAvailable is only raised by RawQuery, not by anything else. This is counter to what the documentation says. I couldn't see how to fix this cleanly. One option would be to catch the DatabaseError propagating up in SQLCompiler.execute_sql() method. This feels wrong to me - there's existing DatabaseError processing at the cursor level, so it feels right that this should be there (though it is effectively done at this level in the RawQuery class). Alternatively, this could be implemented in each backend's CursorWrapper. The problem with this is that we don't have access to the connection's DatabaseFeatures and DatabaseOperations instances to to do the processing required.

• The documentation in the patch refers to django.views.decorators.deadlock.handle_deadlock. This decorator does not exist. Is this something that needs writing, something that's fallen out of the patch at some point, or just some old documentation which is no longer relevant with the current implementation?

As Malcolm mentioned earlier in the history, the API doesn't quite feel right. This is subjective, but I'd probably change:

• nowait as the name of the parameter. It doesn't mean much to someone not familiar with the underlying SQL. I'd prefer to see it renamed to blocking (and of course the meaning of True/False to be flipped, with the default being True).
• select_for_update is a little cumbersome. However, the best I could come up with is for_update (as used in earlier versions of the patch) or, based on the existing select_related name, perhaps select_updatable. If a core dev has feelings on this, I'm more than happy to modify the patch accordingly.

I'd love to get this patch finalised and into Django, so I'm happy to put the time in to get this done.

[Dan Fairs]

Updated patch, with incomplete deadlock handling removed.

[Dan Fairs]

I've updated the patch. Key changes:

• I've removed all the attempts to detect deadlock, retry transactions, and associated documentation. I've done this for two reasons:
  • It keeps this patch much more focussed: it now *only* introduces select_for_update, and does nothing else. Django apps may suffer from deadlock without this patch anyway - there doesn't seem much point in trying to shoehorn a general solution into this patch in particular.
  • Reading the comments in django.db.utils, the intent is for database-related exceptions to mirror what's in PEP249. The closest exception in PEP249 is simply DatabaseError, which is raised already.
• Executing a select_for_update now sets the transaction to be dirty. This is done at the point the query is run, not at the point that select_for_update is actually called.
• Tests that require threading support should now be skipped on platforms that do not support threading.
• Minor doc cleanups

Tests pass on SQLite, PostgreSQL and MySQL. I'd appreciate it if someone with access to Oracle could give them a shot, as my attempts to get Oracle up and running failed.

[Ramiro Morales]

Tweaks to Dan Fairs' for_update_r15007.diff

[Ramiro Morales]

Replying to danfairs:

I've updated the patch [...] Tests pass on SQLite, PostgreSQL and MySQL. I'd appreciate it if someone with access to Oracle could give them a shot

Dan, I've tested for_update_r15007.diff under Oracle and. It runs in my environment after the following modifications:

• Use threading.Thread.isAlive() instead of is_alive() for Python < 2.6 compatibility
• Added tests/modeltests/select_for_update/__init__.py

Attached 2705-for_update-r15013.diff contains them plus:

• Use django.utils.functional.wraps() instead of functools.wraps()
• Tweaked docs and docstrings (typos and use quoting to get non-proportional fonts when rendered)
• Sorted AUTHORS alphabetically.

... as my attempts to get Oracle up and running failed.

If you have the hadware and the time maybe wiki:OracleTestSetup could be of some help.

[Dan Fairs]

Update to prevent spurious failure on MySQL MyISAM

[Dan Fairs]

Replying to ramiro:

Replying to danfairs:
Dan, I've tested for_update_r15007.diff under Oracle and. It runs in my environment after the following modifications:

• Use threading.Thread.isAlive() instead of is_alive() for Python < 2.6 compatibility
• Added tests/modeltests/select_for_update/__init__.py

Attached 2705-for_update-r15013.diff contains them plus:

• Use django.utils.functional.wraps() instead of functools.wraps()
• Tweaked docs and docstrings (typos and use quoting to get non-proportional fonts when rendered)
• Sorted AUTHORS alphabetically.

... as my attempts to get Oracle up and running failed.

If you have the hadware and the time maybe wiki:OracleTestSetup could be of some help.

Many thanks for that test and those tweaks. Following that wiki page, I now have a working Oracle install to run tests against.

I've updated the patch slightly:

• Add a skip dependent on supports_transactions to the block test, which spuriously failed on MyISAM
• In the same test, roll back the transaction before fetching data to check the update done by the separate thread worked - in higher transaction isolation levels, it's not possible to read this data until the current transaction is rolled back or committed.

The tests now all run cleanly on PostgreSQL, MySQL (both InnoDB and MyISAM), Oracle and SQLite.

What needs to happen next?

[Jacob]

Tests are failing for me - psycopg2, PostgreSQL 9:

Creating test database for alias 'default'...
Creating test database for alias 'other'...
Destroying old test database 'other'...
...Exception in thread Thread-2:
Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/threading.py", line 522, in __bootstrap_inner
    self.run()
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/threading.py", line 477, in run
    self.__target(*self.__args, **self.__kwargs)
  File "/Users/jacob/Projects/Django/upstream/tests/modeltests/select_for_update/tests.py", line 117, in run_select_for_update
    people[0].name = 'Fred'
IndexError: list index out of range

F...
======================================================================
FAIL: test_nowait_raises_error_on_block (modeltests.select_for_update.tests.SelectForUpdateTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jacob/Projects/Django/upstream/django/test/testcases.py", line 613, in skip_wrapper
    return test_func(*args, **kwargs)
  File "/Users/jacob/Projects/Django/upstream/tests/modeltests/select_for_update/tests.py", line 110, in test_nowait_raises_error_on_block
    self.check_exc(status[-1])
  File "/Users/jacob/Projects/Django/upstream/tests/modeltests/select_for_update/tests.py", line 71, in check_exc
    self.failUnless(isinstance(exc, DatabaseError))
AssertionError: False is not True

----------------------------------------------------------------------
Ran 7 tests in 3.796s

FAILED (failures=1)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

I'm trying to fix, but I can't seem to work out how this test is supposed to work or what it's doing. SelectForUpdateTests.setUp() creates a Person, but SelectForUpdateTests.run_select_for_update() calls connection._rollback() right off the bat, so where's that Person in the list on line 116 supposed to come from?

I'd really like to check this in, but feature freeze is rapidly approaching. The rest of the patch looks good, but if I can't work out what the tests are doing, I can't be comfortable checking this in.

[Jacob]

One other review comment: the silent discarding of nowait=True if the DB doesn't support it isn't OK. If you pass nowait=True and the database can't support that we should raise an ImproperlyConfigured (or something) right away. Silently blocking is very bad.

[Dan Fairs]

Odd - the tests pass for me on PostgreSQL 8.4 and PostgreSQL 9.0. I'll look into the failure in the next day or so (appreciate that means we'll miss the cut for 1.3).

With regard to nowait - yes, that makes sense. I'll modify the patch accordingly.

[Dan Fairs]

Updated patch to resolve incorrect transaction handling in the tests, and modify NOWAIT behaviour on backends that do not support it.

[Dan Fairs]

Apologies for the delay - holidays, sickness and the resultant backlog are responsible for that.

While I was unable to reproduce the test failures Jacob saw directly, I did see that there was an error with the transaction setup. This caused an unintended commit, which explains why the Person created in setUp() persisted unexpectedly. Once I had fixed this problem, I then started seeing the traceback that Jacob saw.

The newest version of the patch contains these changes, and also changes the NOWAIT behaviour on backends that do not support it by raising a DatabaseError when a query with NOWAIT is run, as Jacob commented.

The docs have also been updated to reflect the new behaviour.

I've tested this with MySQL, PostgreSQL and SQLite, and the tests pass - I'll test on Oracle in the next couple of days.

[Dan Fairs]

Finally had the chance to check the tests on Oracle - they pass.

[Dan Fairs]

OK - now 1.3 is out of the door, is there any chance this could be re-reviewed? The existing patch still applies to trunk (as of r15941) and the select_for_update tests pass on all backends.

[Carl Meyer]

Replying to danfairs:

OK - now 1.3 is out of the door, is there any chance this could be re-reviewed? The existing patch still applies to trunk (as of r15941) and the select_for_update tests pass on all backends.

Have you run the full test suite with this patch applied, at least on the commonly-available DB backends where the suite takes reasonable time (SQLite, Postgres)?

There's a "select_related" where it should be "select_for_update" in the documentation section of this patch.

[Jacob]

One nit: docs/ref/models/querysets.txt claims:

Passing nowait=True to select_for_update using database backends that
do not support nowait, such as MySQL, will cause a DatabaseError to be
raised. This is in order to prevent code unexpectedly blocking.

But docs/ref/databases.txt claims:

MySQL does not support the NOWAIT option to the SELECT ... FOR UPDATE
statement. However, you may call the select_for_update() method of a
queryset with nowait=True. In that case, the argument will be silently
discarded and the generated query will block until the requested lock can be
acquired.

These don't agree, and looking at the code it appears the first claim is correct. So something needs to be updated there.

Otherwise looks RFC to me.

[Dan Fairs]

Replying to carljm:

Have you run the full test suite with this patch applied, at least on the commonly-available DB backends where the suite takes reasonable time (SQLite, Postgres)?

Yes - well, I had done towards the end of last week! I'll update to current Django trunk, rerun before and after on all backends, and make sure that there are no more failures with the patch than without.

There's a "select_related" where it should be "select_for_update" in the documentation section of this patch.

Good spot - I'll fix that.

[Dan Fairs]

Replying to jacob:

One nit: docs/ref/models/querysets.txt claims:

...

But docs/ref/databases.txt claims:

MySQL does not support the NOWAIT option to the SELECT ... FOR UPDATE
statement. However, you may call the select_for_update() method of a
queryset with nowait=True. In that case, the argument will be silently
discarded and the generated query will block until the requested lock can be
acquired.

...

These don't agree, and looking at the code it appears the first claim is correct. So something needs to be updated there.

Ah, good spot. We changed the behaviour of using select_for_update on backends which don't support NOWAIT from a silent failure to raising a DatabaseError a while back. Looks like I failed to update the docs. I'll amend the patch (together with Carl's comments).

Otherwise looks RFC to me.

Great.

[Dan Fairs]

Updated patch with better docs

[Dan Fairs]

Replying to carljm:

I've updated the patch to 2705-for_update-r16022.diff.

Have you run the full test suite with this patch applied, at least on the commonly-available DB backends where the suite takes reasonable time (SQLite, Postgres)?

I've now run the full suite on PostgreSQL, MySQL and SQLite with the latest version of the patch. I get a single failure before the patch is applied, which remains after the patch with PostgreSQL and SQLite - so no more failures. Similar situation with MySQL, only there are two failures (one in common with the PostgreSQL and SQLite backends). For reference, the common failure is:

======================================================================
FAIL: test_view_decorator (regressiontests.cache.tests.CacheMiddlewareTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dan/virtual/django/src/django/tests/regressiontests/cache/tests.py", line 1443, in test_view_decorator
    self.assertEqual(response.content, 'Hello World 18')
AssertionError: 'Hello World 10' != 'Hello World 18'

MySQL additionally has this failure:

======================================================================
FAIL: testI18NWithLocalePaths (regressiontests.views.tests.i18n.JsI18NTestsMultiPackage)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dan/virtual/django/src/django/tests/regressiontests/views/tests/i18n.py", line 160, in testI18NWithLocalePaths
    self.assertContains(response, javascript_quote('este texto de app3 debe ser traducido'))
  File "/home/dan/virtual/django/src/django/django/test/testcases.py", line 425, in assertContains
    msg_prefix + "Couldn't find '%s' in response" % text)
AssertionError: Couldn't find 'este texto de app3 debe ser traducido' in response

The Oracle tests seem very broken both before and after this patch is applied (though the tests specific to the feature on Oracle pass fine, and I had a clean run back against trunk r15174).

I don't think the failures are related to this patch.

There's a "select_related" where it should be "select_for_update" in the documentation section of this patch.

Fixed.

[Jacob]

Actually, I'm getting test failures on the latest patch:

Creating test database for alias 'default'...
Creating test database for alias 'other'...
Destroying old test database 'other'...
...Exception in thread Thread-2:
Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/threading.py", line 522, in __bootstrap_inner
    self.run()
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/threading.py", line 477, in run
    self.__target(*self.__args, **self.__kwargs)
  File "/Users/jacob/Projects/Django/upstream/tests/modeltests/select_for_update/tests.py", line 155, in run_select_for_update
    people[0].name = 'Fred'
IndexError: list index out of range

F...s
======================================================================
FAIL: test_nowait_raises_error_on_block (modeltests.select_for_update.tests.SelectForUpdateTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jacob/Projects/Django/upstream/django/test/testcases.py", line 603, in skip_wrapper
    return test_func(*args, **kwargs)
  File "/Users/jacob/Projects/Django/upstream/tests/modeltests/select_for_update/tests.py", line 122, in test_nowait_raises_error_on_block
    self.check_exc(status[-1])
  File "/Users/jacob/Projects/Django/upstream/tests/modeltests/select_for_update/tests.py", line 83, in check_exc
    self.failUnless(isinstance(exc, DatabaseError))
AssertionError: False is not True

----------------------------------------------------------------------
Ran 8 tests in 4.173s

FAILED (failures=1, skipped=1)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

Patch applied against trunk (r16044), PostgreSQL 9.0.1. Is this PEBKAC, or for real?

[Dan Fairs]

I think actually the P may exist on the other side of the K.

From the traceback, it looks like you're using the system Python on OS X - is that right? I ran into an issue using this particular version of Python, the original thread discussing it is here:

​http://groups.google.com/group/django-developers/browse_thread/thread/cf2b76a9b69f794c/ac09404089bfd185

... which led to this Python bug:

​http://bugs.python.org/issue1242657

I'd totally forgotten about that. In a nutshell, we're seeing a Python regression in the version you're using (and, unfortunately, I suspect a lot of people will be using). The options from here as I see it are:

• Put a note in the documentation alerting people to this problem with Python 2.6.1
• Modify the QuerySet internals to work around the Python issue (some options were discussed in the thread on django-developers)

The former should be done as part of this patch; the latter would probably be a separate ticket (and perhaps wouldn't hold up this patch). Regardless, I'll test this patch later on against an OS X Python, to see if I can reproduce it. (I'd been working on the patch using Python 2.7.2 on Debian).

Assuming that this is what the problem is, where do you think we should go from here?

[Dan Fairs]

I've confirmed that the tests pass fine with PostgreSQL on OS X using Python 2.7.1, and fails with the traceback you experienced using the system Python, 2.6.1.

What do you think the next step should be?

[Jacob]

Oh man, not this bug again! Damn, it just keeps coming up. OK, I suggest that for the purposes of *this* feature we just skip this test on the affected Python versions.

Quite separately, though, we probably need to put a note somewhere (the install docs?) about this issue so that folks on OSX's stock Python aren't confused. I'll open another ticket to track that.

[Dan Fairs]

Sounds good - I'll update the patch to skip that test on Python 2.6.x (similar to ConditionalTests.test_infinite_loop in django/tests/regressiontests/queries/tests.py).

[Dan Fairs]

Same as previous patch, but skip a test on Py2.6.

[Dan Fairs]

OK, 2705-for_update-r16053.diff skips the test in the same way that test_infinite_loop does for Python 2.6.x.

[Jacob]

In [16058]:

Fixed #2705: added a select_for_update() clause to querysets.

A number of people worked on this patch over the years -- Hawkeye, Colin Grady,
KBS, sakyamuni, anih, jdemoor, and Issak Kelly. Thanks to them all, and
apologies if I missed anyone.

Special thanks to Dan Fairs for picking it up again at the end and seeing this
through to commit.