[Django] #28099: Global use of UpdateQuery breaks complex delete updates

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[Django] #28099: Global use of UpdateQuery breaks complex delete updates

Django
#28099: Global use of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
               Reporter:  milosu     |          Owner:  nobody
                   Type:  Bug        |         Status:  new
              Component:  Database   |        Version:  1.11
  layer (models, ORM)                |
               Severity:  Normal     |       Keywords:
           Triage Stage:             |      Has patch:  1
  Unreviewed                         |
    Needs documentation:  0          |    Needs tests:  0
Patch needs improvement:  0          |  Easy pickings:  0
                  UI/UX:  0          |
-------------------------------------+-------------------------------------
 I've discovered some obscure bug, which triggers randomly and only under
 certain circumstances, based on the address space layout.

 Try to apply attached delete_on_update_bug.py and run delete_regress
 multiple times.

 The test case should fail with:

 {{{
 ======================================================================
 FAIL: test_delete_with_custom_handlers
 (delete_regress.tests.CollectorUpdateTests)
 ----------------------------------------------------------------------
 Traceback (most recent call last):
   File "C:\data\django\tests\delete_regress\tests.py", line 381, in
 test_delete_with_custom_handlers
     self.assertEqual(cost_claim_in_report.threatened_id,
 t_org_in_report.pk)
 AssertionError: None != 2

 ----------------------------------------------------------------------
 Ran 19 tests in 0.970s
 }}}

 I can reproduce this bug with latest Django when I make several tweaks,
 that change the address space layout of the Python process, besides
 others:
 - use psycopg2 backend while testing instead of the dummy default
 - try to put pdb near the random shuffle
 - add somehow additional DLLs to the Python process etc.

 The issue can be fixed by applying delete_on_update_fix.diff.

 The root cause is the reuse of sql.UpdateQuery across the inner for-loop,
 due to the way update_batch is implemented (by calling add_update_values
 etc.).

 Caveats:
 The print statement in delete_on_update_bug.diff prints the order of the
 items when iterating over the field_updates dictionary.

 The order that breaks the test case is one of the following:

 organization None set([<ThreatenedOrganization: reported threatened org>])
 threatened None set([<CostClaim: cost claim>])
 organization None set([<CostClaim: reported cost claim>])

 i.e. update of "organization" fkey on <CostClaim: reported cost claim>
 must follow after the update of the "threatened" attr.

--
Ticket URL: <https://code.djangoproject.com/ticket/28099>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/049.a90187185d38a29393c641e51cedfe72%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Global use of UpdateQuery breaks complex delete updates

Django
#28099: Global use of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     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 milosu):

 * Attachment "delete_on_update_bug.diff" added.

 code to reproduce the bug

--
Ticket URL: <https://code.djangoproject.com/ticket/28099>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.0cfe5482831e29a3af8770f89cee8a2f%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Global use of UpdateQuery breaks complex delete updates

Django
In reply to this post by Django
#28099: Global use of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     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 milosu):

 * Attachment "delete_on_update_fix.diff" added.

 patch to fix the bug

--
Ticket URL: <https://code.djangoproject.com/ticket/28099>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.4ee61e93390083548088e413ccdca096%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Reuse of UpdateQuery breaks complex delete updates (was: Global use of UpdateQuery breaks complex delete updates)

Django
In reply to this post by Django
#28099: Reuse of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:
                                     |  Unreviewed
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

--
Ticket URL: <https://code.djangoproject.com/ticket/28099#comment:1>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.8a60404887e75f0c8497b7d2623828c9%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Reuse of UpdateQuery breaks complex delete updates

Django
In reply to this post by Django
#28099: Reuse of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:
                                     |  Unreviewed
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):

 * needs_better_patch:  0 => 1


Comment:

 I haven't investigated this is detail but it would be nice to understand
 the root cause of the issue a bit more to confirm that the fix is the best
 one. The patch should be reworked to apply to master (which supports
 Python 3 only) and the test simplified as much as possible. Existing test
 models should be used as much as possible. If any new models are needed
 they should have as few fields as possible. The test should use `create()`
 rather than `get_or_create()` and I don' think the `DoesNotExist`
 exception catching is necessary.

--
Ticket URL: <https://code.djangoproject.com/ticket/28099#comment:2>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.468ec93821521919d7b5b4dea5fa1cfe%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Reuse of UpdateQuery breaks complex delete updates

Django
In reply to this post by Django
#28099: Reuse of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):

 * stage:  Unreviewed => Accepted


--
Ticket URL: <https://code.djangoproject.com/ticket/28099#comment:3>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.351d5b7414cb38b5ae7ea6cb75314c52%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Reuse of UpdateQuery breaks complex delete updates

