django-ordered-model icon indicating copy to clipboard operation
django-ordered-model copied to clipboard

Optimise owrt checking of double underscore relations

Open shuckc opened this issue 2 years ago • 6 comments

Tentative fix for #293, however it's not straightforward and mandates use of our ModelManager subclass.

Does this help with your query count @rafammpp ?

shuckc avatar Mar 29 '23 15:03 shuckc

Wow! Yes, it does. Now I'm at 9 queries. I made my own aproach #294 , requires two aditional queries on save. I'm wan't to investigate what happens in the admin, and why do you need this.

Edit: It's 8 queries, same with 3.6 version.

rafammpp avatar Mar 30 '23 09:03 rafammpp

I wonder why 9 rather than the 8 queries you had under 3.6.x series? Any chance you can diff them?

shuckc avatar Mar 30 '23 13:03 shuckc

I checked again and now it's 8 queries in both 3.6 version and this pull request. I think sometimes debug toolbar do an aditional query for inspecting or anything.

rafammpp avatar Apr 10 '23 10:04 rafammpp

I would like to get this into shape for the next release 3.8 as its the final piece.

The annotations added here allow us to very cheaply preserve OWRT values (to test for changes) beyond double-underscore relations by fetching all required values in the single initial query rather than walking related objects one by one. We need to support OWRT ending on both a foreign key (adding _id) and a regular field as seen in #317.

If the object is constructed by a RelatedManager (e.g. from Django's Many2Many field's related set) it bypasses our custom QuerySet so we are built without the annotations. However we have introduced ordered_model.fields.OrderedManyToManyField in #285 (to solve the resulting lack of ordering) so we can add our annotations to that code.

@rafammpp posted nice code in #294 to build the 'intitial value' owrt map during save by re-querying the database. I don't want to do this in every case as it's an extra DB query. However as a fallback for the now (very limited) case of saving objects from the RelatedManager and having not used our custom field, I think it should be fine.

So the plan is:

  • preserve simple (non-dunder) field values during __init__() since they are always awaiting at this point
  • annotate dunder-reached fields during ModelManager and our own RelatedManager on a best-effort basis
  • in save(), check for required annotations, and if missing, build the 'shadow' model instance using OrderedModelManager by pk and borrow the missing annotated values from that instance.
  • do the delete/insert logic if OWRT field values have changed

This should keep the minimal number of queries in each case. If a large number of instances from the same RelatedManager are modified, then yes we have an n+1 query, however the fix is to use our OrderedManyToMany field.

shuckc avatar Feb 06 '24 17:02 shuckc