#7539 (ON DELETE support) aka ORM-09 - A call for feedback

[Johannes Dollinger]

There's a new patch on the ticket[1], based on Michael Glassford's  
work, that solves a few remaining issues and should be fully backwards  
compatible. I haven't painted the bikeshed yet, so the API still is  
`on_delete=CASCADE | SET_NONE | SET_DEFAULT | PROTECT | DO_NOTHING |  
SET(value)`. Two minor additions:

     `DO_NOTHING` does nothing. Let the db handle it or resolve the
        dependency in pre_delete (see: #10829 [2])

     `SET(value)` sets the foreign key to an arbitrary value. `value`  
may
        be a callable, `SET(None)` is equivalent to `SET_NULL`.

To make `on_delete` work on m2m intermediary models  
`DeleteQuery.delete_batch_related()` had to go. Intermediary models  
now use (almost) the same related-objects-collection code path as  
every other model (thanks to m2m-refactor). Because that would have  
lead to lots of SELECT queries for related objects, I refactored the  
collection algorithm to collect batches of objects instead of  
individual objects. That reduced the overhead to  
`#INTERMEDIARY_INSTANCES / GET_ITERATOR_CHUNK_SIZE` queries. This  
refactoring has a nice side-effect: Given the following code

     class A(models.Model): pass
     class B(models.Model): a = models.ForeignKey(A)
     class C(models.Model): b = models.ForeignKey(B)

     a = A.objects.create()
     for i in xrange(100): B.objects.create(a=a)
     a.delete()

the `delete()` call results in 103 queries with trunk, and only 4  
queries with my patch applied.

Finally, collecting related objects for auto_created intermediary  
models is short-circuited to avoid the extra SELECTs completely. The  
same could be done for any model that has no related objects, if we  
didn't need the instances to send signals (Someday/Maybe:  
Meta.send_signals = bool or tuple).

Since the constants used for `on_delete` are just callables, it's  
possible to do any kind of calculation to decide what should happen to  
related instances, e.g.:

     def delete_or_setnull(collector, field, objects):
         setnull = []
         cascade = []
         for obj in objects:
             if can_delete(obj):
                 delete.append(obj)
             else:
                 setnull.append(obj)
         SET_NULL(collector, field, setnull)
         CASCADE(collector, field, cascade)

     fk = ForeignKey(To, on_delete=delete_or_setnull, null=True)

This should probably not be documented at first, but it would be a  
nice feature once it's clear the on_delete handler signature will  
remain stable.


FIXME:
* I'd like to introduce `DatabaseFeatures.can_defer_constraint_checks`  
to disable nulling out foreign keys when it's not necessary. This  
would save a couple of UPDATE queries.

* There are ugly contrib.contenttypes imports in  
`DeleteQuery.delete_batch_related()`. I left all contenttypes related  
code there (just renamed the method, it's still called). Someday/Maybe  
this could be removed and handled as a custom `on_delete` argument on  
GenericForeignKey.

* There are no docs.


Any feedback welcome.

@glassfordm: If you're still working on this patch, I'd like to hear  
what you think (and get those tests you mentioned). I'm sorry I  
somewhat hijacked your work.

__
Johannes

[1] http://code.djangoproject.com/ticket/7539
[2] http://code.djangoproject.com/ticket/10829




--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers@...
To unsubscribe from this group, send email to django-developers+unsubscribe@...
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Michael Glassford-2]

I haven't had a chance to look at the patch yet, but what you describe
here sounds good. I don't have any problem with you "hijacking" the work.

Did your patch deal at all with the unit tests in my patch that
intentionally failed to expose things that weren't working right yet?

Mike


Johannes Dollinger wrote:

