Sorry everyone but I was plain wrong!
The Firebird driver doesn't have any pooling issues related to our 
discussion here and there is no need to change any configurations!

The trouble is in the implementation of IncrementGenerator wich creates a 
brand new connection every time it needs to fetch the next value.
Examples of failing tests are EntityWithInverseManyToManyTest and 
EntityWithInverseOneToManyJoinTest. Both of those Fixtures needs to be run 
in one shot to see one of them fail!

I have opened a jira ticket https://nhibernate.jira.com/browse/NH-3591 and 
created a pull request 
https://github.com/nhibernate/nhibernate-core/pull/249

Now on my machine this lets the failing tests drops from 455 (after merging 
the last changes) down to 99.

Amro

On Friday, January 10, 2014 12:03:28 PM UTC+1, Amro El-Fakharany wrote:
>
> Done as discussed so far: 
> https://github.com/nhibernate/nhibernate-core/pull/246
>
> I also need to make a small change to the firebird configuration template 
> for the following reason (it is not included in the pull request yet):
> When there are more than one connection to a database firebird will regect 
> any Drop or Alter table statements from one connection if the table was/is 
> used by any of other connections.
> Now The Firebird driver has some issues with connection pooling in such it 
> sometimes opens a brand new connection instead of returning one from the 
> pool.
> This results in a lot of tests failing to drop the schema after they're 
> done. This in turn causes the following tests to fail because the fixture 
> setup raises an exception complaining about tables being already created.
> More than 200 tests are failing because of this!
>
> We have basically two* options:
> 1) Turn pooling off. This will make the tests run very, very slow but 
> should be relatively safe.
> 2) Force the driver to run under embedded mode. I don't know precisely 
> why, but the pooling issues disappear in this mode and the tests are still 
> running fast. The drawback ist that the driver may change this in furture 
> releases.
> Both of these options can be set in the driver configuration template and 
> we're done.
>
> - Which option would you guys choose? Personally I would go for the second 
> one.
> - Can I include it in the same pull request above?
>
> *There is actually a third option I'm thinking about which is to 
> contribute to the driver project and fix that annoying pooling issue but 
> I'm not sure if I can do that and even if I can it won't happen soon.
>
> Amro
>
> On Monday, December 23, 2013 11:23:27 AM UTC+1, Amro El-Fakharany wrote:
>>
>> Hi Everybody,
>> I noticed that there are some (actually a lot!) tests broken for Firebird 
>> and I decided to tackle each Problem one by one.
>>
>> So far I have found three Issues wich are (roughly description):
>> 1) Decimal data type: Firebird does'nt support precisions larger than 18 
>> and the default of nh is 19. 
>> I extended the FirebirdDialect to override the "GetTypeName" method and 
>> swap larger Decimal precisions with 18.
>>
>> 2) Parameters Within a select clause: Firebird needs a typecast if a 
>> parameter is used within a select clause.
>> I extended the FirebirdClientDriver to override the AdjustCommand method 
>> and typecast the parameter.
>>
>> 3) Temporary Tables: Firebird supports temporary Tables, which must be 
>> created in an isolated transaction, and so I thought it is just a matter of 
>> extending the dialect and overriding the related properties and methods.
>> Running the tests afterwards I discovered that this was'nt the case.
>>
>> After digging deeper in the code I think I now know what's going on and I 
>> think the issue is a "nasty" one and not necessarily related to Firebird.
>> This is going to be a rather long Description but please bear with me 
>> since I really need some guidance here!
>>
>> I'll start with one failing test which is found in 
>> *AbstractEntityWithOneToManyTests.OneToManyCollectionOptimisticLockingWithUpdate()*
>> .
>> The relevant Part is at the end of the test:
>>             s = OpenSession();
>>             *t = s.BeginTransaction();* <== keep this in mind.
>>
>>             c = s.CreateCriteria<Contract>().UniqueResult<Contract>();
>>             *s.CreateQuery("delete from Party").ExecuteUpdate();*
>>
>> An Exception is thrown at s.CreateQuery().ExecuteUpdate().
>>
>> Debuging and "Following the dots" you reach 
>> *MultiTableDeleteExecutor.Execute(parameters, 
>> session)* with:
>>             CoordinateSharedCacheCleanup(session);
>>             *CreateTemporaryTableIfNecessary(persister, session);*
>>
>> the implementation of CreateTemporarayTableIfNecessary() in 
>> AbstractStatementExecutor is:
>>             IIsolatedWork *work = new 
>> TmpIdTableCreationIsolatedWork(persister, log, session);*
>>             if (ShouldIsolateTemporaryTableDDL())
>>             {
>>                 *if 
>> (Factory.Settings.IsDataDefinitionInTransactionSupported)*
>>                 {
>>                     *Isolater.DoIsolatedWork(work, session);*
>>                 }
>>                 else
>>                 {
>>                     Isolater.DoNonTransactedWork(work, session);
>>                 }
>>             }
>>             else
>>             {
>>                 work.DoWork(session.ConnectionManager.GetConnection(), 
>> null);
>>                 session.ConnectionManager.AfterStatement();
>>             }
>>
>> The bold if Statement is absolutly useless! The Property 
>> IsDataDefinitionInTransactionSupported is never used (let alone set to 
>> true!) anywhere in the entire code base and hence it is always false.
>> I commented out the whole thing except *Isolator.DoIsolatedWork(work, 
>> session)* - because this is what firebird requires - just to see what 
>> will happen which was unfortunatly nothing. The test still fails with the 
>> same exception.
>>
>> Digging further starting from *Isolator.DoIsolatedWork(work, session) *you 
>> reach AdoNetTransactionFactory.ExecuteWorkInIsolation(ISessionImplementor 
>> session, IIsolatedWork work, bool transacted) with the relevant 
>> implementaion part of (i removed the comments for brevity):
>>                 if (session.Factory.Dialect is SQLiteDialect)
>>                     connection = session.Connection;
>>                 else
>>                     connection = 
>> session.Factory.ConnectionProvider.GetConnection();
>>
>>                 if (transacted)
>>                 {
>>                     trans = connection.BeginTransaction();
>>                 }
>>
>>                 work.DoWork(connection, trans);
>>
>>                 if (transacted)
>>                 {
>>                     trans.Commit();
>>                 }
>> Notice how It's asking for a new connection and starting a new 
>> transaction (wich is precisly what it should do) if requested. It then 
>> passes both to work.DoWork. In our case here the "work" is 
>> TmpIdTableCreationIsolatedWork.DoWork(*IDbConnection connection*, 
>> *IDbTransaction 
>> transaction*) with the relevant implementation:
>>                     stmnt = *session.ConnectionManager.CreateCommand()*;
>>                     stmnt.CommandText = persister.TemporaryIdTableDDL;
>>                     stmnt.ExecuteNonQuery();
>>                     
>> session.Factory.Settings.SqlStatementLogger.LogCommand(stmnt, 
>> FormatStyle.Ddl);
>>
>> The code is not only *ignoring* the passed in connection and transaction 
>> but is asking for a command from the ConnectionManager which will return a 
>> command assigned to a *different* connection from that passed in!
>>
>> Further more the ConnectionManager.CreateCommand() will assign the 
>> command the very first transaction created by the test and the whole thing 
>> blows up!
>> Here ist the implementation of ConnectionManager.CreateCommand():
>>             var result = *GetConnection().CreateCommand();*
>>             *Transaction.Enlist(result);*
>>             return result;
>>
>> Now I think the solution is actually simple, namly refactor the 
>> TmpIdTableCreationIsolatedWork.DoWork to use the passed in parameters, but 
>> I need to make sure that other dialects will not be affected.
>> (After all there must be a reason why the original author(s) ignored the 
>> passed in parameters right?)
>>
>> 1) Does any dialect support DDL statements within the same transaction as 
>> the one which actually usese them (such as selects or inserts) at all?
>> 2) If yes, will they be broken if we did the refactoring or do they also 
>> support DDL in separate transactions?
>> 3) Can we remove the *if 
>> (Factory.Settings.IsDataDefinitionInTransactionSupported) *and instead 
>> just stick with *Isolater.DoIsolatedWork(work, session);*?
>>
>> Sorry if the post was rather long and Merry Christmas to all of you.
>>
>

-- 

--- 
You received this message because you are subscribed to the Google Groups 
"nhibernate-development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

Reply via email to