Combining extra(), annotate() or distinct() and count() can generate invalid SQL

[briankrznarich]

When applying a count() to a queryset that contains extra() values and either an annotation or a distinct() call, get_aggregation() discards any existing select_related tables. If the extra() components rely on those tables, the generated SQL will be invalid.

The failures are new to 1.8, and do not appear to be present in 1.7. (I discovered the problem when upgrading fom 1.7 to beta for testing).

Example:

Permission.objects.all().select_related('content_type').extra(select={'foo':"django_content_type.app_label>'q'"}).distinct().count()

ProgrammingError: missing FROM-clause entry for table "django_content_type"
LINE 1: SELECT COUNT('*') FROM (SELECT DISTINCT (django_content_type.

Remove the distinct(), or the count(), and it's fine (this is a contrived example, I know it makes no practical sense).

Another Example with some toy models:

class Address(models.Model):
    city = models.CharField(max_length=40)
class Author(models.Model):
    name = models.CharField(max_length=40)
    address = models.ForeignKey(Address)
class BlogPost(models.Model):
    data = models.CharField(max_length=40)
    author = models.ForeignKey(Author)

For each author, select a count of their total blog entries, as well as a custom boolean value based on their address:

Author.objects.all().annotate(blogpost_count=Count('blogpost')).select_related('address').extra(select={'in_maryland':"address.state='MD'"}).count()

This is fine until you append the count()

We hit this issue where TastyPie applies count() to then end of some complicated querysets, far far away from where we constructed them. In our case, we're using extra() values for an order_by that requires several comparisons.

A potential fix is in django/db/models/sql/query.py line 396 in get_aggregation().
Change

inner_query.select_related = False

To

if not self._extra: #Skip optimization, extra() fields might need these joins
    inner_query.select_related = False

The cause appears to be a performance optimization (don't do joins if the results don't appear the affect the aggregate).

This has similar keywords and errors to #11329, but I think the underlying cause is different and possibly easier to address.

[Josh Smeaton]

Great bug report, thank you. Did you want to submit a PR to fix this? If you don't, that's fine, I can get to it in the next few days.

[briankrznarich]

Thanks for the quick response. I've been poking at this trying to understand completely what's going on, and now I'm afraid that the examples I've given may simply have undefined results (which happened to work out in 1.7, but not 1.8).

In both of my examples, I relied on select_related() to ensure that the tables I needed for the extra() call would be available. However, select_related() does not make that guarantee. I've found an even simpler example that fails(and others have historically noted similar problems):

Permission.objects.all().select_related('content_type').extra(select={'foo':"django_content_type.app_label"}).order_by().values('foo')

This fails with a SQL generation error, since values() drops the contents of select_related. The empty .order_by() is needed to remove the default ordering of the Permission class, which references ContentType explicitly:

   class Meta:
        ordering = ('content_type__app_label', 'content_type__model',
                    'codename')

If I leave the default ordering in, the command succeeds, since the ordering forces the join to occur.

Here the query works until values() is called. The ValuesQuerySet class clearly references bug #3358 right before discarding select_related. Bug #3358 says it's fixed, but the source code does not agree.

class ValuesQuerySet(QuerySet):
    def __init__(self, *args, **kwargs):
        super(ValuesQuerySet, self).__init__(*args, **kwargs)
        # select_related isn't supported in values(). (FIXME -#3358)
        self.query.select_related = False

Commenting out the line simply leads to other problems.

It seems the real issue is that it isn't obvious how to ensure that a required JOIN will actually occur for extra() if your queryset is subject to modification by code you did not write. In our case, tastypie is free to append .count(), .distinct(), and .values() to whatever we might construct. The one technique that seems to work is to apply a filter() against the needed table with a clause that is always true. For example, in the Permission example above:

Permission.objects.all().filter(content_type__app_label__isnull=False).select_related('content_type').extra(select={'foo':"django_content_type.app_label"}).order_by().values('foo')

Here I've verified that a guaranteed NOT NULL field is, in fact, not null. Any decent database should manage that rather quickly. This is the technique I have applied to our source code as a workaround.

.order_by() is not even reliable for this purpose, as a distinct() applied later will discard the order_by clause.
Counting works on this query:

Permission.objects.all().extra(select={'foo':"django_content_type.app_label"}).order_by('content_type__app_label').count()

Distinct works, no problem

Permission.objects.all().extra(select={'foo':"django_content_type.app_label"}).order_by('content_type__app_label').distinct()

However, distinct()+count() fails. The distinct() is performed in a subselect with the ORDER BY clause discarded. This is in the same block of code as my original ticket description, but the fix is not so straight forward. PostgreSQL has no problem with ORDER BY in subselects, but other DBs certainly do (SQL Server, for instance).

Permission.objects.all().extra(select={'foo':"django_content_type.app_label"}).order_by('content_type__app_label').distinct().count()

I think the correct fix in the long term(if it is fixed, I know extra() is a little hacky to begin with) is for extra() to have a mechanism to specify the simple JOINs it requires. That's not easy pickings anymore. In the interim, the filter() trick seems like the safe bet. Perhaps the documentation could use some comments on the unreliability of order_by and select_related with regards to extra().

[Josh Smeaton]

For your example here you're able to use .annotate() rather than .extra(), and it'll do the right thing:

In [7]: qs = Permission.objects.select_related('content_type').annotate(foo=F('content_type__app_label')).order_by().values('foo')

In [8]: print(qs.query)
SELECT "django_content_type"."app_label" AS "foo" FROM "auth_permission" INNER JOIN "django_content_type" ON ( "auth_permission"."content_type_id" = "django_content_type"."id" )

Now that 1.8 has support for non-aggregate annotations, nearly anything that .extra(select=..) was used for can be replaced by annotate. This doesn't help you WRT django < 1.8 though.

The long term solution is probably to use .annotate and custom expressions where F() and friends don't give you the exact behaviour you're after. We'd like to reduce the use of extra as much as possible by improving the use case of custom expressions.

.extra does provide a tables argument though: ​https://docs.djangoproject.com/en/1.8/ref/models/querysets/#extra

The docs are sparse for its use, and I've never used the tables argument myself, so I'm not sure if that'll provide you with what you need. Especially since the recommendation is to place it at the start of the queryset to ensure its alias.

Do any of the ideas above help you in your particular use cases?

[briankrznarich]

Our specific need is ordering on a CASE statement. As an example, for any employee in an organizational hierarchy we wanted to show all of that employee's subordinates first, followed by all of his superiors. The subordinates are ordered top-down, the superiors are ordered bottom-up.

I was not aware of F() functions, those will be useful. Since you pointed it out, I've started perusing the new conditional-expression documentation, and it looks like we might be able to replace extra() with that.
​https://docs.djangoproject.com/en/1.8/ref/models/conditional-expressions/

extra()'s tables= argument doesn't work unless you also provide a where= clause where you do the join yourself. Otherwise the table just gets tacked on after a comma. (SELECT * FROM auth_permission,django_content_type WHERE....). This only happens, however, if the table wasn't already needed for a JOIN(which itself seems contrary to the documentation, but anyway...).

As a consequence, and a final example of a stupid Django trick, I've combined all of these misbehaviors into one fun queryset. You'd think len(queryset) and queryset.count() would yield the same value, but....

In [46]: q = Permission.objects.all().select_related('content_type').extra(tables=['django_content_type'],select={'foo':"django_content_type.app_label"})

In [47]: q.count()
Out[47]: 2188688

In [48]: len(q)
Out[48]: 2581

In [49]: q.count()
Out[49]: 2581

The call to len() caches the queryset, and permanently alters the return value of count(). The database counts show what is going on...

=> select count(*) from auth_permission;
 count
-------
  2581
(1 row)

=> select count(*) from django_content_type;
 count
-------
   848
(1 row)

=> select count(*) from django_content_type,auth_permission;
  count
---------
 2188688

Anyway, SQL generation is hard, but Django does an incredible job of it. You just can't win 'em all.

Thanks for your help. As I said above, now the I know it exists, the extra() problem is easy to work around. We'll certainly try to move to F() and friends after 1.8 is in production.

[Simon Charette]

Should we close this ticket as invalid since the reported issue seems to be caused by the wrong assumption that select_related() would always join a table and the expected query is buildable using conditional expressions instead of extra?

[Anssi Kääriäinen]

Not having the required aliases for extra select clauses is exactly the reason why extra needs to be replaced. We aren't going to (and can't) fix all the issues related to extra usage.

I think we can close this one. If we get more reports for the regression part of the bug report, and if that one is easy to fix, then lets reconsider fixing that part.