Django
In reply to this post by Django
#28099: Reuse of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by milosu):

 The root cause is the reuse of the sql.UpdateQuery(model) instance in the
 inner forloop.
 Repeated call to add_update_values called from update_batch is essentially
 adding value items to self.values array of the Query object.

 This can not be tested explicitly in the test case due to the way how the
 delete method on the collector is structured.

 The best way how to deal with this bug is to prevent the reuse of
 UpdateQuery.

 I can polish the testcase somehow per your instructions, but keep in mind
 that the reproducibility of the issue is hard.

 This is a critical ORM bug, which is present in Django since ages, with
 ability to randomly corrupt data in the database.

--
Ticket URL: <https://code.djangoproject.com/ticket/28099#comment:4>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.e545a749285468589d306b422202bec9%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Reuse of UpdateQuery breaks complex delete updates

Django
In reply to this post by Django
#28099: Reuse of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by Shai Berger):

 Hi,

 When you say a bug in Django is triggered by process memory layout, it
 sounds suspicious -- Python code should generally not be affected by such
 low-level concerns. So, it sounds more likely that Django's use triggers a
 bug in the underlying components which are written in C -- Python itself,
 Psycopg2, or the PostgreSQL client libraries. Such bugs are, indeed, more
 likely on Windows, where these components are less used and tested.

 It may be the case that there is such a bug, and your suggested fix makes
 Django work around it -- which may make the fix worth while, but the
 upstream projects should be notified.

 Or, unlikely as it seems to me, it may be that Django is actually doing
 something wrong here. But to show that, in my opinion, you need to show
 not just that something bad can happen when using Django -- you need to
 show where Django is violating some contract of the APIs it is using.

--
Ticket URL: <https://code.djangoproject.com/ticket/28099#comment:5>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.fa796a4d4888bf1dc7ef75de3592ea35%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Reuse of UpdateQuery breaks complex delete updates

Django
In reply to this post by Django
#28099: Reuse of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by milosu):

 Hi, guys, I'm attaching new simplified patch. Fails against Django 1.11 on
 Windows in my test setup. Even against SQL Lite.

 Clear enough now?

--
Ticket URL: <https://code.djangoproject.com/ticket/28099#comment:6>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.e8a0f0086e93e7a3fbde511c41d726de%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Reuse of UpdateQuery breaks complex delete updates

Django
In reply to this post by Django
#28099: Reuse of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by milosu):

 * Attachment "delete_on_update_bug_update.diff" added.


--
Ticket URL: <https://code.djangoproject.com/ticket/28099>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.6a4d84b5deff222bbbde69353c02aeee%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Reuse of UpdateQuery breaks complex delete updates

Django
In reply to this post by Django
#28099: Reuse of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by milosu):

 * Attachment "delete_on_update_bug_update.diff" added.


--
Ticket URL: <https://code.djangoproject.com/ticket/28099>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.022e61b7cfb11d187b9fbcf777e0c91a%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Reuse of UpdateQuery breaks complex delete updates

Django
In reply to this post by Django
#28099: Reuse of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

 Milosu, are you able to reproduce without a custom `on_delete` handler?

--
Ticket URL: <https://code.djangoproject.com/ticket/28099#comment:7>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.b661c61833fe878b29124e40380db610%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Reuse of UpdateQuery breaks complex delete updates

Django
In reply to this post by Django
#28099: Reuse of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by milosu):

 Depends on what you mean by custom on_delete handler.

 Collector.add_field_update is called from models.CASCADE - this sets None
 => can not trigger this bug. Same holds true for models.SET_NULL.

 Than there are handlers like models.SET() and models.SET_DEFAULT => bug
 could be triggered, if callable with variable output is used in these
 handlers.
 I do not have real-world use cases using these "default" handlers, but I
 can produce silly looking regression test cases using specially crafted
 field.default or models.SET callables that will also trigger assertion
 failure.

 The bug report use case is the real-world one, anyway.

--
Ticket URL: <https://code.djangoproject.com/ticket/28099#comment:8>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.fa173ed74952cad6bbcc6565857058c2%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #28099: Reuse of UpdateQuery breaks complex delete updates

Django
In reply to this post by Django
#28099: Reuse of UpdateQuery breaks complex delete updates
-------------------------------------+-------------------------------------
     Reporter:  milosu               |                    Owner:  nobody
         Type:  Bug                  |                   Status:  closed
    Component:  Database layer       |                  Version:  1.11
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:  duplicate
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):

 * status:  new => closed
 * resolution:   => duplicate


Comment:

 Closing as a duplicate of #29016 which has the same fix and simple steps
 to reproduce.

--
Ticket URL: <https://code.djangoproject.com/ticket/28099#comment:9>
Django <https://code.djangoproject.com/>
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 [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/064.baa5e4f88710414a55c4c46568baf4dd%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.