Opened 18 years ago

Closed 13 years ago

#2705 closed New feature (fixed)

[patch] Add optional FOR UPDATE clause to QuerySets

Reported by: Hawkeye Owned by: brunobraga
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: mir@…, rajeev.sebastian@…, vic@…, sebastian@…, justin.fiore@…, lidaobing@…, jdemoor@…, wallenfe@…, sebastian@…, jdi@…, liangent@…, xmm.dev@…, simon@…, physicsnick@…, meticulos.slacker@…, Forest Bond, Oldřich Jedlička, Alexander Koshelev, FTMO, Dan Fairs, miloslav.pojman@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

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

Attachments (29)

for_update.diff (1.5 KB ) - added by Hawkeye 18 years ago.
select_for_update.patch (8.3 KB ) - added by Collin Grady <cgrady@…> 17 years ago.
improvement on same idea, supports backend-specific syntax
select_for_update_r7188.patch (4.7 KB ) - added by KBS 16 years ago.
updated patch that works against r7188 of trunk
for_update_7477.patch (4.1 KB ) - added by sakyamuni 16 years ago.
patch against 7477
for_update_7509.patch (5.9 KB ) - added by sakyamuni 16 years ago.
Updated patch to include test / documentation, please review as I have not submitted tests/documentation before
for_update_7510.patch (6.2 KB ) - added by sakyamuni 16 years ago.
Corrected bug in test, modified documentation
for_update_7510.2.patch (6.2 KB ) - added by sakyamuni 16 years ago.
minor documentation changes
for_update_7513.patch (6.2 KB ) - added by kbs 16 years ago.
should work now with sqlite
for_update_7513.2.patch (7.9 KB ) - added by sakyamuni 16 years ago.
Added NOWAIT support (raises exception with mysql), included KBS'es fix to get sqlite to work
for_update_8031.2.patch (7.9 KB ) - added by Sebastian Bauer 16 years ago.
patch against r8031
for_update_1.0.patch (8.1 KB ) - added by jdemoor 16 years ago.
Updated patch for Django 1.0, several fixes, tested with Postgres and SQLite.
for_update_1.0.2.patch (8.1 KB ) - added by jdemoor 16 years ago.
Fixed MySQL bug.
for_update-1.0.1.diff (18.7 KB ) - added by jdemoor 15 years ago.
Patch against Django 1.0.1
for_update-1.0.1-v2.diff (18.7 KB ) - added by Sebastian Bauer 15 years ago.
with all
for_update-1.0.1-v3.diff (19.5 KB ) - added by Erin Kelly 15 years ago.
Added support for oracle backend
for_update_9962.diff (12.6 KB ) - added by Sebastian Bauer 15 years ago.
for update patch with transaction.commit_unless_managed() inside select_for_update()
for_update_9962.v2.diff (19.6 KB ) - added by Sebastian Bauer 15 years ago.
in last patch i forget include new files
for_update_9975.diff (19.6 KB ) - added by jdemoor 15 years ago.
Fixed bug in tests.
for_update_11366.diff (12.7 KB ) - added by Sebastian Bauer 15 years ago.
for update patch do 1.1
for_update_11366_cdestigter.diff (12.1 KB ) - added by Craig de Stigter 14 years ago.
remove unnecessary/broken changes to django/db/models/base.py
for_update_1.2.0-final.patch.diff (12.2 KB ) - added by FTMO 14 years ago.
based on the file for_update_11366_cdestigter.diff
for_update_14751.diff (12.3 KB ) - added by Dan Fairs 13 years ago.
Update of for_update_1.2.0-final.patch.diff to apply cleanly to trunk r14751
for_update_tests_14778.diff (20.0 KB ) - added by Dan Fairs 13 years ago.
Updated version of patch with tests - really this time! (passed on MySQL, PostgreSQL, sqlite)
for_update_r15007.diff (19.2 KB ) - added by Dan Fairs 13 years ago.
Updated patch, with incomplete deadlock handling removed.
2705-for_update-r15013.diff (18.6 KB ) - added by Ramiro Morales 13 years ago.
Tweaks to Dan Fairs' for_update_r15007.diff
2705-for_update-r15021.diff (18.7 KB ) - added by Dan Fairs 13 years ago.
Update to prevent spurious failure on MySQL MyISAM
2705-for_update-r15174.diff (21.2 KB ) - added by Dan Fairs 13 years ago.
Updated patch to resolve incorrect transaction handling in the tests, and modify NOWAIT behaviour on backends that do not support it.
2705-for_update-r16022.diff (21.7 KB ) - added by Dan Fairs 13 years ago.
Updated patch with better docs
2705-for_update-r16053.diff (22.1 KB ) - added by Dan Fairs 13 years ago.
Same as previous patch, but skip a test on Py2.6.

Download all attachments as: .zip

Change History (120)

comment:1 by mir@…, 18 years ago

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()

by Hawkeye, 18 years ago

Attachment: for_update.diff added

comment:2 by Hawkeye, 18 years ago

Summary: Add optional FOR UPDATE clause to 'get' database operations[patch] Add optional FOR UPDATE clause to 'get' database operations

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!

