Re: [Django] #28010: select_for_update() is too eager in presence of joins

[Django]

#28010: select_for_update() is too eager in presence of joins
-+-
 Reporter:  Ran Benita   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 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 Adam Wróbel):

 * cc: adam@… (added)


--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.a4ae94b6c443e9e56f3cd560062d0e1b%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28010: select_for_update() is too eager in presence of joins

[Django]

#28010: select_for_update() is too eager in presence of joins
-+-
 Reporter:  Ran Benita   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 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 Ran Benita):

 * has_patch:  0 => 1


Comment:

 I submitted a patch: https://github.com/django/django/pull/8330

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.c31dac273486c9b035a706590f0ae54e%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28010: select_for_update() is too eager in presence of joins

[Django]

#28010: select_for_update() is too eager in presence of joins
-+-
 Reporter:  Ran Benita   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Ran Benita):

 Thanks Simon. I will try to implement approach 2. I will start with just
 supporting `self` (not implicit), and see if I can get it to work; that
 would already be useful.

 Also, I need to correct my comment on Oracle a bit: while Oracle requires
 to specify a column, it does not "lock a column", it locks the entire row
 of the table containing the column. So it's equivalent to Postgres, only
 we should write `SELECT ... FOR UPDATE OF "orders_order.id"` in Oracle
 instead of `SELECT ... FOR UPDATE "orders_order"` in Postgres. Here's the
 doc:
 
http://docs.oracle.com/cd/E11882_01/server.112/e41084/statements_10002.htm#SQLRF55372

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.e8589d9afe57b736a41bd5a16344e018%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28010: select_for_update() is too eager in presence of joins

[Django]

#28010: select_for_update() is too eager in presence of joins
-+-
 Reporter:  Ran Benita   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Simon Charette):

 * version:  1.10 => master
 * stage:  Unreviewed => Accepted


Comment:

 Allowing the usage of `FOR UPDATE OF` is definitely something we want to
 add.

 Now for the proposed options the one that makes the most sense to me is
 `1.`. `2.` would be backward incompatible given we actually lock all rows
 by default and some backends don't allow locking only a specific table
 which would make `select_for_update()` behave quite a bit differently on
 different backends.

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.fc96048128f3a90d87c35e2b6f309fc5%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

[Django] #28010: select_for_update() is too eager in presence of joins

[Django]

#28010: select_for_update() is too eager in presence of joins
-+-
   Reporter:  Ran|  Owner:  nobody
  Benita |
   Type:  New| Status:  new
  feature|
  Component:  Database   |Version:  1.10
  layer (models, ORM)|
   Severity:  Normal |   Keywords:
   Triage Stage: |  Has patch:  0
  Unreviewed |
Needs documentation:  0  |Needs tests:  0
Patch needs improvement:  0  |  Easy pickings:  0
  UI/UX:  0  |
-+-
 Using `select_for_update()` on a queryset appends ` FOR UPDATE` to the
 query. If the query joins other tables, the effect is that all tables are
 locked. I believe this is not what most people using `select_for_update()`
 want, instead they only want to lock the row from the main table.

 == Example

 Consider a scenario like this:

 {{{#!python
 class Product(models.Model):
 name = models.CharField(max_length=100)
 price = models.PositiveIntegerField()


 class Order(models.Model):
 product = models.ForeignKey(Product, on_delete=models.PROTECT)
 status = models.CharField(max_length=100, choices=(
 ('NEW', 'New'),
 ('APPROVED', 'Approved'),
 ('CANCELED', 'Canceled'),
 )

 ...

 @classmethod
 def approve(cls, id):
 with transaction.atomic():
 order =
 Order.objects.select_related('product').select_for_update().get(pk=id)
 # ...Check order.product.price & stuff...
 order.status = 'APPROVED'
 order.save()

 @classmethod
 def cancel(cls, id):
 with transaction.atomic():
 # Similar...
 }}}

 Here, locking is needed in order to serialize `approve()` and `cancel()`
 on the same Order, to avoid race conditions.

 I have also used `select_related` in `approve()`, in order to avoid an
 extra query. Potentially, there are many related objects, but to keep the
 example simple I added just one.

 The interaction between `select_related` and `select_for_update` has the
 unintended consequence (IMO) of also locking the related `Product` row.
 Hence, if there is some concurrent task which updates product prices for
 example, it can cause conflicts, slowdowns, deadlocks, and other bad
 things.

 The fix is to remove the `select_related`. But then extra queries are
 needed for each related object access without a reasonable workaround (I
 think?).

 == Possible solutions

 PostgreSQL (which I am using) has an option to specify exactly which
 tables are to be locked, instead of locking all tables in the query:
 https://www.postgresql.org/docs/9.6/static/sql-select.html#SQL-FOR-UPDATE-
 SHARE. The syntax is `SELECT ... FOR UPDATE OF orders_order`.

 Oracle also supports this with similar syntax, however it allows
 specifying which *columns* to lock:
 http://docs.oracle.com/javadb/10.8.3.0/ref/rrefsqlj31783.html. I guess
 there it is possible to lock specific columns of specific rows, while
 postgres only locks full rows.

 I am not familiar with MySQL but it doesn't seem like it supports refining
 the `FOR UPDATE`.

 Here are some possible solutions I can think of:

 1. Add support for `select_for_update` to specify which models to lock,
 e.g. `select_for_update('self', 'product')`. This will use the `OF`
 syntax.

 2. Only support the `select_for_update('self')` behavior, and make it
 implicit, i.e. `select_for_update` only ever locks the main model of the
 QuerySet.

 3. Add a way to do `select_related` on an already-existing object,
 something like a standalone `select_related_objects` after the existing
 `prefetch_related_objects` function:
 https://docs.djangoproject.com/en/1.10/ref/models/querysets/#prefetch-
 related-objects, and keep `select_for_update` as-is. Then it should at
 least be possible to do one extra query instead of one per related object.

 Thanks for reading this far :)

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/051.62779c5a3f59a901e217afc382507673%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.