Django admin list does Count query twice

Django Internals
1

I didn’t want to create a bug directly, since I’m not sure if I’m missing something before reporting it.
With the admin setting show_full_result_count enabled, if you go to a list. Django does the count(*) query twice, both in ChangeList.get_results()

This is the relevant code, I added some extra comments.

        # Get the number of objects, with admin filters applied.
        result_count = paginator.count  # First count() query

        # Get the total number of objects, with no admin filters applied.
        if self.model_admin.show_full_result_count:
            full_result_count = self.root_queryset.count()  # Second count() query
        else:
            full_result_count = None

If there are filters applied to the list, there’s need for both count queries, but if there are not, there’s no need to do the count() again.

So this should avoid the second query if there are no filters. (Not tested)

        # Get the number of objects, with admin filters applied.
        result_count = paginator.count

        # Get the total number of objects, with no admin filters applied.
        if self.model_admin.show_full_result_count:
            if self.has_active_filters:
                full_result_count = self.root_queryset.count()
            else:
                full_result_count = result_count
        else:
            full_result_count = None

I’ll be happy to do a pull request, but before wasting the maintainers time with this I wanted to see if someone else could confirm my understanding.

2 Likes
2

Hmm, I’d be inclined to +1 this because a full table count could be costly :thinking:

btw you could flatten the if/else nesting to this:

if not self.model_admin.show_full_result_count:
    full_result_count = None
elif self.has_active_filters:
    full_result_count = self.root_queryset.count()
else:
    full_result_count = result_count
3

ps this is in the Mentorship sub-category, it’s possible folks interested in having a say aren’t watching here

4

I have also faced this in the past when debugging queries with django-debug-toolbar. I was puzzled for a while trying to understand why DDT would report a repeated COUNT(*) query to be performed twice, until I realized it had to do when filters were (or were not) used.

I’m +1 to having this improved!

5

Thanks for the responses, I have created an issue in the bug tracker and a pull request, hoping I did everything right as a first contributor.

2 Likes
6

I’m going back on this issue, and based on this helpful comment posted on the PR

I guess we could compare root_queryset.query.where to paginator’s queryset where

I think I managed to find a better solution:
The relevant if/else is now this:

        if not self.model_admin.show_full_result_count:
            full_result_count = None
        elif (
            self.has_active_filters
            or isinstance(self.root_queryset, Manager)
            or self.queryset.query.where
        ):
            # Get the total number of objects, with no admin filters applied.
            full_result_count = self.root_queryset.count()
        else:
            # If there are no filters, no need for an extra query.
            full_result_count = result_count

The reason for the isinstance check is because on this test the root_queryset is a custom Manager, which doesn’t have a query property, so in that case I’m assuming we need to redo the count. I don’t have the enough knowledge to know if that assumption makes sense.
I also don’t really know if it would be better to compare the root_queryset.query.where to the queryset.query.where rather than a boolean check on the queryset.query.where

Two other tests needed adjustment to adapt to the new query count. I’ve ran the entire suite in my local and everything seems to w

Now, since the original bug was closed as a wontfix, I don’t really know what’s the process to follow. Do I file a new bug or should the existing one be reopened?

7

It looks like the original ticket “#34593 extra query in admin list if there are no filters.” was closed because the PR accidentally returned a (potentially too high) cached value if there are filters. The PR should have been closed (or adjusted), but the original bug remains - the extra query is still performed, it’s just that the PR with the fix must not trigger when filters are in use, even if (as it was the case in bug #22810) the filter performs some filtering “by default” (with no query params).

I believe the original bug should be reopened unless someone is sure that no such optimization can ever be written.

@alexgmin shouldn’t cache still be used if filters are active but the queryset remains the same? Is that even possible? I’m not sure what the meaing of has_active_filters is, do you know? Is this the filter just being there or is it doing something?

8

has_active_filters is the check on whether there is an active admin filter or search, but only the ones that are directly provided by the admin.

If admin filters are active, you need two counts, since this happens:

SELECT COUNT(*) AS "__count" FROM "users_user" WHERE "users_user"."is_staff"
SELECT COUNT(*) AS "__count" FROM "users_user"

And the admin shows both counts
5 results (199 total)

(I get a 422 generic error uploading images so I had to put it as text)

9

ok but bug #22810 was triggered when a custom admin filter class performed filtering in it’s default configuration (with no parameters). Does has_active_filters catch this case?

10

It doesn’t, thats why there are multiple conditions in the second conditional.

11

Sorry to bump this up, but if no one can guide me on this I’ll just reopen the bug and do a new PR in a few days.