Clarify docs about select_for_update() error if run outside of a transaction

[Przemysław Suliga]

A TransactionManagementError is now raised when select_for_update is executed in autocommit mode - but only on backends that support SELECT ... FOR UPDATE. Perhaps it should be raised also on backends that do not support SELECT ... FOR UPDATE?

Reading the documentation:

Evaluating a queryset with select_for_update() in autocommit mode is a TransactionManagementError error because the rows are not locked in that case. If allowed, this would facilitate data corruption and could easily be caused by calling code that expects to be run in a transaction outside of one.

Using select_for_update() on backends which do not support SELECT ... FOR UPDATE (such as SQLite) will have no effect.

It's not 100% clear if the error will be raised on backends that do not support SELECT ... FOR UPDATE. I was confused by this and assumed that it will error on sqlite - it does not.

Here's a rough patch that will make select_for_update error on all backends in that case: ​https://github.com/django/django/compare/master...suligap:select_for_update-transaction-error-on-all-backends

Alternatively, if this does not make sense due to reasons I don't understand ATM, maybe we could make the docs be more explicit about this aspect.

[Shai Berger]

This makes perfect sense for the common use-case, "use sqlite as scaffolding for development, use a more capable database backend for production". On the other hand, this breaks backwards-compatibility on Sqlite.

The idea of changing Sqlite's behavior to match other backends was discussed when we changed select-for-update to error out (when supported) if executed out of transaction; see ​https://groups.google.com/forum/#!msg/django-developers/8woZjCPcFmA/GqQLDxFZbc8J for the details. Back then, nobody seems to have thought it was worth breaking backwards compatibility; if you want to convince people otherwise, I think it would be best to do so on the DevelopersMailingList.

Even if you can convince people that a break is warranted, we will probably want the change to go through a deprecation cycle; thus, the patch as is would need to be fixed. Otherwise, a patch to improve the documentation is welcome.

(I am not marking this "wontfix" yet, mostly because of the possibility of resolving via documentation).

[Tim Graham]

I'm having trouble seeing backwards compatibility as a major concern. This would affect SQLite users who are using select_for_update() (where it has no effect) outside of a transaction. Am I missing a case you are thinking about?

[Shai Berger]

The case I have in mind, mostly, is a project on Sqlite using generic code, where the generic code calls select_for_update(). Such code, whether it uses transactions or not, has been essentially bogus but allowed to run anyway, since the time select_for_update() was introduced. I expect that this things happen in the wild, because I suspect that people who actually use Sqlite are more likely to be using the default autocommit mode.

My point of view is, essentially: If we're going for correctness on Sqlite, select_for_update() there should be an unconditional error. If we aren't, then the main value of the suggestion here is, to catch on Sqlite something which is only a problem elsewhere (as on Sqlite, select_for_update() is no safer in a transaction than out of it). Is this gain worth breaking code that works (for a lenient enough definition of "works"), and compromising the simple model that says "on Sqlite select_for_update() does nothing"?

As I hinted above, the original decision to keep select_for_update() passing silently when run out of transactions on Sqlite was made on the django-developers mailing list; I think if we're going to change it, that's the place to do that as well.

[Przemysław Suliga]

Thanks for help!

I decided to go with a documentation fix for now: ​https://github.com/django/django/pull/4725

[Tim Graham <timograham@…>]

In d29ed3f:

Fixed #24880 -- Added more explicit docs on select_for_update() on SQLite.

[Tim Graham <timograham@…>]

In 52b5890:

[1.8.x] Fixed #24880 -- Added more explicit docs on select_for_update() on SQLite.

Backport of d29ed3f355b0c57e7036807f1d54f33796d8d820 from master