5 ms·

Yeah, I see this a lot. I think it's especially easy to introduce this kind of "accidentally quadratic" behaviour using magical ORMs like Django's, where an inn

Yeah, I see this a lot. I think it's especially easy to introduce this kind of "accidentally quadratic" behaviour using magical ORMs like Django's, where an innocent-looking attribute access like user.groups can trigger a database query ... access user.groups inside a loop and things get bad quickly. In the case of groups and permissions there's probably only a few of each, so fetching all of them is probably fine. But depending on your data -- say you're fetching comments written by a subset of users, you can tweak the above to use IN filtering, something like this Python-ish code: users = select('SELECT id, name FROM users WHERE id IN $1', user_ids) comments = select('SELECT user_id, text FROM comments WHERE user_id IN $1', user_ids) comments_by_user_id = defaultdict(list) for c in comments: comments_by_user_id[c.user_id].append(c) for u in users: u.comments = comments_by_user_id[u.id] Only two queries, and O(users + comments). For development, we had a ?queries=1 query parameter you could add to the URL to show the number of SQL queries and their total time at the bottom of the page. Very helpful when trying to optimize this stuff. "Why is this page doing 350 queries totalling 5 seconds? Oops, I must have an N+1 query issue!"

simonw 5y ago
Django's prefetch_related mechanism essentially implements the pattern you describe here for you: https://docs.djangoproject.com/en/3.2/ref/models/querysets/#prefetch-related https://docs.djangoproject.com/en/3.2/ref/models/querysets/#...
- benhoyt 5y ago
  Thanks. Yeah, I think I used that years ago when I first ran into this problem, and it worked well. Whether one uses an ORM or not, one needs to know how to use one's tools. My problem (not just with Django, but with ORMs in general) is how they make bad code look good. Like the following (I don't know Django well anymore, but something like this): users = User.objects.all() for u in user: print(u.name, len(u.comments)) To someone who doesn't know the ORM (or Python) well, u.comments looks cheap ("good"), but it's actually doing a db query under the hood each time around the loop ("bad"). Not to mention it's fetching all the comments when we're only using their count. Whereas if you did that in a more direct-SQL way: users = select('SELECT id, name FROM users WHERE id IN $1', user_ids) for u in user: num_comments = get('SELECT COUNT(*) FROM comments WHERE user_id = $1', u.id) print(u.name, num_comments) This makes the bad pattern look bad. "Oops, I'm doing an SQL query every loop!" The other thing I don't like about (most) ORMs is they fetch all the columns by default, even if the code only uses one or two of them. I know most ORMs provide a way to explicitly specify the columns you want, but the easy/simple default is to fetch them all. I get the value ORMs provide: save a lot of boilerplate and give you nice classes with methods for your tables. I wonder if there's a middle ground where you couldn't do obviously bad things with the ORM without explicitly opting into them. Or even just a heuristic mode for development where it yelled loudly if it detected what looked like an N+1 issue or other query inside a loop.
  - simonw 5y ago
    I agree that this stuff can definitely be handled better. https://github.com/django-query-profiler/django-query-profiler https://github.com/django-query-profiler/django-query-profil... has a neat option for detecting likely N+1 queries. I usually use the Django Debug Toolbar for this. Django's ".only()" method lets you specify just the columns you want to retrieve - with the downside that any additional property access can trigger another SQL query. I thought I'd seen code somewhere that can turn those into errors but I'm failing to dig it up again now. I've used the assertNumQueries() assertion in tests to guard against future changes that accidentally increase the number of queries being made without me intending that. The points you raise are valid, but there are various levels of mitigations for them. Always room for improvement though!
    - benhoyt 5y ago
      Thanks for the thoughtful responses and the link -- Django Query Profiler looks nice!
  - cuu508 5y ago
    users = User.objects.all() for u in user: print(u.name, len(u.comments)) This is fine if you are working with a small data set. It is inefficient, but if it's quick enough, readability trumps efficiency IMHO. Django ORM has a succinct way of doing the "SELECT COUNT(*)" pattern: users = User.objects.all() for u in user: print(u.name, u.comments.count()) And you can use query annotations to get rid of the N+1 query issue altogether: users = User.objects.annotate(n_comments=Count("comments")) for u in user: print(u.name, u.n_comments)
    - tinus_hn 5y ago
      > This is fine if you are working with a small data set. It is inefficient, but if it's quick enough, readability trumps efficiency IMHO. And this is how you end up with the problems the parent is describing. During testing and when you setup the system you always have a small dataset so it appears to work fine. But when it’s real work the system collapses.
- petters 5y ago
  select_related and prefetch_related are absolutely essential when coding Django.
kbenson 5y ago
When I was doing a lot of DBIx::Class work, I contemplated writing a plugin to allow me to lock and and unlock automatic relation fetches and die (throw) if one was attempted when locked. I would carefully prefetch my queries (auto-fill relations from a larger joined query) and if something changed or there was an edge case I missed that caused fetching deep in a loop, it might kill performance, but be hard to track down if not given the best directions on what caused it other than "this is slow". It's one of those things that in the long run would have been much more time effective to write, but the debugging never quite took long enough each time to make me take the time.
theptip 5y ago
I’m coming around to considering Django-seal to be mandatory for non-trivial applications. It will “seal” a query set so that you can’t make accidental DB lookups after the initial fetch. That way you can be more confident that you are doing the right joins in the initial query, and you are safe from the dreaded O(N) ORM issue. SqlAlchemy has this as part of the ORM, it should really be part of Django IMO.
- deleted 5y ago
  [deleted]
