Scoped table aliases to session factory configuration #1517

bahusoid · 2018-01-06T09:10:32Z

Make table aliases generated for same mapping but from different session factories the same:

To make sure that the same queries are executed for the same database from different apps (I described this scenario here)
To allow further code improvements (we can for instance start interning aliases/pre-generated queries for entities, as this data now should be the same for same mapping)
Fixes Table counter for aliases should be stable #1417.
~~Though tableCounter is still rise endlessly - but its value is not used for mapped tables stored in Configuration.tables.~~
It turns out that tableCounter is not really needed outside configuration (created table classes don't use UniqueInteger) - so it's been removed.

fredericDelaporte

I would rather:

Keep uniqueInteger readonly.
Add a constructor taking uniqueInteger as an argument.
Obsolete the parameter-less constructor.
Remove tableCounter.
Cease calling the parameter-less constructor from the constructor taking a table name. This constructor is only used in cases which do not need UniqueInteger.
Expose the Configuration.tables count as an internal property.
Use it as the base of temporary table indexes in the session factory constructor. At that point, all tables the session factory will use have been added to the Configuration, so there will be no index collision between temporary tables and other tables used by the session factory.

bahusoid · 2018-01-06T15:15:38Z

Keep uniqueInteger readonly.
Add a constructor taking uniqueInteger as an argument.

It was my first thought. But it's not very convenient to supply index beforehand. Plus this constructor now needs to be also exposed in DenormalizedTable. That's why I made this "shortcut". Though if you think it's a right way to do it - I will do it.

Remove tableCounter.

Wouldn't it be a breaking change? It's a public class and that changes behaviour of default constructor. It seems it's better to keep tableCounter and keep incrementing it to make obsolete constructor behaviour intact and remove it later along with ctor.

fredericDelaporte · 2018-01-06T15:39:40Z

Wouldn't it be a breaking change? It's a public class and that changes behaviour of default constructor. It seems it's better to keep tableCounter and keep incrementing it to make obsolete constructor behaviour intact and remove it later along with ctor.

Removing a private member is not a breaking change. Changing the uniqueness of values of a public property is a gray area, but this PR anyway already does this, since previously any new Table instance would have an unique value per creation thread, while with this PR it may reuse the value of another table from another configuration.
But keeping it as obsolete (both, the constructor and the member) is also valid, so as you wish.

But it's not very convenient to supply index beforehand.

There are only three cases where this index is used in NHibernate.csproj: AddTable, AddDenormalizedTable and PrepareTemporaryTables.

I do not see any difficulties neither for AddTable nor for AddDenormalizedTable. It looks to me as a simple re-ordering of operations in these methods.

The third case will require more work, adding a new overload to PrepareTemporaryTables taking an index, obsoleting the old PrepareTemporaryTables, exposing the Configuration.tables count as an internal property as written previously, ... But that does not look overwhelming to me.

The other usages of Table inside NHibernate.csproj are for schema generation. These usages do not use UniqueIndex (which is only used for GetAlias), they only use Table as a kind of name parsing helper.

Plus this constructor now needs to be also exposed in DenormalizedTable.

Yes, indeed, and the previous one obsoleted too. But that is a small inconvenience I think, allowing to have a consistent UniqueInteger computing, instead of mixing your new way and the old way.

There are also many testes in NHibernate.Test.csproj which will be impacted. I have not checked them, but I except them to either not really need that counter, or to be obsoleted by the way we change the counter logic, or to be not hard to fix for supplying a counter valid for the test.

bahusoid · 2018-01-06T15:51:23Z

I'm looking into PersistentClass.PrepareTemporaryTables:

nhibernate-core/src/NHibernate/Mapping/PersistentClass.cs

Lines 1121 to 1134 in d372ee3

    
           public void PrepareTemporaryTables(IMapping mapping, Dialect.Dialect dialect) 
        
           { 
        
           	if (dialect.SupportsTemporaryTables) 
        
           	{ 
        
           		temporaryIdTableName = dialect.GenerateTemporaryTableName(Table.Name); 
        
           		Table table = new Table(); 
        
           		table.Name = temporaryIdTableName; 
        
           		foreach (Column column in Table.PrimaryKey.ColumnIterator) 
        
           		{ 
        
           			table.AddColumn((Column)column.Clone()); 
        
           		} 
        
           		temporaryIdTableDDL = table.SqlTemporaryTableCreateString(dialect, mapping); 
        
           	} 
        
           }

And from what I see aliases here are irrelevant. Table class is not saved anywhere and only two strings are saved:
temporaryIdTableName which is generated using only table name
'temporaryIdTableDDL' which is a create temp table string SQL query. Something like:

create table #[TableName] ([PrimaryKeyID] INT not null)

So aliases are also not needed.

So it seems we don't really need to do anything for temp tables.

fredericDelaporte · 2018-01-06T15:58:43Z

Indeed, I have forgotten to thoroughly check this case. So there, why not using the constructor with table name overload, and forget about the index? It should be the simplest way to do the change.

bahusoid · 2018-01-06T16:03:42Z

@fredericDelaporte Some strange exceptions from Oracle. Don't you happen to know that it's something that might be not related to my changes? )
https://teamcity.jetbrains.com/viewLog.html?buildId=1282404&buildTypeId=bt1099#testNameId2417795984423602997

