Help prevent inconsistencies with select...for update row locks
#190
Conversation
|
I've tried out my own branch on a project where there's about 70~ models with order that needs updating on every drag, and have not noticed a performance detriment with the select for update statement. |
|
I have borrowed some of your insights for concurrency and preventing race conditions, especially the len(qs) on the get_ordering_queryset is a life saver. Thanks a lot! (I was down the road with a LOCK TABLE raw SQL statement to lock the whole table, but this made the whole application crash in production with concurrent requests) |
|
Glad it helped! |
|
@GeeWee |
|
@gabn88 try |
|
Just want to emphasize that it's been a little while since I wrote this. I was able to still create some race conditions with this setup, if I remember correctly, but it was much, much harder and much less frequent. |
|
I was busy with work. I will have time to look into this in couple of days. Could you rebase, please? |
|
I'm sorry I don't actually use django-ordered-model anymore, so I don't think I'll have time to look at this in the foreseeable future. Edits by maintainers are enabled though, so you should be able to rebase it if you have the time. |
a2c7483 to
5b7db8e
Compare
|
I've rebased and verified tests. I agree using It looks like Perhaps there is a chance we could see the simultaneous insert of a new row (from |
select ... for update row locks
select ... for update row locksselect...for update row locks
select...for update row locksselect...for update row locks
|
If we refactor the lock-taking to a sub or wrapper and use it across |
| current_order_value_in_db = ( | ||
| self.get_ordering_queryset() | ||
| .filter(pk=self.pk) | ||
| .values(self.order_field_name) | ||
| ) |
There was a problem hiding this comment.
this may still have other stale data in self that is not updated, possibly overwriting other changes. So either the final self.save() call should use update_fields, or you could call self.refresh_from_db() here instead of doing this query. The latter form updates all the data after acquiring the lock and should bring the instance up to date again.
Partially fixes #184
This takes the steps outlined in #184 to prevent deadlocks and data inconsistencies.
It locks rows before updating them, on databases that support this. Unfortunately since the testing suite uses SQLite I haven't been able to write a test for this, and I was not comfortable changing the testing DB.
I've also made a change so that
to()always fetches the newest value from the database, which is necessary because otherwise we can end up in situations where there's inconsistencies with concurrent connections. I've added a test for this that was failing before the changes, but is passing now.I haven't gone through and checked whether or not other methods than
toshould have the same optimization applied.