> There's a new patch on the ticket[1], based on Michael Glassford's  
> work, that solves a few remaining issues and should be fully backwards  
> compatible. I haven't painted the bikeshed yet, so the API still is  
> `on_delete=CASCADE | SET_NONE | SET_DEFAULT | PROTECT | DO_NOTHING |  
> SET(value)`. Two minor additions:
>
>      `DO_NOTHING` does nothing. Let the db handle it or resolve the
>         dependency in pre_delete (see: #10829 [2])
>
>      `SET(value)` sets the foreign key to an arbitrary value. `value`  
> may
>         be a callable, `SET(None)` is equivalent to `SET_NULL`.
>
> To make `on_delete` work on m2m intermediary models  
> `DeleteQuery.delete_batch_related()` had to go. Intermediary models  
> now use (almost) the same related-objects-collection code path as  
> every other model (thanks to m2m-refactor). Because that would have  
> lead to lots of SELECT queries for related objects, I refactored the  
> collection algorithm to collect batches of objects instead of  
> individual objects. That reduced the overhead to  
> `#INTERMEDIARY_INSTANCES / GET_ITERATOR_CHUNK_SIZE` queries. This  
> refactoring has a nice side-effect: Given the following code
>
>      class A(models.Model): pass
>      class B(models.Model): a = models.ForeignKey(A)
>      class C(models.Model): b = models.ForeignKey(B)
>
>      a = A.objects.create()
>      for i in xrange(100): B.objects.create(a=a)
>      a.delete()
>
> the `delete()` call results in 103 queries with trunk, and only 4  
> queries with my patch applied.
>
> Finally, collecting related objects for auto_created intermediary  
> models is short-circuited to avoid the extra SELECTs completely. The  
> same could be done for any model that has no related objects, if we  
> didn't need the instances to send signals (Someday/Maybe:  
> Meta.send_signals = bool or tuple).
>
> Since the constants used for `on_delete` are just callables, it's  
> possible to do any kind of calculation to decide what should happen to  
> related instances, e.g.:
>
>      def delete_or_setnull(collector, field, objects):
>          setnull = []
>          cascade = []
>          for obj in objects:
>              if can_delete(obj):
>                  delete.append(obj)
>              else:
>                  setnull.append(obj)
>          SET_NULL(collector, field, setnull)
>          CASCADE(collector, field, cascade)
>
>      fk = ForeignKey(To, on_delete=delete_or_setnull, null=True)
>
> This should probably not be documented at first, but it would be a  
> nice feature once it's clear the on_delete handler signature will  
> remain stable.
>
>
> FIXME:
> * I'd like to introduce `DatabaseFeatures.can_defer_constraint_checks`  
> to disable nulling out foreign keys when it's not necessary. This  
> would save a couple of UPDATE queries.
>
> * There are ugly contrib.contenttypes imports in  
> `DeleteQuery.delete_batch_related()`. I left all contenttypes related  
> code there (just renamed the method, it's still called). Someday/Maybe  
> this could be removed and handled as a custom `on_delete` argument on  
> GenericForeignKey.
>
> * There are no docs.
>
>
> Any feedback welcome.
>
> @glassfordm: If you're still working on this patch, I'd like to hear  
> what you think (and get those tests you mentioned). I'm sorry I  
> somewhat hijacked your work.
>
> __
> Johannes
>
> [1] http://code.djangoproject.com/ticket/7539
> [2] http://code.djangoproject.com/ticket/10829
>
>
>
>
> >
>

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers@...
To unsubscribe from this group, send email to django-developers+unsubscribe@...
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Johannes Dollinger]

Am 10.11.2009 um 17:22 schrieb Michael Glassford:

>
> I haven't had a chance to look at the patch yet, but what you describe
> here sounds good. I don't have any problem with you "hijacking" the  
> work.
>
> Did your patch deal at all with the unit tests in my patch that
> intentionally failed to expose things that weren't working right yet?

Only the first of your patches contains tests. But the ON DELETE  
related tests in this patch that do not test database-level support  
should be covered by my tests in modeltests/on_delete/.

Which of your tests failed?

__
Johannes


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers@...
To unsubscribe from this group, send email to django-developers+unsubscribe@...
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Michael Glassford-2]

Johannes Dollinger wrote:

>
> Am 10.11.2009 um 17:22 schrieb Michael Glassford:
>
>> I haven't had a chance to look at the patch yet, but what you describe
>> here sounds good. I don't have any problem with you "hijacking" the  
>> work.
>>
>> Did your patch deal at all with the unit tests in my patch that
>> intentionally failed to expose things that weren't working right yet?
>
> Only the first of your patches contains tests.