fredericDelaporte · 2018-01-06T16:09:09Z

I have seen them, but I do not see how they could relate to changes here. I can relaunch the build to check if they occur again. Or if you change this PR implementation, we can just wait for the new builds which will be run.

Update: they do not occur on another PR, so either this was a temporary trouble, or the changes in this PR cause issues to Oracle.

fredericDelaporte

It really looks like changing this counter causes issues to Oracle. Now we have to find out why.

fredericDelaporte · 2018-01-07T08:33:00Z

src/NHibernate/Mapping/Table.cs

@@ -194,6 +190,7 @@ public string Schema
 		public int UniqueInteger
 		{
 			get { return uniqueInteger; }
+			internal set { uniqueInteger = value; }


Since this member now depends on being externally supplied (be it by setter or constructor), maybe should we guard against it being used without having been set.
I would do it by switching the backing field to nullable then adjusting the getter to something like get => uniqueInteger ?? throw new InvalidOperation("UniqueInteger has not been supplied");. (If using expression bodied getter, change also the setter for consistency.)

@fredericDelaporte Good idea. Will do it. And before making any changes in ctor... Are you sure about it? ) I see no troubles with this change but I just don't see why to fight to keep uniqueInteger readonly. Considering that it's the only read-only field in this class.

Forget about the constructor. I mentioned it just to tell that if this member value was supplied through a constructor, I would still consider doing this check in the getter.

bahusoid · 2018-01-07T09:20:51Z

It really looks like changing this counter causes issues to Oracle. Now we have to find out why.

Yep. What's more interesting - from what I see exception is thrown not from test. It's thrown from test clean up code:
at NHibernate.Test.TestCase.CheckDatabaseWasCleaned() in ..
at NHibernate.Test.TestCase.TearDown() in ...

fredericDelaporte · 2018-01-07T10:33:22Z

Running NH-1362 test in isolation succeeds. Running it along with NH-1301 test fails. It looks like Oracle caches returned types for a query then fails re-running the same query when the underlying table definition has changed.

Clearing the connection pool avoids the trouble. So we have to use a trick similar to the one put in place for Firebird, see TestCase.DropSchema. In the case of Oracle, the clear should likely be put after the schema has been dropped.

Update: OracleConnection doc for clearing pool. Will need to check if the un-managed driver has similar methods. It looks like the Microsoft version have them. We have drivers in NHibernate for all of them, so doing this will require work for all of them (maybe doable once in the common NHibernate Oracle driver base).
Update 2: the doc linked above is for Oracle managed and un-managed, so these methods exist on both.

fredericDelaporte · 2018-01-07T17:17:36Z

src/NHibernate.Test/TestCase.cs

