Code

Opened 8 years ago

Closed 3 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: master
Severity: Normal Keywords:
Cc: mir@…, rajeev.sebastian@…, vic@…, sebastian@…, justin.fiore@…, lidaobing@…, jdemoor@…, wallenfe@…, sebastian@…, jdi@…, liangent@…, xmm.dev@…, simon@…, physicsnick@…, meticulos.slacker@…, forest, oldium, alexkoshelev, ftmo, danfairs, 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 8 years ago.
select_for_update.patch (8.3 KB) - added by Collin Grady <cgrady@…> 7 years ago.
improvement on same idea, supports backend-specific syntax
select_for_update_r7188.patch (4.7 KB) - added by KBS 6 years ago.
updated patch that works against r7188 of trunk
for_update_7477.patch (4.1 KB) - added by sakyamuni 6 years ago.
patch against 7477
for_update_7509.patch (5.9 KB) - added by sakyamuni 6 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 6 years ago.
Corrected bug in test, modified documentation
for_update_7510.2.patch (6.2 KB) - added by sakyamuni 6 years ago.
minor documentation changes
for_update_7513.patch (6.2 KB) - added by kbs 6 years ago.
should work now with sqlite
for_update_7513.2.patch (7.9 KB) - added by sakyamuni 6 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 anih 6 years ago.
patch against r8031
for_update_1.0.patch (8.1 KB) - added by jdemoor 6 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 6 years ago.
Fixed MySQL bug.
for_update-1.0.1.diff (18.7 KB) - added by jdemoor 6 years ago.
Patch against Django 1.0.1
for_update-1.0.1-v2.diff (18.7 KB) - added by anih 6 years ago.
with all
for_update-1.0.1-v3.diff (19.5 KB) - added by ikelly 5 years ago.
Added support for oracle backend
for_update_9962.diff (12.6 KB) - added by anih 5 years ago.
for update patch with transaction.commit_unless_managed() inside select_for_update()
for_update_9962.v2.diff (19.6 KB) - added by anih 5 years ago.
in last patch i forget include new files
for_update_9975.diff (19.6 KB) - added by jdemoor 5 years ago.
Fixed bug in tests.
for_update_11366.diff (12.7 KB) - added by anih 5 years ago.
for update patch do 1.1
for_update_11366_cdestigter.diff (12.1 KB) - added by cdestigter 5 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 4 years ago.
based on the file for_update_11366_cdestigter.diff
for_update_14751.diff (12.3 KB) - added by danfairs 4 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 danfairs 4 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 danfairs 4 years ago.
Updated patch, with incomplete deadlock handling removed.
2705-for_update-r15013.diff (18.6 KB) - added by ramiro 4 years ago.
Tweaks to Dan Fairs' for_update_r15007.diff
2705-for_update-r15021.diff (18.7 KB) - added by danfairs 4 years ago.
Update to prevent spurious failure on MySQL MyISAM
2705-for_update-r15174.diff (21.2 KB) - added by danfairs 4 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 danfairs 3 years ago.
Updated patch with better docs
2705-for_update-r16053.diff (22.1 KB) - added by danfairs 3 years ago.
Same as previous patch, but skip a test on Py2.6.

Download all attachments as: .zip

Change History (120)

comment:1 Changed 8 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 8 years ago by Hawkeye

