search

nhibernate / nhibernate-core

NHibernate Object Relational Mapper
https://nhibernate.info
GNU Lesser General Public License v2.1
2.13k stars 924 forks source link

NH-3403 - Wrong parameter size in query with MsSql2000Dialect,MsSql2005Dialect and MsSql2008Dialect #1300

Closed nhibernate-bot closed 6 years ago

nhibernate-bot commented 6 years ago

Carlos Bustos created an issue — :

The mapping class :

I do the query with :

session.CreateQuery("from Person p where p.Name =:name") .SetParameter("name", "carlos",TypeFactory.GetAnsiStringType(8)) .List();

If I use NHibernate 3.2.0.4000 the generated query is:

NHibernate: select person0.Id as Id0, person0.Name as Name0 from Person person0 where person0.Name=@p0;@p0 = 'carlos' <Type: AnsiString (8)>

but if I use any higher version the generated query is:

NHibernate: select person0.Id as Id0, person0.Name as Name0 from Person person0 where person0.Name=@p0;@p0 = 'carlos' [Type: AnsiString (8000)]

Oskar Berggren added a comment — : It it a problem?

What should happen if the query is: session.CreateQuery("from Person p where p.Name like :name") .SetParameter("name", patternFromUser, TypeFactory.GetAnsiStringType(8)) .List();

and the user has entered pattern such as "alexander"?

The change you see is because NHibernate will no longer silently truncate the parameter when it's too long. So the question is, does the new behavior cause an actual problem?

Carlos Bustos added a comment — : This is a performance issue. If the user enters a value larger than the size of the field, an exception should be thrown. It´s may cause a performance problem

2013/2/19 Oskar Berggren (JIRA) jira@nhibernate.jira.com

-- by Carlos Javier Bustos

Prabu Weerasinghe added a comment — : Oskar,

I agree with Carlos here. The issue with setting the length to a fixed value is that an implicit conversion is performed by SQL Server when processing the query. This adds a performance hit, which can be significant in certain scenarios.

I have looked at the source, and it seems the SQLClientDriver is where the issue is. It ignores the length property of the SqlType and always set's the length to the default value. I have resolved this issue on a local version of NHibernate for the current project I'm working on.

Oskar Berggren added a comment — : What's your take on the issue described in NH-3036?

Prabu Weerasinghe added a comment — : Oskar,

It's an interesting problem with the LIKE clause and the query generated. I see the reasoning why the approach to use the default maximum length, but it removes control from the developer.

Currently in my branch for NH, I am using the default maximum length if the parameter value's length is greater than the user's defined length, otherwise I stick to the user defined length.

I would prefer not to use the default maximum length, unless the user has not provided a length or a LIKE clause is used. Ideally, I would like to be able to know when the LIKE clause is being used in association to a parameter, and use that as a control to use the default maximum length for the parameter.

We found by specifying the correct parameter lengths in our queries, we achieved performance gains of up to 4 times, especially in larger tables (more implicit conversions were performed to resolve the query).

What are your opinions regarding the issue?

Colum Clissmann added a comment — : Oskar,

I would concur with Prabu as using the defaults does add a performance hit in certain circumstances. Though so long as the type is specified correctly (e..g AnsiString) the performance is okay. We had the type wrong with meant SQL doing a conversion on every row in an index scan. Getting the type fixed this and thus made this change fine with us.

Probably an irrelevant suggestion but wouldn't you only have to increase field size by 2 characters to allow for the % in the parameter?

Regards, Colum

Oskar Berggren added a comment — : I wonder... if a parameter size of e.g. 8000 is specified, will the sql server client library actually transfer 8000 bytes of data for this parameter, even if the actual string is only 8 bytes long? It might be that way, see NH-3401, and that would be unfortunate of course.

This is my recollection of the matter:

Parameter sizes must be set explicitly and not use the length of every individual value - i.e. it needs to use a small number of parameter sizes to encourage query plan caching in SQL Server.

In queries, the search pattern may be longer than the field length, due to wildcards. Such a pattern might still match valid values. Also, since the query string is often user entered, the user may enter a search pattern longer than the field length - such a pattern will not match anything, but it would be good if the application didn't need to handle an exception for such cases.

The comment from Prabu above suggests that correct (smaller) parameter lengths can have a noticeable effect on query performance, but Colum adds that it might be more related to type than size. It would be interesting to see more tests and analysis of why this is so.

The current behavior can be described as "round length up to max size". So perhaps the answer is to have more rounding steps? When the value is shorter than the field length, the field length can be used. If the value is larger, it can either use the max length, or perhaps round up to the nearest even 1000. A pull request with this would be useful.

Lauri Kotilainen added a comment — : I ran into a scenario where this has an actual, very noticeable impact. I filed a bug report here:

https://nhibernate.jira.com/browse/NH-3745

but it was closed because Alexander considered it an Azure issue. I disagree; I believe it's strongly linked to this one.

I also wrote a brief summary about the way I encountered the issue here:

http://blog.rytmis.net/2014/12/25/nhibernate-timeoutexception-with-azure-sql/

After writing that post, I invoked our company's Azure support contract and had a lengthy email exchange with some SQL Server support techs. Here's a verbatim quote from them:

"I tried to reproduce your behaviour in a private lab (on premise machine) and found the following:

Param size  IsMissingIndexImplemented?   Time (ms)
----------  --------------------------   ---------
1           no                            98
4000        no                            8389
1           yes                           60
4000        yes                           94

This confirms what you observed.
As you may also see, the index presence can remarkably reduce the execution time.

Conclusions
You should set the parameter type and size in your application code to match the table’s column type to avoid the implicit conversion.
With the information we have so far, we are not able to determine which exact differences are in place that let the query to take longer time on certain machines."