+			// So exception can be thrown if two tests execute same query but with different types in result (like for Entity.Id int and Entity.Id Guid)
+			if (Dialect is Oracle8iDialect)
+			{
+				OracleConnection.ClearAllPools();


This will work only if the Oracle driver is the managed one. But two other Oracle drivers are usable. (Currently their specific TeamCity builds are not enabled.)

I have made a PR on your branch for handling them all: bahusoid#1.

Thanks. I've adjusted it per @hazzik comments

fredericDelaporte · 2018-01-08T12:46:20Z

Now it succeeds but the Oracle build time seems to have raised from twenty minutes to fifty... It is too near to the one hour timeout.

For checking it twice, I have relaunched another Oracle build for the latest commit of this PR.

If it takes fifty minutes again, we will have to devise another solution.

Moving the pool clear into DropSchema, at its end?
It would avoid clearing the pool on tests which have been ignored before even creating the schema, on which case there is no reason anything could have been cached with connections. But I do not expect this to gain a lot of time.
Putting the pool clear in the AppliesTo(ISessionFactoryImplementor factory) of tests we see failing?
It will allow clearing the pool far less, but it will not handle new tests, and it may be a bit iterative process: clearing the pool allowing caching new query schema causing other following tests to fail.
~~A try catch in the CheckDatabaseWasCleaned, catching the ORA-00932, clearing the pool in such case and trying again (with some logic for avoiding an endless retry loop)?~~
It would still be "generic", able of handling new test encountering the trouble, while avoiding clearing the pool for each fixture. But this ORA-00932 trouble could also occur in the test itself as shown by NH-1716 failure here, not only in its cleanup checking. So this solution will not work.
Or maybe a combination of 3. then 2. for remaining failing tests.
It will clear the less possible while reducing cases needing to be handled specifically.

Or other idea?

fredericDelaporte · 2018-01-08T13:21:56Z

The second Oracle run has taken fifty minutes again, we need another solution.

fredericDelaporte

Your last solution works great, 21 minutes. Now previous attempts need to be cleaned-up, and some guidance should be added for developer willing to run the tests locally. See review comments.

fredericDelaporte · 2018-01-10T09:32:52Z

src/NHibernate.Test/TestCase.cs

@@ -14,6 +14,8 @@
 using NUnit.Framework.Interfaces;
 using System.Text;
 using NHibernate.Driver;
+using NHibernate.SqlCommand;


Changes to this file should be reverted, since they were done for the "clear pool" solution.

fredericDelaporte · 2018-01-10T09:46:13Z

src/NHibernate.sln.DotSettings

@@ -1,13 +1,16 @@
 <wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">
-	<s:String x:Key="/Default/CodeStyle/CodeFormatting/CommonFormatter/ALIGNMENT_TAB_FILL_STYLE/@EntryValue">USE_SPACES</s:String>
+


Better revert the DotSettings update for now. The alignement tab fill style seems removed while we had explicitly set it.

fredericDelaporte · 2018-01-10T09:47:28Z

src/NHibernate/Driver/FirebirdClientDriver.cs

@@ -1,9 +1,7 @@
-using System;
 using System.Collections.Generic;
 using System.Data;


Changes to this file should be reverted, since they were done for the "clear pool" solution.

fredericDelaporte · 2018-01-10T09:47:37Z

src/NHibernate/Driver/OracleClientDriver.cs

@@ -59,5 +59,17 @@ protected override void OnBeforePrepare(DbCommand command)
 				" does not support CallableStatement syntax (stored procedures)." +
 				" Consider using OracleDataClientDriver instead.");
 		}
+


Changes to this file should be reverted, since they were done for the "clear pool" solution.

src/NHibernate/Driver/OracleDataClientDriver.cs

fredericDelaporte · 2018-01-10T09:48:02Z

src/NHibernate/Driver/OracleDataClientDriverBase.cs

@@ -2,7 +2,6 @@
 using System.Collections.Generic;
 using System.Data;
 using System.Data.Common;
-using System.Reflection;


Changes to this file should be reverted, since they were done for the "clear pool" solution.

fredericDelaporte · 2018-01-10T09:48:13Z

src/NHibernate/Driver/ReflectionBasedDriver.cs

@@ -70,5 +72,69 @@ public override DbCommand CreateCommand()
 		{
 			return connectionCommandProvider.CreateCommand();
 		}
