Help prevent inconsistencies with select...for update row locks

[GeeWee]

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 to should have the same optimization applied.

[GeeWee]

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.

[gabn88]

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)

[GeeWee]

Glad it helped!

[gabn88]

@GeeWee
In our production server the len(qs) takes approx 1 second, because that is the time it takes to do a count (4,5 milion rows, indexed). Is there a way to set the select_for_update without a count that you know of?

[imomaliev]

@gabn88 try qs.exists()

[GeeWee]

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.

[imomaliev]

I was busy with work. I will have time to look into this in couple of days. Could you rebase, please?

[GeeWee]

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.

[shuckc]

I've rebased and verified tests. I agree using select_for_update to take row-level locks over the currently selected order_with_respect_to partition looks like a plausible way to go to avoid race conditions without the cost of escalating to a table lock. It would be reassuring if we could test that SELECT .... FOR UPDATE SQL actually gets emitted.

It looks like current_order_value_in_db is refreshed with those locks held, so probably this is also a good idea and worth the extra query in the uncontended case.

Perhaps there is a chance we could see the simultaneous insert of a new row (from create/save operation) that sneaks into the same partition above our range, but would not be locked, and would then miss the increase resulting in an overlap. I think we probably should audit how some of these queries could run concurrently before merging. However the patch is at least current and available for anyone who wants it.

[shuckc]

If we refactor the lock-taking to a sub or wrapper and use it across save, delete, swap, to, above and below methods I think I'd be happy merging it.

[sergei-maertens]

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.