Fixed #28107 -- Added DatabaseFeatures.allows_group_by_selected_pks_on_model() to allow enabling optimization for unmanaged models.

[Tasssadar]

No description provided.

[felixxm]

@Tasssadar Thanks for this patch 👍

[felixxm]

@Tasssadar Thanks for updates 👍 . I left few more comments.

[charettes]

This seems like the right approach but here's a few comments

1. The current code is not schema aware and could break with usage of SET search_path and friends or namedspaced db_table (e.g. Meta.db_table = '"schema"."table".
2. We should try to find a more appropriate name than ordinary to categorize such tables.
3. Using Model._meta as a cache store is a big no-no IMO because it doesn't take connection aliases into account. Results should be cached using a (alias, schema, table_name) namespace.

Given the rare cases when this is useful and the complexity of implementing a bullet proof caching mechanism I'd favor simply exposing the allows_group_by_selected_pks_on_model hook and letting developers override it they deem it necessary.

[Tasssadar]

Thanks for your comments!

1. The current code is not schema aware and could break with usage of `SET search_path` and friends or namedspaced `db_table` (e.g. `Meta.db_table = '"schema"."table"`.

I copied the WHERE partly from get_table_description in PostgreSQL's introspection.py, it didn't seem to be aware of schemas either so I didn't bother :/

2. We should try to find a more appropriate name than _ordinary_ to categorize such tables.

That's the nomenclature used by Postgres docs, https://www.postgresql.org/docs/9.3/catalog-pg-class.html , that's why I put it everywhere (instead of just is_table).

3. Using `Model._meta` as a cache store is a big no-no IMO because it doesn't take connection aliases into account. Results should be cached using a `(alias, schema, table_name)` namespace.

Given the rare cases when this is useful and the complexity of implementing a bullet proof caching mechanism I'd favor simply exposing the allows_group_by_selected_pks_on_model hook and letting developers override it they deem it necessary.

Where should this hook be? I can't see a way to override connection features from the user app without creating a new DB engine :/

[charettes]

Hey @Tasssadar

I copied the WHERE partly from get_table_description in PostgreSQL's introspection.py, it didn't seem to be aware of schemas either so I didn't bother :/

get_table_description doesn't implement any form of caching so it doesn't have to deal with this extra layer of complexity.

That's the nomenclature used by Postgres docs

TIL, I didn't know that was the name used by Postgres. Thanks for pointing that out. Reading the documentation makes me wonder if the optimization works for foreign and partitioned tables though. I guess that's another reason why this should be left to the user.

Where should this hook be? I can't see a way to override connection features from the user app without creating a new DB engine :/

That would effectively require the user to subclass the Postgres db engine which is not as daunting as it might seems.

[Tasssadar]

I have removed the autodetection and just added a hook that just returns managed by default. I'm not sure about the tests, I think it's not necessary to change them for this hook so I reverted them back to original.

The allows_group_by_selected_pks boolean in features.py is now useless I guess. It can be replaced just with the allows_group_by_selected_pks_on_model method that returns False by default and model._meta.managed in posgresql backend, do you prefer that to keeping both the bool and method, as it is now in the PR?

I tried writing the subclassed engine, and it's not bad:

from django.db.backends.postgresql import base, features

class DatabaseFeatures(features.DatabaseFeatures):
    def allows_group_by_selected_pks_on_model(self, model):
        return True

class DatabaseWrapper(base.DatabaseWrapper):
    features_class = DatabaseFeatures

I'm not sure how you're merging PRs, there's a lot of commits here now with useless code, I can rebase it to master if you wish.

[Tasssadar]

Hello, any followup please? The tests failed, but I'm fairly sure it was something in the test environment wrong, not the code.

[felixxm]

@Tasssadar Thanks. I rebased from master, squashed commits, restored one test and added release notes.

[Tasssadar]

Rebased due to conflicts, the failing test is caused by ed2d411 and not this PR.

[felixxm]

@Tasssadar Yes my mistake, I will fix this in a minute.

[felixxm]

@Tasssadar @carltongibson I rebased from master, moved "Subclassing the built-in database backends" to a separate commit and rephrased it a bit. IMO we're ready to merge 🎊

[carltongibson]

Yes, great stuff @felixxm. That reads very nicely.

First, you have to create a new directory with a base module in it...

Smooth 🙂

👍

[felixxm]

@Tasssadar Thanks, welcome aboard ⛵

@carltongibson Thanks for checking 👍

[Tasssadar]

Thank you! This first release with this included will be 3.0 in December, right?

[felixxm]

Yes.