+


Changes to this file should be reverted, since they were done for the "clear pool" solution.

fredericDelaporte · 2018-01-10T09:49:55Z

src/NHibernate/Mapping/PersistentClass.cs

@@ -1123,8 +1123,7 @@ public void PrepareTemporaryTables(IMapping mapping, Dialect.Dialect dialect)
 			if (dialect.SupportsTemporaryTables)
 			{
 				temporaryIdTableName = dialect.GenerateTemporaryTableName(Table.Name);
-				Table table = new Table();
-				table.Name = temporaryIdTableName;
+				Table table = new Table(temporaryIdTableName);


This change is not necessary, since the parameter-less constructor is not obsoleted for one taking the counter.

fredericDelaporte · 2018-01-10T09:56:45Z

teamcity.build

@@ -116,15 +116,15 @@
 	<target name="setup-teamcity-oracle">
 		<property name="nhibernate.connection.driver_class"			value="NHibernate.Driver.OracleClientDriver" />
 		<property name="nhibernate.dialect"							value="NHibernate.Dialect.Oracle10gDialect" />
-		<property name="nhibernate.connection.connection_string"	value="User ID=nhibernate;Password=nhibernate;Data Source=XE" />
+		<property name="nhibernate.connection.connection_string"	value="User ID=nhibernate;Password=nhibernate;Data Source=XE;Metadata Pooling=false;Self Tuning=false;" />


These parameters are ODP.NET specific, they are not supported by System.Data.Oracle (which is used by OracleClientDriver). Do not add them to this build. Anyway System.Data.Oracle is obsolete since long and it is unlikely we will test it some day again.

fredericDelaporte · 2018-01-10T10:10:55Z

teamcity.build

 		<property name="NHibernate.Test.IgnoreFail" value="true" />
 		<property name="teamcity.last.result" value="${root.dir}/lib/teamcity/oracle/NHibernate.Test.last-results.xml" />
 	</target>

 	<target name="setup-teamcity-oracle32">
 		<property name="nhibernate.connection.driver_class"			value="NHibernate.Driver.OracleDataClientDriver" />
 		<property name="nhibernate.dialect"							value="NHibernate.Dialect.Oracle10gDialect" />
-		<property name="nhibernate.connection.connection_string"	value="User ID=nhibernate;Password=nhibernate;Data Source=(DESCRIPTION = (ADDRESS = (PROTOCOL = TCP)(HOST = localhost)(PORT = 1521)) (CONNECT_DATA = (SERVER = DEDICATED) (SERVICE_NAME = XE)))" />
+		<property name="nhibernate.connection.connection_string"	value="User ID=nhibernate;Password=nhibernate;Metadata Pooling=false;Self Tuning=false;Data Source=(DESCRIPTION = (ADDRESS = (PROTOCOL = TCP)(HOST = localhost)(PORT = 1521)) (CONNECT_DATA = (SERVER = DEDICATED) (SERVICE_NAME = XE)))" />


Good finding with these parameters, it works way better than clearing the pool. Can you please update lib\teamcity\oracle\oracle_installation.txt too? It should tell about these parameters for helping developers having the tests pass on their box. (But it cannot be put in the config template, since this one is also distributed with NHibernate as a config sample, and that disabling these parameters makes sens only for the tests, not for a normal application.)

Something like the following could be added toward the end of oracle_installation.txt, just above its last line I think:

Adjust the connection string for the tests: The tests involve creating and dropping many tables, sometimes with the same names but different data types. This does not play well with Oracle meta data pooling, which needs to be disabled. Add into your ODP.NET connection string: Metadata Pooling=false;Self Tuning=false;

fredericDelaporte · 2018-01-10T13:50:21Z

I have pushed the cleanup and my suggestion about Oracle instructions.

bahusoid · 2018-01-10T13:51:27Z

Thanks for you changes. I've just come back to start working on your comments :)

bahusoid · 2018-01-10T13:58:49Z

I see there is still some leftovers like (dotSettings file). I will handle it

fredericDelaporte · 2018-01-10T14:15:44Z

I got trapped by the "end of file normalization" due to editor setting. You will need to revert the end file last line with an editor which does not honor .editorconfig.

bahusoid · 2018-01-10T14:39:05Z

@fredericDelaporte Done

fredericDelaporte · 2018-01-11T10:27:22Z

Let me play a little bit with stable table aliases

What do you mean by this? Have you some additional changes to do here, or can this be merged?

bahusoid · 2018-01-11T10:30:58Z

@fredericDelaporte This PR is ready. I meant play with interning aliases and related stuff. It's all about another PR #1503 where I made this comment in response to #1503 (comment).

fredericDelaporte reviewed Jan 6, 2018

View reviewed changes

fredericDelaporte reviewed Jan 7, 2018

View reviewed changes

bahusoid force-pushed the tableAlias branch 2 times, most recently from 5721fd0 to 7694de0 Compare January 7, 2018 14:20

fredericDelaporte reviewed Jan 7, 2018

View reviewed changes

bahusoid force-pushed the tableAlias branch 3 times, most recently from 070d735 to cedaecb Compare January 10, 2018 06:33

fredericDelaporte reviewed Jan 10, 2018

View reviewed changes

Scoped table.UniqueInteger to session factory configuration

dab000d

bahusoid and others added 5 commits January 10, 2018 19:58

Removed tableCounter with tests

2108e49

guarded against UniqueInteger undefined usage

0c29314

Fixed test

15edb12

Disable metadata pooling

0497814

Completed Oracle instructions.

187560d

bahusoid force-pushed the tableAlias branch from 8229837 to 187560d Compare January 10, 2018 14:37

fredericDelaporte approved these changes Jan 10, 2018

View reviewed changes

fredericDelaporte self-assigned this Jan 10, 2018

fredericDelaporte added the t: Fix label Jan 10, 2018

fredericDelaporte added this to the 5.1 milestone Jan 10, 2018

bahusoid mentioned this pull request Jan 11, 2018

[WIP] Intern string metadata in Session Factory #1503

Closed

fredericDelaporte merged commit 690e96f into nhibernate:master Jan 11, 2018

fredericDelaporte added the r: Fixed label Jan 11, 2018

This was referenced Jul 12, 2021

Add Oracle to GitHub Actions #2848

Merged

Investigate why TeamCity Oracle builds so slow #2854

Closed

		@@ -1,13 +1,16 @@
		<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">
		<s:String x:Key="/Default/CodeStyle/CodeFormatting/CommonFormatter/ALIGNMENT_TAB_FILL_STYLE/@EntryValue">USE_SPACES</s:String>

Scoped table aliases to session factory configuration #1517

Scoped table aliases to session factory configuration #1517

Uh oh!

Conversation

bahusoid commented Jan 6, 2018 • edited by fredericDelaporte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

bahusoid commented Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Jan 6, 2018

Uh oh!

bahusoid commented Jan 6, 2018

Uh oh!

fredericDelaporte commented Jan 6, 2018

Uh oh!

bahusoid commented Jan 6, 2018

Uh oh!

fredericDelaporte commented Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte left a comment

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!

bahusoid commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Jan 7, 2018 • edited 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!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Jan 8, 2018

Uh oh!

fredericDelaporte commented Jan 8, 2018

Uh oh!

fredericDelaporte left a comment

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!

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!

fredericDelaporte Jan 10, 2018 • 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 Jan 10, 2018 • 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 commented Jan 10, 2018

Uh oh!

bahusoid commented Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bahusoid commented Jan 10, 2018

Uh oh!

bahusoid commented Jan 6, 2018 •

edited by fredericDelaporte

Loading

bahusoid commented Jan 6, 2018 •

edited

Loading

fredericDelaporte commented Jan 6, 2018 •

edited

Loading

bahusoid commented Jan 7, 2018 •

edited

Loading

fredericDelaporte commented Jan 7, 2018 •

edited

Loading

fredericDelaporte Jan 10, 2018 •

edited

Loading

fredericDelaporte Jan 10, 2018 •

edited

Loading

bahusoid commented Jan 10, 2018 •

edited

Loading

bahusoid commented Jan 11, 2018 •

edited

Loading