If order_with_respect_to is a foreign key of a foreign key it generates aditional queries. #293

rafammpp · 2023-03-28T15:23:25Z

Example

class Site(models.Model):
    ...

class Category(models.Model):
    site = models.ForeignKey(Site)

class Article(OrderedModel):
    order_with_respect_to = 'category__site'
    category = models.ForeignKey(Category)

If I do a queryset of Articles, it make an aditional query for every article, even if I use Article.objects.select_related('category__site').

@ibaguio manage to fix this problem in c4630dc for a simple foreign key, but if you order with respect to a foreign key of a foreign key it makes a query to retrieve the category.

If I understand it well, you save the initial value of the wrt_map to the instance at init time to check for changes later on save, and then do the proper arragments if needed. wrt_map is a dict of every item in order_with_respect_to with it's respective values. But you only need it on save. My proposal is to get the saved value from the db, and then check if it's different from the current instance to be saved. Yes, we make an aditional query, but only when we need it (on save) and can be the cheapest query if we defer all values other than the pk.

I've a view that list articles. Currently it makes over a thousand queries, and with this patch implemented, or with the previous version of this package, it gets down to eight.

I'm gonna make a pull request with my proposal, see you there.

The text was updated successfully, but these errors were encountered:

shuckc · 2023-03-29T11:05:17Z

I'm aiming for it not be an additional query because those fk values have already been fetched from the DB when the object was queried. This does not currently hold with a 'double underscore' relation, which looks to be your case.

We spoke about how to move forward during review of that PR. It seems to me that there should be a modification to the QuerySet to dynamically add the extra relation, perhaps adding Article.objects.select_related('category__site_id'), then a query-counting test case around it to make sure it holds.

At the moment the admin does multiple subqueries whilst constructing a similar, but different, map of the ordered set for use in constructing the admin change links. It is with optimising this that I do not want to defer the lookup of order fields until save occurs on the model.

ibaguio · 2023-03-31T06:41:55Z

@shuckc, i'm personally always cautious when using select_related as the default solution to an FK problem, coz it "downloads" the entire related table, where you only need one field. My years of django experience has taught me to always default to use annotations, and only use select_related when you can't help it.

For @rafammpp 's problem, i think this can be solved by annotating the values on the queryset, top of mind here's how i would implement it:

class OrderModelQuerySet:
    def annotate_wrt(self):
        owrt = self.model.order_with_respect_to

        if any("__" in wrt for wrt in owrt):
            return self.annotate(
                **{
                    field_path: F(f"{field_path}__id")
                    for field_path in owrt
                    if "__" in field_path
                }
            )
        
        # no dunder relation found in order_with_respect_to
        # no annotations needed      
        return self

class OrderModelManager:
    def get_queryset(self):
        # Annotate the WRT by default
        return super().get_queryset().annotate_wrt()


# Sample model
class Answer(OrderdModel):
    order_with_respect_to = ("question__asker")


a = Answer.objects.all()
a.question__asker  # this will print a.question.asker_id, without any additional SQL query, since it's annotated

Here we are annotating the values of the related fields by default.
I can write the PR for this if you think this is a viable solution.

Notes:

Code above is untested, but should give you the idea.
We need to also consider RelatedManagers, i.e question.answers.first() will not trigger the get_queryset afaik.
We will need to add a fallback, when the value is not annotated, it has to go through the inefficient method of following the relation

shuckc · 2023-03-31T10:27:03Z

8000

Yes that's the idea you mentioned previously, I put it together (using annotate())two days ago on #295
https://github.com/django-ordered-model/django-ordered-model/pull/295/files#diff-62376346d0f6565502ee25a8a0af0478fc35c504178a023026ca9bf462e2ab0dR100

I annotated non-dunder FK's too, as that is presumably done at no cost and keeps the handling similar.

shuckc · 2023-03-31T10:35:22Z

You have a good point about needing a better fallback for RelatedManagers - if you modify then save the object derived from one of those, it will not be annotated. My code assumes it is new object in this case and will not upshuffle the departing wrt set. I'll add a test for it.

During model __init__() the annotations are not yet available (and you don't know if they ever will be...), so I read the annotations during save(). However at that point its too late to examine the 'prior' value of double underscore relations, as they may have been changed, and repeating the DB query into a shadow object to inspect (as OP @rafammpp does in #294) is itself not entirely without issues.

* add failing test for django-ordered-model#293 * Query ForeignKey id values during load to optimise order_with_respect_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. --------- Co-authored-by: Chris Shucksmith <chris@shucksmith.co.uk>

ZipBrandon · 2024-08-12T15:57:16Z

Can we resurrect this?

rafammpp mentioned this issue Mar 28, 2023

[WIP] Don't do aditional queries if order_with_respect_to is a foreign key of a foreign key #294

Draft

shuckc added a commit that referenced this issue Mar 29, 2023

add failing test for #293

3aaf7fa

shuckc added a commit that referenced this issue Mar 29, 2023

add failing test for #293

c75b839

shuckc mentioned this issue Mar 29, 2023

Optimise owrt checking of double underscore relations #295

Open

riconnon pushed a commit to riconnon/django-ordered-model that referenced this issue Jul 23, 2023

add failing test for django-ordered-model#293

53dd6ba

riconnon pushed a commit to riconnon/django-ordered-model that referenced this issue Oct 8, 2023

add failing test for django-ordered-model#293

dd7779d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

If order_with_respect_to is a foreign key of a foreign key it generates aditional queries. #293

If order_with_respect_to is a foreign key of a foreign key it generates aditional queries. #293

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

If order_with_respect_to is a foreign key of a foreign key it generates aditional queries. #293

If order_with_respect_to is a foreign key of a foreign key it generates aditional queries. #293

Comments

Uh oh!

Example

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!