Oops, right you are. The unit tests were unintentionally omitted. I've
uploaded a new patch which is the pretty much the same as my previous
patch but with my unit tests included (although comparing the two now I
see a few other differences, probably because I also updated to the
latest code in SVN).

 > But the ON DELETE
> related tests in this patch that do not test database-level support  
> should be covered by my tests in modeltests/on_delete/.

> Which of your tests failed?

The tests that fail are in tests/modeltests/on_delete_django/tests.py.
They have names like test_issue_1a, test_issue_1b, test_issue_2a,
test_issue_2b, etc. The comments on the tests indicate the expected
result. Once the issues they test for are fixed, they should all pass.

Mike


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers@...
To unsubscribe from this group, send email to django-developers+unsubscribe@...
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Johannes Dollinger]

Am 11.11.2009 um 17:50 schrieb Michael Glassford:

>
> Johannes Dollinger wrote:
>>
>> Am 10.11.2009 um 17:22 schrieb Michael Glassford:
>>
>>> I haven't had a chance to look at the patch yet, but what you  
>>> describe
>>> here sounds good. I don't have any problem with you "hijacking" the
>>> work.
>>>
>>> Did your patch deal at all with the unit tests in my patch that
>>> intentionally failed to expose things that weren't working right  
>>> yet?
>>
>> Only the first of your patches contains tests.
>
> Oops, right you are. The unit tests were unintentionally omitted. I've
> uploaded a new patch which is the pretty much the same as my previous
> patch but with my unit tests included (although comparing the two  
> now I
> see a few other differences, probably because I also updated to the
> latest code in SVN).
>
>> But the ON DELETE
>> related tests in this patch that do not test database-level support
>> should be covered by my tests in modeltests/on_delete/.
>
>> Which of your tests failed?
>
> The tests that fail are in tests/modeltests/on_delete_django/tests.py.
> They have names like test_issue_1a, test_issue_1b, test_issue_2a,
> test_issue_2b, etc. The comments on the tests indicate the expected
> result. Once the issues they test for are fixed, they should all pass.

Thanks a lot. I ran your tests with my patch. There were a few  
(harmless) failing tests:
- sanity checks for SET_NULL on fields that are not nullable or  
SET_DEFAULT on fields that have no default value. I moved the code to  
model validation (and added tests there), so I just removed these tests.

- you assumed SET_NULL as the default behavior for nullable FKs. That  
would be backwards incompatible. I changed the default to CASCADE, so  
I had to update related tests.

The remaining failing tests are test_issue_2a, -_2b, -_2d, -_2e. They  
test if related object caches are updated/cleared after `delete()`,  
e.g.:

         data = Data.objects.create(data=1)
         m = M.objects.create(fk=data) # fk is a nullable ForeignKey
         data.delete()
         self.assertEqual(None, m.fk) #Fails

I don't think this is required. #17[1] was just rejected in the 1.2  
voting process, and without identity mapping (or a global object  
collection or excessive use of signals) you would not be able to cover  
all use-cases. If you called Data.objects.all().delete() instead of  
data.delete() you'd still have outdated related object caches. Or if  
you had obtained m through M.objects.get(..). Or ..

I'm aware that django currently *does* null out PKs of deleted objects  
as well as nullable references to those objects (which will only be  
present in objects that have themselves been deleted). But it's  
limited to instances reachable from Model.delete() via reverse one-to-
one descriptors and instances passed to signal handlers.
And finally: it's neither documented nor tested (you can remove the  
relavant four lines from delete_objects() without breaking existing  
tests).

My patch actually changes the behavior of Model.delete() in this  
regard: reverse one-to-one caches are not updated.

I'd rather see model instances as a "checkout" from the db that can  
get out of sync. Are there any deeper problems with this approach?

[1] http://code.djangoproject.com/ticket/17
__
Johannes

--

You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers@....
To unsubscribe from this group, send email to django-developers+unsubscribe@....
For more options, visit this group at http://groups.google.com/group/django-developers?hl=.