Separates the PrepareStatement functionality from executeStatement.

Also separate functionality to bind idenity, new values and old values to the prepared statement in separate methods.

For database enigines that do not support binding of null values in where conditions the update statement is modified at the moment. Can you please outline how we handle these cases prior of starting with this subtask.

This patch involved massive rafactoring. I would try to summarize the main rafactoring things here.

There are numerous ways in which we can refactor this class. The code is bit complex and big. I followed the following design.

The previous executeStatement has been restructured and modular based on the functionality implemented in that. The new method that are included in this patch are:

bindData
bindIdentity
bindNewEntity
bindOldEntity
executeUpdate
executeQuery
processData
prepareStatement
closeStatement
executeStatement

As you can see, each function name is self explainatory about the functionality that is contained with in them. The identity size check has been shifted to bindIdentity method for the time being but in the next subtast this will be removed from this class. Similarly the code for extended classes is still there and this will be removed in the next subtasks.

Comments ??

As far as null values are concerned. This is an issue of compatibility with different db engines. I can imagine two solutions to that

1) We should follow one base db engine dialect and construct our queries according to that. And then at the very last step of query construction we check on which db this query is going to execute. According to the type of db engine, we should mold our query.

2) The second approach which can be possible is to have separate classes from different database engines.

I think it should be better to touch these things while doing new implementation according to new Class Heirarchy concept.

Regards, Ahmad

Executing old test framework I recognized that we have introduced a race condition as TC06 failed. To fix this I had to synchronize the executeStatement() method. To prvent problems I also synchronized the executeStatement() method of SQLStatementRemove.

Over all the patch makes SQLSTatementStore much easier to understand even if it is not ideal yet. I therefore commit the patch for the time being with the synchronization explained above.

With regard to the binding of null values in where clauses I think 2 second approach is much better. I would prefer that you create a new subtask of castor-2713 for splitting SQLStatementStore in 2 implementations where 1 simply binds null and the other creates a new sql statement at every execution. In my opinion this should be resolved before starting with the sql generator class hierarchy.

Ralf, Ii am just trying to follow a bit whilst launching a commercial product elsewhere, so time is limited right now. Still I'd like to understand as to why synchronization had to be introduced. What is the problem at stakes that needs to be addressed through such a statement ?

Let me just give you an example why I consider this very dangerous: the synchronized statement around the executeStatement() method basically prevents any thread from entering and executing that method whilst any other thread tries to execute a prepared statement. If there's a statement that does take a long time to execute (for whatever reason, be it e.g. a race condition, some lock resolution taking place, ....) this will reduce concurrency to almost nothing.

I just had a short peek at the source code and whilst I have not followed all the refactorings completely, introducing such synchronization does not feel good.

I agree that the synchronization reduces concurrency and that we finally have to remove it once again. The change that requires this synchronization is, that we made the prepared statement and some other things class properties. If you like to know what happens, remove the synchronization and run TC06 of the old test suite.

Thanks Werner for highlighting the issue. We have to make it thread-safe in future refactoring as mentioned by Ralf. The thread-safety can be achieved in either of the following ways

1)Synchronized methods
2) Synchronized blocks
3) Method Attributes rather than Class attributes.

The last method of achieving thread-safety is the best one in the sense that it will not be a hurdle in achieving concurrency. On the otherhand, the Synchronization will only allow at most one thread to execute the code at any particular time.

Regards, Ahmad

No problem. I am just trying to follow as much as I can. Quite honestly, I'd opt for option c) in this context, especially when taking prepared statement pooling into consideration. But let's have this discussion once we have to have it.

Which kind of makes me ask the next question: I was thinking about shipping an 1.3.1 release within the next few weeks, as I think we should try to make the regression changes available. How does this impact your work here ?

I also prefer method attributes but I first have to look at the impacts on design where I have not taken concurrency into account earlier.

I also think shipping a 1.3.1 release soon would be a good idea. I think within a few weeks we are able to finish the reorganization of delete and update statements. Thereafter we try to take even more care not to introduce new issues.

Re: 1.3.1: great. I am facing a launch currently, so it won't be before next week that I will be able to return my attention to Castor.

Can I ask you a small question: who's using classes like SqlStatementStore et alis at run-time. I guess there will be instances of each of those per Java class that has a JDO mapping. Correct ? If that's the case, I do not think that we want to synchronize at all. There's absolutely no reason as to why deleting an instance of class A should have any relation to deleting an instance of class B. And the same applies to two instances of class A. At any time, all these operations should be possible in parallel.