comment:3 by Hawkeye, 18 years ago

Summary: [patch] Add optional FOR UPDATE clause to 'get' database operations[patch] Add optional FOR UPDATE clause to QuerySets

by Collin Grady <cgrady@…>, 17 years ago

Attachment: select_for_update.patch added

improvement on same idea, supports backend-specific syntax

comment:4 by Collin Grady <cgrady@…>, 17 years ago

Cc: cgrady@… added

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)

comment:5 by Malcolm Tredinnick, 17 years ago

Triage Stage: UnreviewedDesign decision needed

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.

comment:6 by anonymous, 17 years ago

Cc: rajeev.sebastian@… added

comment:7 by Vic, 17 years ago

Cc: vic@… added

comment:8 by Jacob, 16 years ago

Triage Stage: Design decision neededSomeday/Maybe

comment:9 by Jacob, 16 years ago

Triage Stage: Someday/MaybeAccepted

comment:10 by KBS, 16 years ago

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.

by KBS, 16 years ago

updated patch that works against r7188 of trunk

comment:11 by anonymous, 16 years ago

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

comment:12 by anonymous, 16 years ago

Cc: sebastian@… added

comment:13 by anonymous, 16 years ago

Cc: justin.fiore@… added

in reply to:  11 comment:14 by James Bennett, 16 years ago

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.

by sakyamuni, 16 years ago

Attachment: for_update_7477.patch added

patch against 7477

comment:15 by sakyamuni, 16 years ago

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

Thanks.

comment:16 by anonymous, 16 years ago

Needs documentation: set
Needs tests: set
Version: SVN

by sakyamuni, 16 years ago

Attachment: for_update_7509.patch added

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

by sakyamuni, 16 years ago

Attachment: for_update_7510.patch added

Corrected bug in test, modified documentation

by sakyamuni, 16 years ago

Attachment: for_update_7510.2.patch added

minor documentation changes

by kbs, 16 years ago

Attachment: for_update_7513.patch added

should work now with sqlite

by sakyamuni, 16 years ago

Attachment: for_update_7513.2.patch added

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

comment:17 by sakyamuni <x@…>, 16 years ago

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

comment:18 by LI Daobing <lidaobing@…>, 16 years ago

Cc: lidaobing@… added

comment:19 by Collin Grady, 16 years ago

Needs documentation: unset

comment:20 by anonymous, 16 years ago

Cc: jdemoor@… added

by Sebastian Bauer, 16 years ago

Attachment: for_update_8031.2.patch added

patch against r8031

comment:21 by wallenfe, 16 years ago

Cc: wallenfe@… added

comment:22 by wallenfe, 16 years ago

Will this patch be included in the 1.0 release?

in reply to:  22 comment:23 by Russell Keith-Magee, 16 years ago

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.

comment:24 by Collin Grady, 16 years ago

Cc: cgrady@… removed

comment:25 by Sebastian Bauer, 16 years ago

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

comment:26 by mrts, 16 years ago

milestone: post-1.0

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

by jdemoor, 16 years ago

Attachment: for_update_1.0.patch added

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

comment:27 by spaetz, 16 years ago

Cc: sebastian@… added

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

by jdemoor, 16 years ago

Attachment: for_update_1.0.2.patch added

Fixed MySQL bug.

by jdemoor, 15 years ago

Attachment: for_update-1.0.1.diff added

Patch against Django 1.0.1

comment:28 by jdemoor, 15 years ago

Needs tests: unset

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.

comment:29 by Ramiro Morales, 15 years ago

Patch needs improvement: set

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.

by Sebastian Bauer, 15 years ago

Attachment: for_update-1.0.1-v2.diff added

with all

comment:30 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:31 by Sebastian Bauer, 15 years ago

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

comment:32 by Erin Kelly, 15 years ago

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.

by Erin Kelly, 15 years ago

Attachment: for_update-1.0.1-v3.diff added

Added support for oracle backend

comment:33 by Erin Kelly, 15 years ago

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.

comment:34 by anonymous, 15 years ago

Cc: jdi@… added

comment:35 by liangent, 15 years ago

Cc: liangent@… added

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()?

comment:36 by liangent, 15 years ago

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")

by Sebastian Bauer, 15 years ago

Attachment: for_update_9962.diff added

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

in reply to:  36 ; comment:37 by Sebastian Bauer, 15 years ago

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

in reply to:  33 ; comment:38 by Erin Kelly, 15 years ago

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.

by Sebastian Bauer, 15 years ago

Attachment: for_update_9962.v2.diff added

in last patch i forget include new files

in reply to:  38 comment:39 by Sebastian Bauer, 15 years ago

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)

comment:40 by Erin Kelly, 15 years ago

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.

in reply to:  37 ; comment:41 by liangent, 15 years ago

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.

in reply to:  40 comment:42 by liangent, 15 years ago

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.

in reply to:  41 ; comment:43 by Sebastian Bauer, 15 years ago

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/

in reply to:  43 comment:44 by liangent, 15 years ago

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.

