#24880 closed Cleanup/optimization (fixed)
Clarify docs about select_for_update() error if run outside of a transaction
Reported by: | Przemysław Suliga | Owned by: | nobody |
---|---|---|---|
Component: | Documentation | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (8)
comment:1 by , 10 years ago
Summary: | Error on all db backends is `select_for_update` is run outside of a transaction → Error on all db backends if `select_for_update` is run outside of a transaction |
---|
comment:2 by , 10 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
comment:3 by , 10 years ago
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?
comment:4 by , 10 years ago
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.
comment:5 by , 10 years ago
Thanks for help!
I decided to go with a documentation fix for now: https://github.com/django/django/pull/4725
comment:6 by , 10 years ago
Component: | Uncategorized → Documentation |
---|---|
Needs documentation: | unset |
Patch needs improvement: | unset |
Summary: | Error on all db backends if `select_for_update` is run outside of a transaction → Clarify docs about select_for_update() error if run outside of a transaction |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
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).