- c17r 5y ago
  I've never heard of django-seal before, thanks!
mschuster91 5y ago
> For development, we had a ?queries=1 query parameter you could add to the URL to show the number of SQL queries and their total time at the bottom of the page. Very helpful when trying to optimize this stuff. "Why is this page doing 350 queries totalling 5 seconds? Oops, I must have an N+1 query issue!" I'm so glad Symfony (PHP framework) has a built-in profiler and analysis tooling there...

simonw 5y ago

Django's prefetch_related mechanism essentially implements the pattern you describe here for you: https://docs.djangoproject.com/en/3.2/ref/models/querysets/#prefetch-related https://docs.djangoproject.com/en/3.2/ref/models/querysets/#...

benhoyt 5y ago
Thanks. Yeah, I think I used that years ago when I first ran into this problem, and it worked well. Whether one uses an ORM or not, one needs to know how to use one's tools. My problem (not just with Django, but with ORMs in general) is how they make bad code look good. Like the following (I don't know Django well anymore, but something like this): users = User.objects.all() for u in user: print(u.name, len(u.comments)) To someone who doesn't know the ORM (or Python) well, u.comments looks cheap ("good"), but it's actually doing a db query under the hood each time around the loop ("bad"). Not to mention it's fetching all the comments when we're only using their count. Whereas if you did that in a more direct-SQL way: users = select('SELECT id, name FROM users WHERE id IN $1', user_ids) for u in user: num_comments = get('SELECT COUNT(*) FROM comments WHERE user_id = $1', u.id) print(u.name, num_comments) This makes the bad pattern look bad. "Oops, I'm doing an SQL query every loop!" The other thing I don't like about (most) ORMs is they fetch all the columns by default, even if the code only uses one or two of them. I know most ORMs provide a way to explicitly specify the columns you want, but the easy/simple default is to fetch them all. I get the value ORMs provide: save a lot of boilerplate and give you nice classes with methods for your tables. I wonder if there's a middle ground where you couldn't do obviously bad things with the ORM without explicitly opting into them. Or even just a heuristic mode for development where it yelled loudly if it detected what looked like an N+1 issue or other query inside a loop.
- simonw 5y ago
  I agree that this stuff can definitely be handled better. https://github.com/django-query-profiler/django-query-profiler https://github.com/django-query-profiler/django-query-profil... has a neat option for detecting likely N+1 queries. I usually use the Django Debug Toolbar for this. Django's ".only()" method lets you specify just the columns you want to retrieve - with the downside that any additional property access can trigger another SQL query. I thought I'd seen code somewhere that can turn those into errors but I'm failing to dig it up again now. I've used the assertNumQueries() assertion in tests to guard against future changes that accidentally increase the number of queries being made without me intending that. The points you raise are valid, but there are various levels of mitigations for them. Always room for improvement though!
  - benhoyt 5y ago
    Thanks for the thoughtful responses and the link -- Django Query Profiler looks nice!
- cuu508 5y ago
  users = User.objects.all() for u in user: print(u.name, len(u.comments)) This is fine if you are working with a small data set. It is inefficient, but if it's quick enough, readability trumps efficiency IMHO. Django ORM has a succinct way of doing the "SELECT COUNT(*)" pattern: users = User.objects.all() for u in user: print(u.name, u.comments.count()) And you can use query annotations to get rid of the N+1 query issue altogether: users = User.objects.annotate(n_comments=Count("comments")) for u in user: print(u.name, u.n_comments)
  - tinus_hn 5y ago
    > This is fine if you are working with a small data set. It is inefficient, but if it's quick enough, readability trumps efficiency IMHO. And this is how you end up with the problems the parent is describing. During testing and when you setup the system you always have a small dataset so it appears to work fine. But when it’s real work the system collapses.
petters 5y ago
select_related and prefetch_related are absolutely essential when coding Django.

kbenson 5y ago

When I was doing a lot of DBIx::Class work, I contemplated writing a plugin to allow me to lock and and unlock automatic relation fetches and die (throw) if one was attempted when locked. I would carefully prefetch my queries (auto-fill relations from a larger joined query) and if something changed or there was an edge case I missed that caused fetching deep in a loop, it might kill performance, but be hard to track down if not given the best directions on what caused it other than "this is slow". It's one of those things that in the long run would have been much more time effective to write, but the debugging never quite took long enough each time to make me take the time.

theptip 5y ago

I’m coming around to considering Django-seal to be mandatory for non-trivial applications. It will “seal” a query set so that you can’t make accidental DB lookups after the initial fetch. That way you can be more confident that you are doing the right joins in the initial query, and you are safe from the dreaded O(N) ORM issue. SqlAlchemy has this as part of the ORM, it should really be part of Django IMO.

deleted 5y ago
[deleted]
c17r 5y ago
I've never heard of django-seal before, thanks!

mschuster91 5y ago

> For development, we had a ?queries=1 query parameter you could add to the URL to show the number of SQL queries and their total time at the bottom of the page. Very helpful when trying to optimize this stuff. "Why is this page doing 350 queries totalling 5 seconds? Oops, I must have an N+1 query issue!" I'm so glad Symfony (PHP framework) has a built-in profiler and analysis tooling there...