Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimise owrt checking of double underscore relations #295

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shuckc
Copy link
Member

@shuckc shuckc commented Mar 29, 2023

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

Does this help with your query count @rafammpp ?

…_to check during save()

This favours a smaller overall query count over simpler queries.

By adding attributes during evaluation of the queryset, we can store the prior value of
ForeignKey relations, for the cost of a join for each entry within
order_with_respect_to (nested joins if LOOKUP_SEPARATOR (__) is used).

The fetched attibutes are not available during OrderedModel.__init__() so we defer creation
of self._original_wrt_map until save(). Since this reads from attributes it no longer
causes any additional queries.

This seems to balance correctness in the case of modifying the fields you are ordered
with respect to, against minimising extra queries during either get() or save()
operations, and does not require the full related object to be fetched to detect the change.
@shuckc shuckc force-pushed the optimise-double-underscore-relations branch from a7480d5 to 1b691f8 Compare March 29, 2023 15:13
@rafammpp
Copy link

rafammpp commented Mar 30, 2023

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.

@shuckc
Copy link
Member Author

shuckc commented Mar 30, 2023

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

@rafammpp
Copy link

rafammpp commented Apr 10, 2023

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.

@shuckc
Copy link
Member Author

shuckc commented Feb 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants