[Django] #24880: Error on all db backends is `select_for_update` is run outside of a transaction
Django
transaction
-------------------------------+--------------------
Reporter: suligap | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24880>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
transaction
-------------------------------+--------------------------------------
Reporter: suligap | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by suligap):
* needs_docs: => 0
* needs_better_patch: => 0
* needs_tests: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24880#comment:1>
Django
transaction
-------------------------------+--------------------------------------
Reporter: suligap | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by shaib):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/24880#comment:2>
Django
transaction
-------------------------------+--------------------------------------
Reporter: suligap | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/24880#comment:3>
Django
transaction
-------------------------------+--------------------------------------
Reporter: suligap | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by shaib):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24880#comment:4>
Django
transaction
-------------------------------+--------------------------------------
Reporter: suligap | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by suligap):
Thanks for help!
I decided to go with a documentation fix for now:
https://github.com/django/django/pull/4725
--
Ticket URL: <https://code.djangoproject.com/ticket/24880#comment:5>
Django
transaction
--------------------------------------+------------------------------------
Reporter: suligap | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
* component: Uncategorized => Documentation
* needs_docs: 1 => 0
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24880#comment:6>
Django
transaction
--------------------------------------+------------------------------------
Reporter: suligap | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"d29ed3f355b0c57e7036807f1d54f33796d8d820" d29ed3f]:
{{{
#!CommitTicketReference repository=""
revision="d29ed3f355b0c57e7036807f1d54f33796d8d820"
Fixed #24880 -- Added more explicit docs on select_for_update() on SQLite.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24880#comment:7>
Django
transaction
--------------------------------------+------------------------------------
Reporter: suligap | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"52b5890f5237ad8859108de1e0757be2b3f354c4" 52b5890]:
{{{
#!CommitTicketReference repository=""
revision="52b5890f5237ad8859108de1e0757be2b3f354c4"
[1.8.x] Fixed #24880 -- Added more explicit docs on select_for_update() on
SQLite.
Backport of d29ed3f355b0c57e7036807f1d54f33796d8d820 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24880#comment:8>