comment:2 Changed 8 years ago by Hawkeye

  • Summary changed from Add optional FOR UPDATE clause to 'get' database operations to [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 8 years ago by Hawkeye

  • Summary changed from [patch] Add optional FOR UPDATE clause to 'get' database operations to [patch] Add optional FOR UPDATE clause to QuerySets

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

improvement on same idea, supports backend-specific syntax

comment:4 Changed 7 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 7 years ago by mtredinnick

  • Triage Stage changed from Unreviewed to Design 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 7 years ago by anonymous

  • Cc rajeev.sebastian@… added

comment:7 Changed 7 years ago by Vic

  • Cc vic@… added

comment:8 Changed 7 years ago by jacob

  • Triage Stage changed from Design decision needed to Someday/Maybe

comment:9 Changed 7 years ago by jacob

  • Triage Stage changed from Someday/Maybe to Accepted

comment:10 Changed 6 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 6 years ago by KBS

updated patch that works against r7188 of trunk

comment:11 follow-up: Changed 6 years ago by anonymous

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

comment:12 Changed 6 years ago by anonymous

  • Cc sebastian@… added

comment:13 Changed 6 years ago by anonymous

  • Cc justin.fiore@… added

comment:14 in reply to: ↑ 11 Changed 6 years ago by ubernostrum

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 6 years ago by sakyamuni

patch against 7477

comment:15 Changed 6 years ago by sakyamuni

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

Thanks.

comment:16 Changed 6 years ago by anonymous

  • Needs documentation set
  • Needs tests set
  • Version set to SVN

Changed 6 years ago by sakyamuni

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

Changed 6 years ago by sakyamuni

Corrected bug in test, modified documentation

Changed 6 years ago by sakyamuni

minor documentation changes

Changed 6 years ago by kbs

should work now with sqlite

Changed 6 years ago by sakyamuni

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

comment:17 Changed 6 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 6 years ago by LI Daobing <lidaobing@…>

  • Cc lidaobing@… added

comment:19 Changed 6 years ago by cgrady

  • Needs documentation unset

comment:20 Changed 6 years ago by anonymous

  • Cc jdemoor@… added

Changed 6 years ago by anih

patch against r8031

comment:21 Changed 6 years ago by wallenfe

  • Cc wallenfe@… added

comment:22 follow-up: Changed 6 years ago by wallenfe

Will this patch be included in the 1.0 release?

comment:23 in reply to: ↑ 22 Changed 6 years ago by russellm

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 6 years ago by cgrady

  • Cc cgrady@… removed

comment:25 Changed 6 years ago by anih

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

comment:26 Changed 6 years ago by mrts

  • milestone set to post-1.0

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

Changed 6 years ago by jdemoor

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

comment:27 Changed 6 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 6 years ago by jdemoor

Fixed MySQL bug.

Changed 6 years ago by jdemoor

Patch against Django 1.0.1

comment:28 Changed 6 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 6 years ago by ramiro

  • 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 6 years ago by anih

with all

comment:30 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:31 Changed 5 years ago by anih

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

comment:32 Changed 5 years ago by ikelly

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 5 years ago by ikelly

Added support for oracle backend

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

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

  • Cc jdi@… added

comment:35 Changed 5 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 follow-up: Changed 5 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 5 years ago by anih

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

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

comment:38 in reply to: ↑ 33 ; follow-up: Changed 5 years ago by ikelly

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 5 years ago by anih

in last patch i forget include new files

comment:39 in reply to: ↑ 38 Changed 5 years ago by anih

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 follow-up: Changed 5 years ago by 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.

comment:41 in reply to: ↑ 37 ; follow-up: Changed 5 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 5 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 ; follow-up: Changed 5 years ago by 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/

comment:44 in reply to: ↑ 43 Changed 5 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 5 years ago by anih

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 5 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 5 years ago by jdemoor

Fixed bug in tests.

comment:47 Changed 5 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 5 years ago by xmm

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

comment:49 Changed 5 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 5 years ago by sfllaw

  • Cc simon@… added

Changed 5 years ago by anih

for update patch do 1.1

comment:51 Changed 5 years ago by physicsnick

  • Cc physicsnick@… added

comment:52 Changed 5 years ago by meticulos_slacker

  • Cc meticulos.slacker@… added

comment:53 Changed 5 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 5 years ago by anonymous

  • Owner changed from nobody to anonymous

Changed 5 years ago by cdestigter

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

comment:55 Changed 5 years ago by cdestigter

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

comment:56 Changed 4 years ago by forest

  • Cc forest added

comment:57 Changed 4 years ago by forest

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 4 years ago by oldium

  • Cc oldium added

comment:59 Changed 4 years ago by alexkoshelev

  • Cc alexkoshelev added

Changed 4 years ago by ftmo

based on the file for_update_11366_cdestigter.diff

comment:60 Changed 4 years ago by brunobraga

  • Owner changed from anonymous to brunobraga
  • Status changed from new to assigned

comment:61 Changed 4 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 follow-up: Changed 4 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 4 years ago by Kronuz

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 4 years ago by danfairs

  • Cc danfairs added

Changed 4 years ago by danfairs

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

comment:65 Changed 4 years ago by danfairs

  • Needs tests set

comment:66 Changed 4 years ago by danfairs

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

Changed 4 years ago by danfairs

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

comment:67 Changed 4 years ago by danfairs

  • 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 4 years ago by danfairs

Updated patch, with incomplete deadlock handling removed.

comment:68 follow-up: Changed 4 years ago by danfairs

  • 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 4 years ago by ramiro

Tweaks to Dan Fairs' for_update_r15007.diff

comment:69 in reply to: ↑ 68 ; follow-up: Changed 4 years ago by ramiro

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 4 years ago by danfairs

Update to prevent spurious failure on MySQL MyISAM

comment:70 in reply to: ↑ 69 Changed 4 years ago by danfairs

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 4 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 4 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 4 years ago by danfairs

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 4 years ago by danfairs

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

comment:74 Changed 4 years ago by danfairs

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 3 years ago by danfairs

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

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

comment:77 Changed 3 years ago by lrekucki

  • Severity changed from normal to Normal
  • Type changed from enhancement to New feature

comment:78 Changed 3 years ago by mila

  • Cc miloslav.pojman@… added

comment:79 in reply to: ↑ 76 ; follow-ups: Changed 3 years ago by carljm

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 follow-up: Changed 3 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 3 years ago by danfairs

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 3 years ago by danfairs

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 3 years ago by danfairs

Updated patch with better docs

comment:83 in reply to: ↑ 79 Changed 3 years ago by danfairs

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 3 years ago by jacob

  • Triage Stage changed from Accepted to Ready for checkin

comment:85 Changed 3 years ago by jacob

  • Triage Stage changed from Ready for checkin to Accepted

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 3 years ago by danfairs

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 3 years ago by danfairs

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 3 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 3 years ago by danfairs

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 3 years ago by danfairs

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

comment:90 Changed 3 years ago by danfairs

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 3 years ago by jacob

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.