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

[gliljas]

Modified Restrictions.cs to always use LikeExpression for Like, so that the parameters will not have a specified size. LikeExpression should attempt to respect the string type used.

[gliljas]

This changes some return types in Restrictions.cs. In fact it changes them so that they follow what's specified in the XML documentation.

[gliljas]

Clearly, I need to fix some other things as well.

[gliljas]

The remaining problem is triggered when using SetParameter instead of SetString, which leaves it up to NHibernate to find the type to use. It automatically sets the type of the right hand parameter to the type of the left hand node (or vice versa). The only feasible remedy I can find is to intercept this behavior in BinaryLogicOperatorNode.cs.

[hazzik]

Please revert this file

[hazzik]

@gliljas this fix only for Criteria/QueryOver, right?

[gliljas]

@hazzik No, it's for HQL and Linq too.

[bunceg]

Hi, Just to make sure - this fixes the problem whereby where a table has varchar(n) fields but you use linq to select on these, it defaults to nvarchar(nnnn) when generating the SQL?

This then means the index usage is completely inefficient (SQL Server), so to work around this you have to fall back to using HQL/Criteria/QueryOver where you can SetAnsiString as a query parameter thereby forcing the SQL generated to correctly use varchar(nnnn), then using efficient indexing instead.

I believe that this makes Linq pointless for SQL Server varchar column selects so it's a reasonably important patch.... we only realised this bug last week when running some performance tests on our queries :(

[oskarb]

Here formatting is messy. Please use tab for indenting and SPACE for alignment to avoid it breaking with different tab widths.

[oskarb]

criteria.GetType()

If I read that correctly, it will always say "Type mismatch in NHibernate.Impl.CriteriaImpl:..." (or "NHibernate.Impl.CriteriaImpl.SubCriteria"), since those are the only two implementations of ICriteria? If so it seems like a non-helpful implementation detail and might just say "... in criteria:..." instead.

[oskarb]

There was some discussion quite a while ago on the possibility of rounding the parameter size up to certain fixed sizes (e.g. 10, 100, 500, 1000, etc) to avoid always going directly to 8000. Am I right in thinking that if the current PR works out, the "levels of fixed parameter sizes" won't be needed?

[bunceg]

In my tests so far, it appears that varchar(8000) is acceptable in SQL Server. The problem is where nvarchar(8000) is used as a parameter against a varchar(n) column, the index seek becomes very inefficient. We were using this in SQL Azure and blowing our performance throttle instantly (99.98% of capacity) - when we patched the app to use custom queries instead of Linq, we were down to about 20% of capacity, without any other changes needed.

