[patch] Add optional FOR UPDATE clause to QuerySets

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

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!

improvement on same idea, supports backend-specific syntax

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)

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.

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.

updated patch that works against r7188 of trunk

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

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.

patch against 7477

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

Thanks.

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

Corrected bug in test, modified documentation

minor documentation changes

should work now with sqlite

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

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

patch against r8031

Will this patch be included in the 1.0 release?

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.

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

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

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

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

Fixed MySQL bug.

Patch against Django 1.0.1

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.

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.

with all

Milestone post-1.0 deleted

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

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.

Added support for oracle backend

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.

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

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

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

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

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.

in last patch i forget include new files

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)

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.

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.

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.

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/

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.

i see two solutions:

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

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

i dont now if second solution is ok

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

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

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

Fixed bug in tests.

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.

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

for update patch do 1.1

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.

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

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

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.