Opened 17 years ago

Closed 12 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 17 years ago.
select_for_update.patch (8.3 KB) - added by Collin Grady <cgrady@…> 16 years ago.
improvement on same idea, supports backend-specific syntax
select_for_update_r7188.patch (4.7 KB) - added by KBS 15 years ago.
updated patch that works against r7188 of trunk
for_update_7477.patch (4.1 KB) - added by sakyamuni 15 years ago.
patch against 7477
for_update_7509.patch (5.9 KB) - added by sakyamuni 15 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 15 years ago.
Corrected bug in test, modified documentation
for_update_7510.2.patch (6.2 KB) - added by sakyamuni 15 years ago.
minor documentation changes
for_update_7513.patch (6.2 KB) - added by kbs 15 years ago.
should work now with sqlite
for_update_7513.2.patch (7.9 KB) - added by sakyamuni 15 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 15 years ago.
patch against r8031
for_update_1.0.patch (8.1 KB) - added by jdemoor 15 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 15 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 Ian Kelly 14 years ago.
Added support for oracle backend
for_update_9962.diff (12.6 KB) - added by Sebastian Bauer 14 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 14 years ago.
in last patch i forget include new files
for_update_9975.diff (19.6 KB) - added by jdemoor 14 years ago.
Fixed bug in tests.
for_update_11366.diff (12.7 KB) - added by Sebastian Bauer 14 years ago.
for update patch do 1.1
for_update_11366_cdestigter.diff (12.1 KB) - added by Craig de Stigter 13 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 13 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 12 years ago.
Updated patch, with incomplete deadlock handling removed.
2705-for_update-r15013.diff (18.6 KB) - added by Ramiro Morales 12 years ago.
Tweaks to Dan Fairs' for_update_r15007.diff
2705-for_update-r15021.diff (18.7 KB) - added by Dan Fairs 12 years ago.
Update to prevent spurious failure on MySQL MyISAM
2705-for_update-r15174.diff (21.2 KB) - added by Dan Fairs 12 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 12 years ago.
Updated patch with better docs
2705-for_update-r16053.diff (22.1 KB) - added by Dan Fairs 12 years ago.
Same as previous patch, but skip a test on Py2.6.

Download all attachments as: .zip

Change History (120)

comment:1 Changed 17 years ago by 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()

Changed 17 years ago by Hawkeye

Attachment: for_update.diff added

comment:2 Changed 17 years ago by Hawkeye

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 Changed 17 years ago by Hawkeye

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

Changed 16 years ago by Collin Grady <cgrady@…>

Attachment: select_for_update.patch added

improvement on same idea, supports backend-specific syntax

comment:4 Changed 16 years ago by Collin Grady <cgrady@…>

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 Changed 16 years ago by Malcolm Tredinnick

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 Changed 16 years ago by anonymous

Cc: rajeev.sebastian@… added

comment:7 Changed 16 years ago by Vic

Cc: vic@… added

comment:8 Changed 16 years ago by Jacob

Triage Stage: Design decision neededSomeday/Maybe

comment:9 Changed 16 years ago by Jacob

Triage Stage: Someday/MaybeAccepted

comment:10 Changed 15 years ago by 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.

Changed 15 years ago by KBS

updated patch that works against r7188 of trunk

comment:11 Changed 15 years ago by anonymous

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

comment:12 Changed 15 years ago by anonymous

Cc: sebastian@… added

comment:13 Changed 15 years ago by anonymous

Cc: justin.fiore@… added

comment:14 in reply to:  11 Changed 15 years ago by 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.

Changed 15 years ago by sakyamuni

Attachment: for_update_7477.patch added

patch against 7477

comment:15 Changed 15 years ago by sakyamuni

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

Thanks.

comment:16 Changed 15 years ago by anonymous

Needs documentation: set
Needs tests: set
Version: SVN

Changed 15 years ago by sakyamuni

Attachment: for_update_7509.patch added

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

Changed 15 years ago by sakyamuni

Attachment: for_update_7510.patch added

Corrected bug in test, modified documentation

Changed 15 years ago by sakyamuni

Attachment: for_update_7510.2.patch added

minor documentation changes

Changed 15 years ago by kbs

Attachment: for_update_7513.patch added

should work now with sqlite

Changed 15 years ago by sakyamuni

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 Changed 15 years ago by 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()

comment:18 Changed 15 years ago by LI Daobing <lidaobing@…>

Cc: lidaobing@… added

comment:19 Changed 15 years ago by Collin Grady

Needs documentation: unset

comment:20 Changed 15 years ago by anonymous

Cc: jdemoor@… added

Changed 15 years ago by Sebastian Bauer

Attachment: for_update_8031.2.patch added

patch against r8031

comment:21 Changed 15 years ago by wallenfe

Cc: wallenfe@… added

comment:22 Changed 15 years ago by wallenfe

Will this patch be included in the 1.0 release?

comment:23 in reply to:  22 Changed 15 years ago by 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.

comment:24 Changed 15 years ago by Collin Grady

Cc: cgrady@… removed

comment:25 Changed 15 years ago by Sebastian Bauer

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

comment:26 Changed 15 years ago by mrts

milestone: post-1.0

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

Changed 15 years ago by jdemoor

Attachment: for_update_1.0.patch added

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

comment:27 Changed 15 years ago by spaetz

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

Changed 15 years ago by jdemoor

Attachment: for_update_1.0.2.patch added

Fixed MySQL bug.

Changed 15 years ago by jdemoor

Attachment: for_update-1.0.1.diff added

Patch against Django 1.0.1

comment:28 Changed 15 years ago by jdemoor

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 Changed 15 years ago by Ramiro Morales

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.

Changed 15 years ago by Sebastian Bauer