There may be other efficiences that can be gained (we haven't analysed it throughly enough), but IMO, the fix to use varchar(8000) instead of nvarchar(8000) for a varchar(n) column makes enough of a difference to release this seperately to any other patch that may be appropriate.

[oskarb]

How about inverting this ìf` so one return statement is enough?

[gliljas]

@bunceg

Yes, it means just that, although right now, the problem still occurs when using a LIKE search in Linq (only interesting for StartsWith). Working on that. It works just fine for equality now. A unit test confirms that.

It could have been enough to force varchar(8000) for "ansistring" properties, but the chosen approach with specific sizes felt a bit more explicit. The disadvantage (or advantage) is that errors will be thrown when too long values are used. Opinions are welcome.

Should we deal with CHAR and NCHAR too?

[oskarb]

Double negation, which I think is unintentional. However, intentional or not this isn't strictly true, is it? Looks to me like the parameter type IS controlled by the other operand, except that the length aspect (only) is ignored? So this could be written less confusingly.

Also, I don't quite like the style (with regards to the new if-statement in Initialize()) of skipping a large block of code if another large block of code that does almost the same thing happens to return true. Hmm... I think what I mean is that as currently written it looks "unbalanced".

Instead, would it work to have something like SetExpectedType(expectedTypeAwareNode, otherOperand, otherOperandType), and have that replace just the lshExpectedTypeAwareNode.ExpectedType = rhsType; line (and one more of course)? (I haven't fully analyzed this option.)

[gliljas]

I agree, the language was confusing.

I did entertain exactly what you suggested, with a SetExpectedType method, but didn't want to modify the interface just yet.

[oskarb]

I don't understand what you mean with modifying the interface - the SetExpectedType() I proposed was intended as a private method only called from Initialize(). Though it could of course be protected and overriden if we want to do a LikeOperatorNode as a subclass, but I don't think we need a subclass just for this.

[gliljas]

Ok, I misunderstood your idea.

[bunceg]

@gliljas

Should we deal with CHAR and NCHAR too?

I haven't explictly checked the access paths for these, but I would expect it to have the same issue

[oskarb]

With regards to the return types in Restrictions, oddly enough we have a
commit from 2008-03-07:
"* The Restrictions factory now return an AbstractCriterion for all expressions."
The oddity is that it actually changed only a few of the methods and I have
no idea why the discrepancy.

Sadly, since not all methods were changed back then, this change is now API
breaking, and I don't think we can accept it as is for 4.1. Perhaps the
regular .Add(Restrictions...) won't break, but some potential wrapper
method typed as SimpleExpression certainly will.

Perhaps we can work around it... but deciding on this particular detail can wait until the other issues with the patch have been worked out, to see where that leads us.

[gliljas]

Perverting SimpleExpression seems like it's the way to go then.

[gliljas]

OK, I managed to get the Linq StartsWith to work too, but couldn't find a better place to handle the type fiddling than the BinaryLogicOperatorNode. Again.

It basically required a test to see whether one side of a LIKE was a Concat method, and then set the type of all "untyped" parameters to the type of the other side of the LIKE.

While it doesn't feel quite right to be mucking around in BinaryLogicOperatorNode, it certainly does the trick. It would actually feel a lot nicer to have an HqlWalkerNodeTreeVisitor, akin to an ExpressionTreeVisitor, that could be derived from and used for specific purposes, like fixing unknown types. Or do you have any other suggestions?

Also, maybe we should define the goal? Is it to fix specific issues for MSSQL or is it to create query trees or criteria that faithfully represent the intention of the query? It's clear that almost none of the drivers/DBMS:es care about parameter sizes (or maybe they do, for optimization purposes?), but right now they actually get data (in SqlType) about the intended size, but just ignore it. That's fine, but the data is actually wrong. An Equals query will get an SqlType with the size of the mapped column. If that's not what we want, we shouldn't rely on the driver to ignore it.

Suggestions:

1. Remove size data where it should be ignored.
2. Ensure that size data is available and correct when it should be used.
3. Fixed strings (CHAR and NCHAR) must be handled properly (NH-3745)

[oskarb]

Gunnar, you didn't publish anything regarding StartsWith did you?

There is a TODO in BinaryLogicOperatorNode.MutateRowValueConstructorSyntaxesIfNecessary() indicating that we may not yet know the proper types... perhaps that is also a hint that a more general solution would be useful.

See also my post in nhdev.

[gliljas]

No, the StartWith code was not "push worthy". Just a test of the idea and I started to realise that maybe we should discuss further before I spend time refactoring.

[oskarb]

This all got very confusing. Trying to decipher exactly what doesn't work under what circumstances.

@bunceg Claims that "NH linq defaults to nvarchar" which causes problems when the column is varchar. But it seems BinaryLogicOperatorNode already gets the type right, it is just the length that's wrong (but in released versions either ignored or maximized by the driver). So is it in fact just for StartsWith/EndsWith/Contains the problem with wrong type occurs? I did find https://nhibernate.jira.com/browse/NH-3565 (StartsWith / EndsWith / Contains don't use correct AnsiString type) on this matter.

[gliljas]

Yes, that is by far the most crucial issue

2016-07-31 21:20 GMT+02:00 Oskar Berggren notifications@github.com:

This all got very confusing. Trying to decipher exactly what doesn't work
under what circumstances.

@bunceg https://github.com/bunceg Claims that "NH linq defaults to
nvarchar" which causes problems when the column is varchar. But it seems
BinaryLogicOperatorNode already gets the type right, it is just the length
that's wrong (but in released versions either ignored or maximized by the
driver). So is it in fact just for StartsWith/EndsWith/Contains the problem
with wrong type occurs? I did find
https://nhibernate.jira.com/browse/NH-3565 (StartsWith / EndsWith /
Contains don't use correct AnsiString type) on this matter.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#480 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARt9p68N3sAZUNjDw3b2HDefL8xMBIuks5qbPVlgaJpZM4JKO0_
.

[oskarb]

I've updated the additional tests in PR #481 to resolve/ignore some failures. Only MySQL have a relevant failure now, because it refuses to prevent truncation. Not sure what that is about - documentation suggests default behaviour in modern version is to raise an error if truncation would occur.

[bunceg]

@oskarb. Hi, your comment is a little confusing - is the issue with "equals" statements fixed, but not released, or do you think it's just with LIKE and IN statements and equals is fine?

I can confirm that in my testing, with the release version 4.0.4.4000 that "equals" statements have the issue with Linq. We had to replace this (e.g.):

var x = Session.Query<XYZClass>().SingleOrDefault(x => x.Name == _v1 && x.Value2 == _v2)

with Hql (in our case, we don't use QueryOver that much) and SetAnsiString parameters on the query (and returning a UniqueResult). Name and Value2 are varchar(n) fields, not nvarchar(n).

SELECT ox FROM XYZClass ox WHERE ox.Name= :v1 AND ox.Value2 = :v2

This gave us significantly better performance. It's really noticeable when using SQL Azure because you hit your throttle limits very quickly with Linq but hardly go above ~2% of the limit when using Hql with the same load.

[oskarb]

@bunceg Oh yes, this is easy to be confused by. I was referring to what it looks like it should do in the released NH 4.0. There is code in there that attempts to set the type of the parameter based on the type of the other operand, for binary comparison operators. It doesn't work in all cases, but it looks like it should work for simple things like column == stringVariable. I'll try to test a bit later.

[pauljame]

Hey, That's a very important issue in my project. I cant even use NH anymore in several linq queries with Contains .
I'm using now MappedAs in Linq query to change parameter from nvarchar to varchar. But now I need set the length. How can I do that? I am willing to compile my own version of NH to "fix" that.

[hazzik]

@pauljame like this: x.MappedAs(NHibernate.Type.TypeFactory.Basic("AnsiString(200)"))

[pauljame]

@hazzik Thanks! It worked great...
Its seems strange to need to do that to get your query working, but its working fine....

[pauljame]

@hazzik Actually it not worked with 4.1.1 version... I was testing with my local custom NH version, where I uncommented the code from SqlClientDriver.cs

if (sqlType.LengthDefined && !IsText(dbParam, sqlType) && !IsBlob(dbParam, sqlType))
{
	dbParam.Size = sqlType.Length;
}

[sentientpc]

FYI using NH with MS SQL Server Always Encrypted columns causes issues when command parameters are not sized exactly.

Example error:
SqlException: Operand type clash: varchar(8000) encrypted with (encryption_type = 'DETERMINISTIC', encryption_algorithm_name = 'AEAD_AES_256_CBC_HMAC_SHA_256', column_encryption_key_name = '...', column_encryption_key_database_name = '...') collation_name = 'SQL_Latin1_General_CP1_CI_AS' is incompatible with varchar(200) encrypted with (encryption_type = 'DETERMINISTIC', encryption_algorithm_name = 'AEAD_AES_256_CBC_HMAC_SHA_256', column_encryption_key_name = '...', column_encryption_key_database_name = '...') collation_name = 'Latin1_General_BIN2' Statement(s) could not be prepared.

Uncommenting the code in SqlClientDriver.cs resolves the SQL AE issue, but causes other issues as discussed in this thread.

if (sqlType.LengthDefined && !IsText(dbParam, sqlType) && !IsBlob(dbParam, sqlType))
{
	dbParam.Size = sqlType.Length;
}

[gliljas]

Replaced by #1844