[Django] #27193: ORDER BY clause not included in subqueries using select_for_update()
Django
-------------------------------+--------------------
Reporter: sqwishy | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
This was tested using Django 1.9.5. I believe the code below fails to
generate the correct SQL. I explicitly set an ordering in the subquery but
it is not expressed in the generated SQL. This alters the behaviour of the
SELECT statement when the FOR UPDATE clause is used. My use case is to
lock rows in a particular order with the goal of preventing deadlocks.
Example code:
{{{
with transaction.atomic():
print(User.objects.filter(id__in=User.objects.order_by('id').select_for_update()))
}}}
PostgreSQL logs:
{{{
2016-09-08 02:59:43 ACWST [30398-20] testsystem@testsystem LOG:
statement: BEGIN
2016-09-08 02:59:43 ACWST [30398-21] testsystem@testsystem LOG:
statement: SELECT "testapp_user"."id", "testapp_user"."password", ... FROM
"testapp_user" WHERE "testapp_user"."id" IN (SELECT "testapp_user"."id"
FROM "testapp_user" FOR UPDATE) LIMIT 21
2016-09-08 02:59:43 ACWST [30398-22] testsystem@testsystem LOG:
statement: COMMIT
}}}
It looks like the order_by is being cleared on the queryset object here
https://github.com/django/django/blob/master/django/db/models/sql/compiler.py#L476
I can verify this as it looks like ORDER BY does show up when I include
distinct() in the subquery - which isn't a suitable workaround as DISTINCT
is not compatible with FOR UPDATE - and by slicing the subquery as to set
an offset or a limit.
For now, an acceptable workaround seems to make the SELECT FOR UPDATE in a
separate query instead of a subquery. That at least seems to accomplish
the goal I am trying to achieve.
--
Ticket URL: <https://code.djangoproject.com/ticket/27193>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: sqwishy | Owner: nobody
Type: Uncategorized | Status: closed
Component: Uncategorized | Version: 1.9
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by shaib):
* status: new => closed
* needs_better_patch: => 0
* resolution: => needsinfo
* needs_tests: => 0
* needs_docs: => 0
Comment:
Hi,
You said:
> My use case is to lock rows in a particular order with the goal of
preventing deadlocks.
Can you please point to some documentation, or example code with
execution, that shows this idea may work? As far as I know, the locking is
atomic -- it is done for all rows matching the criteria together, and thus
the order-by clause on the inner select is indeed meaningless and can be
dropped.
If and when you show this, please re-open the ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/27193#comment:1>
Django
Reporter: sqwishy | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.9
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sqwishy):
* status: closed => new
* resolution: needsinfo =>
Comment:
The documentation at the following link may suggest that rows are locked
while a query is running rather than all at once atomically; see
particularly the second paragraph under "13.2.1. Read Committed Isolation
Level" https://www.postgresql.org/docs/current/static/transaction-iso.html
#XACT-READ-COMMITTED
This is a bit of code where adding the order by seems to prevent
deadlocks.
{{{#!python
from django.db import transaction
from django.contrib.auth.models import Group
from django.db.models import F
def setup():
for i in range(20):
Group.objects.get_or_create(name='test_27193_%i' % i)
def test():
for _ in range(20):
with transaction.atomic():
Group.objects.filter(
id__in=Group.objects.filter(name__startswith='test_27193').select_for_update()
).update(name=F('name'))
#Group.objects.filter(
#
id__in=Group.objects.filter(name__startswith='test_27193').select_for_update().order_by('id')[1:]
#).update(name=F('name'))
setup()
test()
}}}
I have that saved to a file aptly named `script.py` and run it 20 times at
once (I'm sure there is a less ugly way to do this):
{{{
seq 20 | parallel --ungroup -j 20 "echo {}; ./manage.py shell --plain <
script.py"
}}}
Doing this with the script as shown, exceptions are raised in python about
deadlocks and PostgreSQL logs about it. If you comment out that first
query and uncomment the second one everything runs fine. The slice there
is just to convince django not to reset the order-by value in the
queryset.
--
Ticket URL: <https://code.djangoproject.com/ticket/27193#comment:2>
Django
Reporter: sqwishy | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.9
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by shaib):
Well, I don't quite see it in the doc you linked, but it's mentioned in
[https://www.postgresql.org/message-
id/freemail.20070...@fm10.freemail.hu this thread] and your
code example is convincing. I stand corrected.
--
Ticket URL: <https://code.djangoproject.com/ticket/27193#comment:3>
Django
Reporter: sqwishy | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by shaib):
* cc: shaib (added)
* component: Uncategorized => Database layer (models, ORM)
* type: Uncategorized => New feature
* stage: Unreviewed => Accepted
Comment:
That said, 1.9 and even 1.10 are past the phase where this can be put in
them. Accepting tentatively, assuming it can be reproduced in master (I
don't have time to check right now)
--
Ticket URL: <https://code.djangoproject.com/ticket/27193#comment:4>
Django
Reporter: sqwishy | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/7319 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27193#comment:5>
Django
Reporter: sqwishy | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"8ac115c730353dfc6bdbd03e0671a978dcc99e15" 8ac115c7]:
{{{
#!CommitTicketReference repository=""
revision="8ac115c730353dfc6bdbd03e0671a978dcc99e15"
Fixed #27193 -- Preserved ordering in select_for_update subqueries.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27193#comment:6>