+972 52-548-6969
Reviewing NerdDinner: The Select N+1 pitfall
During my review of NerdDinner, I came across the following piece of code:
And I knew that there is a Select N+1 problem here. This is quite similar to the problem that I described here. RSVPs is a lazily loaded collection. As such, calling Any() on it will result in a database query.
Now the question was whatever or not it is used somewhere in a loop. ReSharper will help us figure that one out:
The first usage location is here:
And this certainly doesn’t look like it can cause a Select N+1, right. Just to be sure, I checked where it is called from the client side as well:
This looks all right, we will have a query, but it is only one per request.
Great, now let us take a look at the second usage. You can tell that this is going to be a problem just by the ascx suffix. The code is pretty simple:
I have a moral issue with the view generating a call to the database, but at least it isn’t being done in a loop.
I am certain, however, that we are going to find this .ascx file in a grid, which will cause a loop. This .ASCX file is being used in Details.aspx and is used like this:
However, it is not actually being used in a loop.
There is not a Select N+1 issue, which renders this entire post moot.
However, while it is not an active Select N+1, it is a dormant one. It would be very easy to have a new requirements that would make use of the RSVPStatus.ascx file in a loop and cause the issue.
Comments
It looks like the function was designed to see if the currently logged-in user is registered; and it is not like you can have several logged-in users per context at the same time.
But I can see where a scenario like this can cause a potential SELECT N+1 problem. Traditional lazy-loading is not the only way to create this problem.
Interesting point about your moral objection to the view kicking off a database call.
In this case, the view doesn't know it's causing a database call. It's merely a consequence of returning an IQueryable which defers the call till it's being accessed.
I don't think this is necessarily bad in this case because the view is in process with the controller and very unlikely to be disconnected from the controller. Why do I say that? Well the responsibility for setting up the database call still lies in the controller and model. So I would say that separation of concerns is maintained. After all, if you called ToList() in the controller, the view code wouldn't have to change.
However, from another angle, I understand the moral objection from the perspective of it's kind of hidden. What if I dispose the data context in the controller action method. That seems like a reasonable thing to do, but would cause a problem.
In any case, Spark View Engine's new output caching feature relies on delayed execution of the model: whereslou.com/2009/07/28/spark-output-caching
What do you think of that? It solves a particular problem in a very neat manner, but it faces your moral objection.
I love this post.
It clearly illustrates the issues that can show up if you do something "immoral". Even if it can seem trivial on each level alone, somewhere down the pipeline, it'll get ya.
More interestingly, I'm curious about better approach you would use to tackle that in this particular example.
The concept of rich domain-model and lazy-load often colide each other, sometimes causing massive N+1 explossion.
As another example, let's say User has a list of RegisteredRsvps, defined as lazy-load association.
And the user has instance property (or method):
public bool IsRegistered
{
}
This is a pretty simple and valid domain design (I believe). But it's a dormant N+1. (perhaps all attempts for rich domain abstractions are likely to end up with dormant N+1s).
That code works fine for current purposes, until we have a page that displays all users in a grid with a 'Status' column (Registered or Unregistered). It will fire an N+1 call.
How would you tackle this? Do all controllers (who're about to loop over User.IsRegistered property) need to know they got to tell UserRepository to force join-load ReservedRsvps property? Why does UI controller even need to know about this knowledge anyway?
Haacked,
I follow the logic of doing that in the view, in fact, I can argue for it for delayed batch execution purposes (when the later you run it, the better you are).
I don't like it very much because it tend to create problems down the wrong, big ones.
Changing the view should NOT be a cause to change the database behavior, and that is very often the case in such things.
As for output caching, I don't like the way spark is doing that at all. HTML generation, for the most part, is the cheapest part in the whole deal, and not something that I particularily worry about.
Data is what I tend to cache, and for that, I certainly don't want to do it in the view.
Hendry,
View Models, mostly. Which force me to explicitly marshal the value.
More likely, I would simply watch stuff like in NH Prof
I agree and I'm a firm believer of VIewModel.
But how does it solve N+1 problem? Doesn't it merely shifts the N+1 problem from View to Controller?
"ReSharper will help us figure that one out"
What 's the shortcut to get that resharper menu open?
I only know of the right click Find Usages that opens in a Tool Window
@John
alt+shift+F12
excellent post and great point about spark's html data caching!
still, the question pointed by Hendry remains open, so the only remedy is to do an eagear load?
(not in L2S but in generall as L2S eager load is flawed anyway)
the nhProf note made a little grin on my face though :D
Aren't you going too far in the hunt for Select N+1? I mean, you assume there's Select N+1 problem before even seeing the application at work or looking at database queries. This is not a very valuable review method.
Hendry,
Yes, it does. But it does so in a way that make is explicit that something is going on there, in most cases.
Look at the key map
ctrl+alt+f7, ctrl+shift+f7 on my machine
cowgaR,
Yes, you need to eager load
Rafal,
all apps have select n+1
You start from that assumption, and you can only prove it to be false under very limited circumstances
Yes, all apps have select n+1 problem, like all apps use too much memory and execute inefficient code. But is it a problem if the application does its job and gives acceptable results? Look, you can put any code in a loop so if I followed your logic I would have to declare all components as 'dormant' problems.
Rafal,
I wouldn't go quite that far.
The reason for this post is that I run into a bit of code that met the "we have select n+1 here" format quite well, and then tracked it down to end up in not having a select n+1.
There is still an issue of a superfluous select, and I think that showing suspicious code can be very helpful.
Comment preview