As you can see from the table, one can mitigate the issue with an index, and indeed, the index is probably the right thing to have anyway. However, this does highlight the aforementioned performance hit, and I believe that a more fine-grained approach would be better. Also, the tech's comments indicate that he reproduced the behavior on an on-premises SQL Server instance, so it's not Azure-specific.

eL-Prova added a comment — : Its a annoying bug. We have a char(1) in the database. When we put a index on it, NHibernate throws an exception! When running the generated query in management studio, it works without any problem.

We get the error: Creating or altering table 'FakeWorkTable' failed because the minimum row size would be 16017, including 4 bytes of internal overhead. This exceeds the maximum allowable table row size of 8094 bytes.

The query created is like: SELECT column FROM table WHERE something = @p1 AND (column2 = 'A' OR column2 = 'B')

We are using v3.3.3.4001

Gunnar Liljas added a comment — : IMO, this problem is much worse than the problem the causing change in NH-3036 fixed

Graham Bunce added a comment — : I'm getting this too in SQL Server - are there any plans to fix this (if it's deemed necessary?)

Gunnar Liljas added a comment — : Yes, but it's a tricky one to fix, without causing a regression on NH-3036. I will put some effort into it.

Graham Bunce added a comment — : Understood.

It affects linq too in that linq defaults to nvarchar params so you need to dump linq and use hql etc instead to force an ansistring param which is a bit awkward

Thanks anyway

Sent from my Windows PhoneFrom: <1>Gunnar Liljas (JIRA) Sent: ‎07/‎07/‎2016 08:51 To: <2>grahambunce@hotmail.com Subject: [JIRA] (NH-3403) Wrong parameter size in query with MsSql2000Dialect,MsSql2005Dialect and MsSql2008Dialect

<3>Gunnar Liljas commented on [4]NH-3403   [5]Re: Wrong parameter size in query with MsSql2000Dialect,MsSql2005Dialect and MsSql2008Dialect Yes, but it's a tricky one to fix, without causing a regression on [6]NH-3036 Closed . I will put some effort into it. [7] [8]Add Comment   This message was sent by Atlassian JIRA (v1000.140.0#100004-sha1:6c5f7c6) ---------------------------------------------------------------------------------------- <1> mailto:jira@nhibernate.jira.com [2] mailto:grahambunce@hotmail.com [3] https://nhibernate.jira.com/secure/ViewProfile.jspa?name=gliljas [4] https://nhibernate.jira.com/browse/NH-3403 [5] https://nhibernate.jira.com/browse/NH-3403 [6] https://nhibernate.jira.com/browse/NH-3036 [7] https://nhibernate.jira.com/browse/NH-3403#add-comment [8] https://nhibernate.jira.com/browse/NH-3403#add-comment --- **Gunnar Liljas** added a comment — : Pull request: https://github.com/nhibernate/nhibernate-core/pull/480 --- **Oskar Berggren** added a comment — : Silly github/jira integration. I made a commit *related* to this issue. Never claimed it fixed it. --- **Oskar Berggren** added a comment — : Difficult matter. The provided PR seems to need more work. Moving to 5.0.
fabiomaulo commented 6 years ago

I can remember this matter of some years ago and it was a hard work to know which is the property related to each parameter to then set the right parameter type with its size. sig

fabiomaulo commented 6 years ago

The situation is even worse since the parameter size is no set even using 

var q = session.CreateQuery("from Marca m where m.NickName = :pNickName")
    .SetParameter("pNickName", nickname, nickNameQueryType)
    .SetReadOnly(true);

with private static readonly NullableType nickNameQueryType = TypeFactory.GetAnsiStringType(50); produces this query

NHibernate: 
    select
        marca0_.MarcaID as Marca1_18_,
        marca0_.Nombre as Nombr2_18_,
        marca0_.NickName as NickN3_18_,
        marca0_.Nuevo as Nuevo4_18_,
        marca0_.Borrado as Borra5_18_,
        marca0_.NombrePlanFinanciacion as Nombr6_18_,
        marca0_.PaisID as PaisI7_18_,
        marca0_.CertificadoUsadosID as Certi8_18_,
        marca0_.Copete as Copet9_18_ 
    from
        RT_Marcas marca0_ 
    where
        marca0_.NickName=@p0;
    @p0 = 'fiat' [Type: AnsiString (8000:0:0)]
fabiomaulo commented 6 years ago

Well... just because was disabled de final part, the fix, for those who don't care about use LIKE with a string length as the length of the field, the solution is pretty simple:

    public class BackToTheFutureSqlClientDriver: Sql2008ClientDriver
    {
        protected override void InitializeParameter(IDbDataParameter dbParam, string name, SqlType sqlType)
        {
            base.InitializeParameter(dbParam, name, sqlType);
            if (sqlType.LengthDefined && !IsText(dbParam, sqlType) && !IsBlob(dbParam, sqlType))
            {
                dbParam.Size = sqlType.Length;
            }

            if (sqlType.PrecisionDefined)
            {
                dbParam.Precision = sqlType.Precision;
                dbParam.Scale = sqlType.Scale;
            }
        }
    }
gjmv commented 5 years ago

Hi, the same issue occurs in EnumCharType, and is not solved because inherits from AbstractEnumType (this solution aplies to AbstractStringType). I solved it locally in my proyect mixing this solution in custom EnumCharType. Also, i created an EnumAnsiCharType (same but fixed ansi).

I think be great put this solution in EnumCharType too.

Sorry my english is bad. Thanks

fredericDelaporte commented 5 years ago

Can you open a new issue for EnumCharType? Or directly a PR with tests if you can contribute your fix. About EnumAnsiCharType, it would have to be done as a separated issue or PR, since that is a new feature.