Attachment: for_update-1.0.1-v2.diff added

with all

comment:30 Changed 14 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:31 Changed 14 years ago by Sebastian Bauer

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

comment:32 Changed 14 years ago by 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.

Changed 14 years ago by Ian Kelly

Attachment: for_update-1.0.1-v3.diff added

Added support for oracle backend

comment:33 Changed 14 years ago by 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.

comment:34 Changed 14 years ago by anonymous

Cc: jdi@… added

comment:35 Changed 14 years ago by liangent

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 Changed 14 years ago by 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")

Changed 14 years ago by Sebastian Bauer

Attachment: for_update_9962.diff added

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

comment:37 in reply to:  36 ; Changed 14 years ago by 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

comment:38 in reply to:  33 ; Changed 14 years ago by 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.

Changed 14 years ago by Sebastian Bauer

Attachment: for_update_9962.v2.diff added

in last patch i forget include new files

comment:39 in reply to:  38 Changed 14 years ago by 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)

comment:40 Changed 14 years ago by 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.

comment:41 in reply to:  37 ; Changed 14 years ago by 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.

comment:42 in reply to:  40 Changed 14 years ago by 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.

comment:43 in reply to:  41 ; Changed 14 years ago by 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/

comment:44 in reply to:  43 Changed 14 years ago by 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.

comment:45 Changed 14 years ago by Sebastian Bauer

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 Changed 14 years ago by liangent

  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?

Changed 14 years ago by jdemoor

Attachment: for_update_9975.diff added

Fixed bug in tests.

comment:47 Changed 14 years ago by 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.

comment:48 Changed 14 years ago by xmm

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

comment:49 Changed 14 years ago by xmm

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 Changed 14 years ago by Simon Law

Cc: simon@… added

Changed 14 years ago by Sebastian Bauer

Attachment: for_update_11366.diff added

for update patch do 1.1

comment:51 Changed 14 years ago by physicsnick

Cc: physicsnick@… added

comment:52 Changed 14 years ago by meticulos_slacker

Cc: meticulos.slacker@… added

comment:53 Changed 14 years ago by 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.

comment:54 Changed 14 years ago by anonymous

Owner: changed from nobody to anonymous

Changed 13 years ago by Craig de Stigter

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

comment:55 Changed 13 years ago by Craig de Stigter

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

comment:56 Changed 13 years ago by Forest Bond

Cc: Forest Bond added

comment:57 Changed 13 years ago by 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.

comment:58 Changed 13 years ago by Oldřich Jedlička

Cc: Oldřich Jedlička added

comment:59 Changed 13 years ago by Alexander Koshelev

Cc: Alexander Koshelev added

Changed 13 years ago by FTMO

based on the file for_update_11366_cdestigter.diff

comment:60 Changed 13 years ago by brunobraga

Owner: changed from anonymous to brunobraga
Status: newassigned

comment:61 Changed 13 years ago by brunobraga

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 Changed 13 years ago by 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,

comment:63 in reply to:  62 Changed 13 years ago by 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

comment:64 Changed 13 years ago by Dan Fairs

Cc: Dan Fairs added

Changed 13 years ago by Dan Fairs

Attachment: for_update_14751.diff added

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

comment:65 Changed 13 years ago by Dan Fairs

Needs tests: set

comment:66 Changed 13 years ago by Dan Fairs

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

Changed 13 years ago by Dan Fairs

Attachment: for_update_tests_14778.diff added

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

comment:67 Changed 13 years ago by Dan Fairs

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.

Changed 12 years ago by Dan Fairs

Attachment: for_update_r15007.diff added

Updated patch, with incomplete deadlock handling removed.

comment:68 Changed 12 years ago by Dan Fairs

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.

Changed 12 years ago by Ramiro Morales

Attachment: 2705-for_update-r15013.diff added

Tweaks to Dan Fairs' for_update_r15007.diff

comment:69 in reply to:  68 ; Changed 12 years ago by 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.

Changed 12 years ago by Dan Fairs

Attachment: 2705-for_update-r15021.diff added

Update to prevent spurious failure on MySQL MyISAM

comment:70 in reply to:  69 Changed 12 years ago by 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?

comment:71 Changed 12 years ago by 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.

comment:72 Changed 12 years ago by 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.

comment:73 Changed 12 years ago by 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.

Changed 12 years ago by Dan Fairs

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 Changed 12 years ago by 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.

comment:75 Changed 12 years ago by Dan Fairs

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

comment:76 Changed 12 years ago by 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.

comment:77 Changed 12 years ago by Łukasz Rekucki

Severity: normalNormal
Type: enhancementNew feature

comment:78 Changed 12 years ago by Miloslav Pojman

Cc: miloslav.pojman@… added

comment:79 in reply to:  76 ; Changed 12 years ago by 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.

comment:80 Changed 12 years ago by 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.

comment:81 in reply to:  79 Changed 12 years ago by 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.

comment:82 in reply to:  80 Changed 12 years ago by 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.

Changed 12 years ago by Dan Fairs

Attachment: 2705-for_update-r16022.diff added

Updated patch with better docs

comment:83 in reply to:  79 Changed 12 years ago by 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.

comment:84 Changed 12 years ago by Jacob

Triage Stage: AcceptedReady for checkin

comment:85 Changed 12 years ago by Jacob

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 Changed 12 years ago by 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?

comment:87 Changed 12 years ago by 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?

comment:88 Changed 12 years ago by 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.

comment:89 Changed 12 years ago by 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).

Changed 12 years ago by Dan Fairs

Attachment: 2705-for_update-r16053.diff added

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

comment:90 Changed 12 years ago by 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.

comment:91 Changed 12 years ago by Jacob

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