comment:45 by Sebastian Bauer, 15 years ago

i see two solutions:

  1. in documentation put information about this problem and workaround with force_update=True
  1. 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?

comment:46 by liangent, 15 years ago

  1. will SELECT COUNT() FOR UPDATE lock more rows? i'm not sure
  1. or we can save whether an instance was selected by .select_for_update() in that instance?

by jdemoor, 15 years ago

Attachment: for_update_9975.diff added

Fixed bug in tests.

comment:47 by jdemoor, 15 years ago

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.

comment:48 by xmm, 15 years ago

Cc: xmm.dev@… added
Has patch: unset
Patch needs improvement: unset

comment:49 by xmm, 15 years ago

Has patch: set
Patch needs improvement: set

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

comment:50 by Simon Law, 15 years ago

Cc: simon@… added

by Sebastian Bauer, 15 years ago

Attachment: for_update_11366.diff added

for update patch do 1.1

comment:51 by physicsnick, 15 years ago

Cc: physicsnick@… added

comment:52 by meticulos_slacker, 15 years ago

Cc: meticulos.slacker@… added

comment:53 by physicsnick, 14 years ago

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.

comment:54 by anonymous, 14 years ago

Owner: changed from nobody to anonymous

by Craig de Stigter, 14 years ago

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

comment:55 by Craig de Stigter, 14 years ago

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

comment:56 by Forest Bond, 14 years ago

Cc: Forest Bond added

comment:57 by Forest Bond, 14 years ago

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.

comment:58 by Oldřich Jedlička, 14 years ago

Cc: Oldřich Jedlička added

comment:59 by Alexander Koshelev, 14 years ago

Cc: Alexander Koshelev added

by FTMO, 14 years ago

based on the file for_update_11366_cdestigter.diff

comment:60 by brunobraga, 13 years ago

Owner: changed from anonymous to brunobraga
Status: newassigned

comment:61 by brunobraga, 13 years ago

Cc: FTMO added

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,

comment:62 by brunobraga, 13 years ago

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,

in reply to:  62 comment:63 by German M. Bravo, 13 years ago

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

comment:64 by Dan Fairs, 13 years ago

Cc: Dan Fairs added

by Dan Fairs, 13 years ago

Attachment: for_update_14751.diff added

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

comment:65 by Dan Fairs, 13 years ago

Needs tests: set

comment:66 by Dan Fairs, 13 years ago

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

by Dan Fairs, 13 years ago

Attachment: for_update_tests_14778.diff added

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

comment:67 by Dan Fairs, 13 years ago

Needs documentation: set
Needs tests: unset

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.

by Dan Fairs, 13 years ago

Attachment: for_update_r15007.diff added

Updated patch, with incomplete deadlock handling removed.

comment:68 by Dan Fairs, 13 years ago

Needs documentation: unset
Patch needs improvement: unset

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.

by Ramiro Morales, 13 years ago

Attachment: 2705-for_update-r15013.diff added

Tweaks to Dan Fairs' for_update_r15007.diff

in reply to:  68 ; comment:69 by Ramiro Morales, 13 years ago

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.

by Dan Fairs, 13 years ago

Attachment: 2705-for_update-r15021.diff added

Update to prevent spurious failure on MySQL MyISAM

in reply to:  69 comment:70 by Dan Fairs, 13 years ago

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?

comment:71 by Jacob, 13 years ago

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.

comment:72 by Jacob, 13 years ago

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.

comment:73 by Dan Fairs, 13 years ago

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.

by Dan Fairs, 13 years ago

Attachment: 2705-for_update-r15174.diff added

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

comment:74 by Dan Fairs, 13 years ago

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.

comment:75 by Dan Fairs, 13 years ago

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

comment:76 by Dan Fairs, 13 years ago

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.

comment:77 by Łukasz Rekucki, 13 years ago

Severity: normalNormal
Type: enhancementNew feature

comment:78 by Miloslav Pojman, 13 years ago

Cc: miloslav.pojman@… added

in reply to:  76 ; comment:79 by Carl Meyer, 13 years ago

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.

comment:80 by Jacob, 13 years ago

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.

in reply to:  79 comment:81 by Dan Fairs, 13 years ago

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.

in reply to:  80 comment:82 by Dan Fairs, 13 years ago

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.

by Dan Fairs, 13 years ago

Attachment: 2705-for_update-r16022.diff added

Updated patch with better docs

in reply to:  79 comment:83 by Dan Fairs, 13 years ago

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.

comment:84 by Jacob, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:85 by Jacob, 13 years ago

Triage Stage: Ready for checkinAccepted

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?

comment:86 by Dan Fairs, 13 years ago

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?

comment:87 by Dan Fairs, 13 years ago

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?

comment:88 by Jacob, 13 years ago

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.

comment:89 by Dan Fairs, 13 years ago

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).

by Dan Fairs, 13 years ago

Attachment: 2705-for_update-r16053.diff added

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

comment:90 by Dan Fairs, 13 years ago

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

comment:91 by Jacob, 13 years ago

Resolution: fixed
Status: assignedclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top