Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#28549 closed Bug (fixed)

Can't defer() fields from super- and sub-class at the same time

Reported by: Jeremy Kerr Owned by: Jeremy Kerr
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Jeremy Kerr, Simon Charette Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Using the models:

from django.db import models

class Base(models.Model):
    f1 = models.CharField(max_length=10)

class Sub(Base):
    f2 = models.CharField(max_length=10)

it seems that I can't defer() both f1 and f2 in a single query:

>>> Sub.objects.defer('f1', 'f2').query.sql_with_params()[0]
u'SELECT "defer_base"."id", "defer_base"."f1", "defer_sub"."base_ptr_id" FROM "defer_sub" INNER JOIN "defer_base" ON ("defer_sub"."base_ptr_id" = "defer_base"."id")'

(we're seeing f1 in the SELECT value list).

I seem to be able to defer f1 or f2 separately though.

I'm no django hacker, but: it seems as though django.db.models.sql.query.Query.deferred_to_data() is iterating both models in the loop:

#640:
            for model, values in six.iteritems(seen):
                for field in model._meta.fields:
                    if field in values:
                        continue

- and so is discovering f1 twice: once as Base.f1 and again as Sub.f1. Since the field in values test only skips Base.f1, we're still left with Sub.f1 in the workset dict.

Change History (10)

comment:1 by Jeremy Kerr, 7 years ago

Cc: Jeremy Kerr added
Component: UncategorizedDatabase layer (models, ORM)
Type: UncategorizedBug

comment:2 by Simon Charette, 7 years ago

Triage Stage: UnreviewedAccepted
Version: 1.11master

I think your intuition is right and that the bug lies in deferred_to_data(). Could you try replacing for field in model._meta.fields by for field in model._meta.local_fields and see if it helps?

in reply to:  2 comment:3 by Jeremy Kerr, 7 years ago

Replying to Simon Charette:

I think your intuition is right and that the bug lies in deferred_to_data(). Could you try replacing for field in model._meta.fields by for field in model._meta.local_fields and see if it helps?

Yep, that seems to fix the issue:

>>> Sub.objects.defer('f1', 'f2').query.sql_with_params()[0]
u'SELECT "defer_base"."id", "defer_sub"."base_ptr_id" FROM "defer_sub" INNER JOIN "defer_base" ON ("defer_sub"."base_ptr_id" = "defer_base"."id")'

I was concerned that using local_fields would omit inherited fields from the query, but looks like that's not the case; inherited (but not deferred) fields work fine with that change:

class Base(models.Model):
    f1 = models.CharField(max_length=10)
    f3 = models.CharField(max_length=10)

class Sub(Base):
    f2 = models.CharField(max_length=10)
>>> Sub.objects.defer('f1', 'f2').query.sql_with_params()[0]
u'SELECT "defer_base"."id", "defer_base"."f3", "defer_sub"."base_ptr_id" FROM "defer_sub" INNER JOIN "defer_base" ON ("defer_sub"."base_ptr_id" = "defer_base"."id")'

- correctly includes f3 in the query. Behaviour also look to remain correct when only deferring fields from one class too.

comment:4 by Jeremy Kerr, 7 years ago

Owner: changed from nobody to Jeremy Kerr
Status: newassigned

Patch coming.

comment:5 by Jeremy Kerr, 7 years ago

Has patch: set

Pull request sent: PR

Since this is a trivial patch (and my first), I've not added to AUTHORS or submitted a CLA. Let me know if I need to do either of those.

comment:6 by Simon Charette, 7 years ago

Cc: Simon Charette added

Awesome, thank you for submitting a PR and adding a test!

comment:7 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In 84b7cb7d:

Fixed #28549 -- Fixed QuerySet.defer() with super and subclass fields.

Previously, deferring fields in different classes didn't omit the
superclass' deferred field.

Thanks Simon Charette for the suggested fix.

comment:8 by Chris Lamb, 7 years ago

Curious why this didn't reach into the 1.11.6 bugfix release.? We are carrying this patch in Debian after a user request:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=876816

(It was the cause of performance regressions in his application so seemed sufficiently important to carry)

comment:9 by Simon Charette, 7 years ago

Chris, probably because it was never reported as a regression in the first place. Can you confirm this was working in a previous version of Django?

in reply to:  9 comment:10 by Chris Lamb, 6 years ago

Replying to Simon Charette:

Chris, probably because it was never reported as a regression in the first place. Can you confirm this was working in a previous version of Django?

Yep. If I understand your question correctly, from: https://bugs.debian.org/876816#5:

This bug caused significant performance degradation when we upgraded a
Django [1.x] application to a new version that relied on model inheritance.

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