Opened 7 years ago

Closed 7 years ago

#9083 closed (fixed)

get_admin_log tag improvement to prevent extra sql queries

Reported by: santip Owned by: nobody
Component: contrib.admin Version: master
Severity: Keywords: LogEntry get_admin_log select_related content_type
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I've noticed that the default index page for the admin site uses the get_admin_log tag to load the 10 most recent changes:

This the template snippet that shows that

            {% load log %}
            {% get_admin_log 10 as admin_log for_user user %}
            {% if not admin_log %}
            <p>{% trans 'None available' %}</p>
            {% else %}
            <ul class="actionlist">
            {% for entry in admin_log %}
                ... part of the snippet removed for clarity ...
                {% trans entry.content_type.name %}    <-- here's the access to content_type 
                                                           that causes a query in each step of the loop
                ... part of the snippet removed for clarity ...
            {% endfor %}
            </ul>
            {% endif %}

This causes 11 sql queries because even though the tag uses select_related() on the queryset, this doesn't join the content_type foreign key because it has blank=True and that results in a LEFT OUTER JOIN. I understand that this is a reasonable behavior for select_related() so my change doesn't change that but changes the tag implementation to explicitly define the fields to be joined.

It's a fairly simply change which automatically reduces the number of queries of the main admin page by 10, I'm pretty sure it doesn't have any negative effects since I haven't found any other use of this tag that wouldn't require the content_type foreign key to be loaded.

If the default behavior of select_related isn't the one it is supposed to have, then this fix shouldn't be included and a fix to that functionality should be introduced, if this is the case, I'm willing to look into it and create a different patch. Also if it is the expected behavior I think it should be properly documented, once this ticket is closed I'll submit another one with the modification to the documentation.

I'm adding a patch that should suffice if you agree with my remarks.

Attachments (1)

getAdminLogRelated.patch (932 bytes) - added by santip 7 years ago.
patch file

Download all attachments as: .zip

Change History (2)

Changed 7 years ago by santip

patch file

comment:1 Changed 7 years ago by adrian

  • Resolution set to fixed
  • Status changed from new to closed

(In [9045]) Fixed #9083 -- Improved get_admin_log template tag so that it doesn't run a separate SQL query for every record in the 'history' sidebar on the admin homepage. Thanks for the patch, santip

Note: See TracTickets for help on using tickets.
Back to Top