This post is my digest of facts and opinions on the infamous NHibernate parameter sizes controversy.
Disclaimer: Everything that follows is MY INTERPRETATION of the story, based on MY UNDERSTANDING of the statements and replics made by all the participants.
The issue was first reported in a blog post by Grant Fritchey on 04/29/2008. He discovered that the way NHibernate (then v1.2) generates some of its parametrized queries (specifically for inserts of entities) causes a proliferation of execution plans in MS SQL Server's (then v2005) execution plan cache. This happens because NHibernate uses actual length of parameter's value as the size of the variable in the generated SQL query.
The issue has resurfaced on 03/03/2009 in this thread on Italian section of NH User Group by Claudio Maccari (the conversation is in Italian so here's a link to google-translated version). In this post Claudio further reports that it's not just entity operations, but regular queries (HQL and ICriteria) that also suffer from the same problem. [Since entity inserts/upates/selects always go by PKs which rarely use variable-length types the impact of the issue would be low if it did not apply to queries -- zvolkov 10/28]. Fabio first thinks its a Fluent NH problem, then he theoretizes it could be a mapping problem (due to length="X" missing). Claudio provides numerous examples of incorrect behavior (even with length="X" present) and proposes setting "prepare_sql" to true as a possible solution. Claudio shows how this setting forces NH to use nvarchar(4000) for string parameter sizes. He also shows how using an overload of SetParameter (e.g. TypeFactory.GetStringType(50) or TypeFactory.GetAnsiStringType(50)) one can override both type and size. Fabio seems surprised and frustrated at the apparent difficulties NH presents for dealing with parameters.
On 03/10 Claudio publishes a post on his blog, in which he explains the issue and the solutions he found so far.
On the same day, 03/10, Daniel Auger, having found the original post by Grant Fritchey, starts another thread on NH Users Group. Fabio seems frustrated with apparent misunderstanding of the nature of the issue, points out this is a MS SQL Server specific issue and implies that the fix should be done in MS SQL Server itself. He also repeats Claudio's idea to use prepare_sql=true as a way to deal with the issue.
On 3/13, the question is raised at the very beginning of Herding Code podcast, episode #38 with Ayende et al. Despite lacking complete info, Ayende lightheartedly dismisses the issue as not existent or(!) perhaps being easy to fix.
On 03/15, in a comment to Claudio Maccari's blog post, Fabio again claims the problem to be a MS SQL Server issue. Claudio argues that the issue should be fixed in NHibernate by making MsSql2005Dialect "use Length attribute by-default".
On 03/16, by Fabio's request Claudio opens a JIRA ticket (NH-1707) to default prepare_sql to true for MS SQL Server. During subsequent several months, the change is implemented, reverted, and reimplemented again, as NH team discovers and fixes related issues (NH-1713, NH-1710, NH-1718). At one point Fabio even reverts the ticket, concerned that defaulting prepare_sql to true may require user code changes, to call a different overload of SetParameter. The final version of NH 2.1, has the change INCLUDED EXCLUDED (~ zvolkov 11/2).
On 04/01, Carsten Hess replies to Daniel Auger's NH user group thread, and points out the difference between parametrized (sp_executesql) and prepared (sp_prepexec) queries. He points out that prepared queries (used when prepare_sql is set to true) are only reused for the same connection which will cause proliferation of query plans as generated by multiple connections. (This statement later proves not accurate, see below -- zvolkov 11/2) Based on this argumentation he suggests to not set prepare_sq to true and instead solve the problem in SqlClientDriver.cs by making GenerateCommand always call SetParameterSizes. However, he argues, even that fix would be suboptimal as the generated parameters will be of type nvarchar(4000) unless the undocumented type="String(100)" syntax is used in the mappings (this turns out to be incorrect as well ~zvolkov 11/2)
On 05/02, Ayende posts on the issue, claiming that prepare_sql=true is the right solution to the problem. On 05/04 Carsten Hess comments on the post, repeating his line of argumentation, but Ayende disables the comments w/o replying.
The issue resurfaces again on 10/26/2009, in a blog post by Naz of objectreference.net. Besides restating the problem, the post clearly illustrates how this specific issue (whether it's a real problem or not) hurts NH adoption.
On same day of 10/26, Daniel Auger opens a new thread on NH Users Group, requesting a clarification on the status of the issue. At first, Fabio can't seem to remember the exact issue and even suggests it was fixed in NH 2.1.1, but finally he repeats his claim that this is a MS SQL Server issue. Other users agree with the assesment but repeat the NH adoption argument. Fabio provides detailed instructions for subclassing SqlClientDriver and injecting it through connection.driver_class setting. Commenting on the possibility of making this change a permanent part of NH trunk, Fabio insists that the condition of calling SetParameterSizes only when prepare_sql=true must be there for a reason and expresses concern that such change may have unintended consequences in some scenarios [i.e. for data types other than strings and RDBMSes other than MS SQL Server -- zvolkov 10/28]. He also replies to the adoption argument by saying that every technology has a limited lifecycle and so does NH. At the end, Fabio seems to be open to the idea of making the SetParameterSizes change, provided the normal procedure is followed (JIRA ticket complete with explanation of the issue and failing tests).
Note: this part of the post has been updated since it was first written. Click here to see the new ending.
As you see, the jury is still out on what's the right way to fix the issue. From my perspective, prepare_sql=true is definitely evil, while always calling SetParameterSizes definitely improves current behavior in most common scenarios. Whether or not always calling SetParameterSizes is safe in exotic scenarios we shall live and see, meanwhile it does seem to be the direction the community leans towards.
In light of the above, and until the change is implemented in NH, we have to do the following:
- set prepare_sql=false in NH settings
- Create you own class inheriting from SqlClientDriver and override GenerateCommand to always call SetParameterSizes:
using System.Data;
using NHibernate.Driver;
using NHibernate.SqlCommand;
using NHibernate.SqlTypes;
namespace XXX.YYY.ZZZ
{
public class CustomSqlClientDriver : SqlClientDriver
{
public override IDbCommand GenerateCommand(CommandType type, SqlString sqlString, SqlType[] parameterTypes)
{
var command = base.GenerateCommand(type, sqlString, parameterTypes);
SetParameterSizes(command.Parameters, parameterTypes);
return command;
}
}
}
- set connection.driver_class to assembly-qualified name of your custom driver implementation
- (optional) add type="AnsiString(LENGTH)" to mappings of string properties to make NH use varchar(LENGTH) instead of nvarchar(4000) for most basic queries
- Keep an eye on more complex queries and be ready to use the overload of SetParameter that takes TypeFactory.GetAnsiStringType(LENGTH)
Alternatively, someone should assess Claudio's idea of making MsSql2005Dialect "use Length attribute by default" as that could prove to be less-intrusive than always calling SetParameterSizes.
Note: this part of the post has been updated since it was first written. Click here to see the original ending.
On 10/30, Carsten Hess came back with a statement that turned the story on its head. According to his findings, execution plans generated by sp_prepexec ARE actually global and not per connection (unlike he said before), so having prepare_sql=true does not cause query cache pollution, at least not in NH 2.1 (apparently NH 2.0 may still have some issues since some parametrization defects were not fixed until 2.1). Among other things this means we no longer have to override GenerateCommand in SqlClientDriver to always call SetParameterSizes, as that's what NH 2.1 does by default, with prepare_sql set to true (this is no longer accurate, see below ~zvolkov 11/2). This also means Fabio and Ayende were right in their downplaying of the issue, even though I still think they did a bad job communicating their position to the public (something I can't help but attribute to cultural differences).
After my first correction of this post, on 11/2, Naz replies to the NH thread. He clarifies that prepare_sql is in fact NOT defaulted to true in NH 2.1, and so the users will still have to make this configuration change by hand.
The same day, Trent Niemeyer in a comment to my second blog post corrects another Carsten Hess's statement and says that (at least in v2.1) NHibernate does NOT ignore length attribute in the mappings and that undocumented type="String(100)" syntax is no longer the only way to effectively specify the length of query parameters.
I just verified both news and found them to be correct (and their original counterparts, correspondingly, wrong). As V.I. Lenin once said: "Trust, but verify!"
To summarize, all we need to do is:
- set prepare_sql to true
- (optional) add type="AnsiString" and length="LENGTH" to mappings of string properties to make NH use varchar(LENGTH) instead of nvarchar(4000) for most basic queries
- Keep an eye on more complex queries and be ready to use the overload of SetParameter that takes TypeFactory.GetAnsiStringType(LENGTH)
The reason prepare_sql is an opt-in feature is because it may change behavior of existing applications. Still, it is highly recommended that users of MS SQL Server set it to true, to avoid the query cache pollution. The alternative solution -- the SqlDriverOverride hack -- can be employed to keep the NH-generated SQL queries visible in SQL Profiler every time they are executed, and not once per connection, which is what happens when prepare_sql is set to true:
- leave prepare_sql at its default value of false
- Create you own class inheriting from SqlClientDriver and override GenerateCommand to always call SetParameterSizes:
using System.Data;
using NHibernate.Driver;
using NHibernate.SqlCommand;
using NHibernate.SqlTypes;
namespace XXX.YYY.ZZZ
{
public class CustomSqlClientDriver : SqlClientDriver
{
public override IDbCommand GenerateCommand(CommandType type, SqlString sqlString, SqlType[] parameterTypes)
{
var command = base.GenerateCommand(type, sqlString, parameterTypes);
SetParameterSizes(command.Parameters, parameterTypes);
return command;
}
}
}
- set connection.driver_class to assembly-qualified name of your custom driver implementation
- (optional) add type="AnsiString" and length="LENGTH" to mappings of string properties to make NH use varchar(LENGTH) instead of nvarchar(4000) for most basic queries
- Keep an eye on more complex queries and be ready to use the overload of SetParameter that takes TypeFactory.GetAnsiStringType(LENGTH)
To conclude the post, here are the lessons I learned from this story:
- If you think you've found NH 2.1 does something stupid that will kill your production server you're most likely wrong. NHibernate 2.1 is a mature enterprise-ready ORM with most major wrinkles already ironed out.
- If your customer community thinks your product has a bug and you know it does not, they will do their best to fix it anyway, unless you take time to explain there is no bug.
- In serious organizations the DBAs are a big power, better have them as your friends.
- Blogging about programming controversies is not the best way to drive traffic to your website. Breaking news work better and are much easier to make.
- Never take anything on faith w/o verifying the facts for yourself (added 11/2)
Tags: