Fixes with prepare_sql #710

hazzik · 2017-10-09T00:55:59Z

prepare_sql requires all sizes/scales to be specified upfront.

~~This PR to be dropped if all tests are successful.~~

fredericDelaporte · 2017-10-09T07:13:23Z

It seems to demonstrate older issues, mainly with Time. That is for Odbc that some size specifications were removed. Not for SQL Client driver. Time was never sized.

hazzik · 2017-10-09T07:20:58Z

Ok, then we need to add these sizings

fredericDelaporte · 2017-10-09T07:22:55Z

Currently testing that, going to add a Jira. Any way good catch. Since this seems a SQL Client specific trouble, do we add this prepare_sql to the 2012 build by example? Testing 2008 without, 2012 with? Or do we duplicate all SQL Server builds, so long for the total build time?

fredericDelaporte · 2017-10-09T07:49:52Z

I have overwritten your merge with a rebase + a quick fix to check if there are other issues than time with SQL Server.

hazzik · 2017-10-09T07:56:45Z

src/NHibernate/Driver/SqlClientDriver.cs

@@ -144,6 +145,9 @@ protected static void SetDefaultParameterSize(DbParameter dbParam, SqlType sqlTy
 				case DbType.DateTimeOffset:
 					dbParam.Size = MaxDateTimeOffset;
 					break;
+				case DbType.Time:


Ideally, this should be in 2008 driver, as 2005 does not support time and reverts it to DateTime. ~~Same for DateTime2.~~

And same for DateTimeOffset. Defining defaults for those types supported only for 2008+ in the driver for previous versions is not very sound, excepted that the current pattern of sharing that method with SQL Server Ce, causing it to be static, causes it to be hard to be overridden in 2008 driver.

The reverting for DbType.Time and DbType.Date seems to be a specific "feature" of DbParameter interface implementation for SqlParameter, when setting types through DbType property, forcing us to set them through SqlDbType, and only into 2008 driver.

To clean-up that maybe we should cease sharing with SQL Server CE, switch to instance methods and override them. But it would be a breaking change for those using DateTime2 and DateTimeOffset with the old SQL Server driver.

Another option could be to check the dialect in SqlClientDriver for flagging whether we are using 2008+ or not, and if yes port and use corresponding code from SqlClient2008Driver. That way, no need to override anything anymore in SqlClient2008Driver, which would be then set as obsolete.

fredericDelaporte · 2017-10-09T08:40:56Z

ODBC, Firebird and PostgreSQL are failing due to to four tests, like NormalHqlShouldThrowUserException, which get another failure than the one expected with ODBC when prepare SQL is enabled.

fredericDelaporte · 2017-10-09T09:19:07Z

SQL Server CE fails with insufficient memory errors on blob types tests. It seems to be yet another issue.

fredericDelaporte · 2017-10-09T12:54:14Z

SQL Server CE fails with insufficient memory errors on blob types tests. It seems to be yet another issue.

This occurs only with 32 bits version. The 64 bits does not have the trouble. The root of the trouble is the default sizing of binary parameters to 2Gb when prepare_sql is enabled... This wrecks the 32 bits versions, as if it was trying to allocate such a size two or more times, hitting the 4Gb limit for 32 bits processes. The 64 bits surely does same but with my 16Gb setup, it does not get out of memory.
Disabling parameters sizing removes the trouble for SQL Server CE and let all tests work, although according to its documentation sizing parameters is mandatory when preparing SQL.

So we could fix this NH-4091 by ceasing specifying parameter sizes even when prepare_sql is enabled, which would, by the way, allow to switch this sizing method to an instance and override it for Sql 2008. Or we need to change it for not specifying blob sizes when called by SQL Server CE.

hazzik · 2017-10-09T21:40:08Z

src/NHibernate.Test/Async/NHSpecificTest/SqlConverterAndMultiQuery/Fixture.cs

 			// the test SqlConverter to do its job.
-			return !(Dialect is MsSqlCeDialect);
+			var prepareSql = PropertiesHelper.GetBoolean(Environment.PrepareSql, cfg.Properties, false);
+			return !(prepareSql || Dialect is MsSqlCeDialect);


Maybe it shall be only for SqlClientDriver?

return !(prepareSql && !(driver is SqlClientDriver) || Dialect is MsSqlCeDialect);? Or just return driver is SqlClientDriver?

I do not really get why it should be considered a Sql Server only test. For me it is more like this tests reveal there is some "too wide" catching in place causing the feature to be unavailable for most databases when prepare is enabled. Anyways, that is a quite secondary feature in my mind, and the code lacks indications as to why this "wide" catch is there, so I do not think we should investigate/fix this point, at least not for 5.0.

Ok, leave it as is

Or maybe more accurately, reveals that the test implementation (closing the connection) is not suitable for most databases when prepare is enabled. Maybe should it instead use a trick (like the nullable no null column in some transactions tests for failures) to alter the schema and use that to cause an actual db side failure.

Well, already changed, with comment corresponding to my last comment just above.

Need to regen the async

Done. Thanks for the reminder.

fredericDelaporte · 2017-10-09T22:19:24Z

src/NHibernate/Driver/SqlServerCeDriver.cs

-			if (prepareSql)
-			{
-				SqlClientDriver.SetVariableLengthParameterSize(dbParam, sqlType);
-		}


What do you think of this removal? It works but is in contradiction with documentation:

Before you call Prepare, specify the data type of each parameter in the statement to be prepared. For each parameter that has a variable-length data type, you must set the Size property to the maximum size needed. Prepare returns an error if these conditions are not met.

It is strange that it works.

I dislike it quite. But that is the simplest change to get it work... Otherwise I could move the code from SqlClientDriver into Sql CE driver and adjust it to its needs, meaning removing 2008 stuff and not specifying blob sizes, but letting all the other been set, since it will be more compliant with doc and should work too.

I don't think that SqlServer CE requires having all parameter sizes to be set when Prepare is used.

So we just keep that change as is. This PR could then be merged after dropping the first commit.

hazzik · 2017-10-10T01:11:57Z

src/NHibernate/Driver/Sql2008ClientDriver.cs


 namespace NHibernate.Driver
 {
 	public class Sql2008ClientDriver : SqlClientDriver
 	{
+		public const byte MaxTime = 5;


~~Why is this 5? It shall be either 7 or 8 (same as datetime2)~~. Ok, it is size in bytes, represents the (16,7) precision

Yes, the same for datetime2 and datetimeoffset, where it wants to knows their size instead of their scale.

About those MaxSizes, I have put it public for mimicking what was done for others. But well, I do not think there is need for that. Maybe time to clean-up a bit the driver by switching back to protected those parameter sizing members.

Although specifying parameters size is documented as mandatory when preparing statements, it appears it still works without.

hazzik · 2017-10-10T08:14:20Z

The first commit dropped.

hazzik requested a review from fredericDelaporte October 9, 2017 00:56

fredericDelaporte force-pushed the prepare_sql branch from 58e39af to 7eec6eb Compare October 9, 2017 07:47

hazzik commented Oct 9, 2017

View reviewed changes

fredericDelaporte force-pushed the prepare_sql branch from 6a49288 to 3a0a383 Compare October 9, 2017 13:25

hazzik commented Oct 9, 2017

View reviewed changes

fredericDelaporte reviewed Oct 9, 2017

View reviewed changes

hazzik commented Oct 10, 2017

View reviewed changes

fredericDelaporte force-pushed the prepare_sql branch 2 times, most recently from d566954 to 224f25f Compare October 10, 2017 07:53

hazzik changed the title ~~Set prepare_sql = true to test #703 changes~~ Fixes with prepare_sql Oct 10, 2017

fredericDelaporte added 3 commits October 10, 2017 21:10

NH-4091 - Cease specifying parameters size for avoiding out of memory

6655c2e

Although specifying parameters size is documented as mandatory when preparing statements, it appears it still works without.

NH-4090 - Set time parameter size for Sql Server

6cfb2a2

Restrict a test to Sql Server

5c01806

hazzik force-pushed the prepare_sql branch from 224f25f to 5c01806 Compare October 10, 2017 08:12

hazzik added this to the 5.0 milestone Oct 10, 2017

hazzik merged commit a33d88e into master Oct 10, 2017

fredericDelaporte deleted the prepare_sql branch October 10, 2017 09:02

fredericDelaporte added r: Fixed t: Bug labels Oct 10, 2017

fredericDelaporte mentioned this pull request Sep 13, 2018

Adjust string parameter sizes for MSSQL #1844

Merged

Fixes with prepare_sql #710

Fixes with prepare_sql #710

Uh oh!

Conversation

hazzik commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Oct 9, 2017

Uh oh!

hazzik commented Oct 9, 2017

Uh oh!

fredericDelaporte commented Oct 9, 2017

Uh oh!

fredericDelaporte commented Oct 9, 2017

Uh oh!

hazzik Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Oct 9, 2017 • edited by hazzik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Oct 10, 2017

Uh oh!

Uh oh!

hazzik commented Oct 9, 2017 •

edited

Loading

hazzik Oct 9, 2017 •

edited

Loading

fredericDelaporte Oct 9, 2017 •

edited

Loading

fredericDelaporte commented Oct 9, 2017 •

edited

Loading

fredericDelaporte commented Oct 9, 2017 •

edited

Loading

fredericDelaporte commented Oct 9, 2017 •

edited by hazzik

Loading

fredericDelaporte Oct 9, 2017 •

edited

Loading

hazzik Oct 10, 2017 •

edited

Loading