Bug 338783

Comment 1 Adrian Goerler 2011-03-03 08:54:02 EST

Created attachment 190269 [details]
proposed patch

Tested with Oracle10Platform: 
- WDF: OK
- JPA SRG: OK
- Core SRG: OK

Comment 2 Andrei Ilitchev 2011-03-07 15:44:50 EST

I talked to James about this bug and it appears to be bigger than it seems.

Related bugs: 
Bug 266151 - Delete queries return invalid values for executeUpdate when batching is enabled;
Bug 328714 - support batch writing with optimistic locking.

Background:

In DatabaseAccessor.basicExecuteCall when batch writing is enabled the decision is made whether to add the call to the batch or to execute the previously batched calls before executing the call:

if (isInBatchWritingMode(session)) {
    // if there is nothing returned and we are not using optimistic locking then batch
    //if it is a StoredProcedure with in/out or out parameters then do not batch
    //logic may be weird but we must not batch if we are not using JDBC batchwriting and we have parameters
    // we may want to refactor this some day
    if (dbCall.isNothingReturned() && (!dbCall.hasOptimisticLock() || getPlatform().canBatchWriteWithOptimisticLocking(dbCall) ) 
        && (!dbCall.shouldBuildOutputRow()) && (getPlatform().usesJDBCBatchWriting() || (!dbCall.hasParameters())) && (!dbCall.isLOBLocatorNeeded())) {
        // this will handle executing batched statements, or switching mechanisms if required
        getActiveBatchWritingMechanism().appendCall(session, dbCall);
        //bug 4241441: passing 1 back to avoid optimistic lock exceptions since there   
        // is no way to know if it succeeded on the DB at this point.
        return Integer.valueOf(1);
    } else {
        getActiveBatchWritingMechanism().executeBatchedStatements(session);
    }
}


Problem 1.
DeleteAllQuery doesn't support optimistic locking, therefore
DeleteAllQuery should not set hasOptimisticLock flag to true (see CallQueryMechanism.prepareDeleteAll method - both of these calls should be removed).

However, fixing Problem 1 arises the following
Problem 2.
There is no way to get a number of deleted fields from DeleteAllQuery if batch writing is used - it will always be batched (and therefore 1 always will be returned, see DatabaseAccessor.basicExecuteCall above).
In fact, the same happens to native queries, too.
Suggested solution is to define a new boolean flag indicating that the query call(s) could be batched (batchable ?) on ModifyQuery with default value true.
A call that is not batchable will trigger batch execution in DatabaseAccessor.basicExecuteCall.
The corresonding QueryHint should be defined for JPA.
Also in JPA case the user usually expects the query to be executed immediately, therefore EJBQueryImpl.executeUpdate should set QueryHint.BATCHABLE to false (unless of course it has been already set to true by the user). So in JPA case by default UpdateAll, DeleteAll and native queries will be executed right away (even when batching is enabled0 and return the number of updated / deleted rows.

Problem 3.
DatabasePlatform.executeBatch method.
Oracle10Platform uses getUpdateCount to retun the number of updated / deleted rows after batch writing. Why would not that work for not PreparedStatement, too? Does it work for any other data bases? If so, should be moved to the base method in DatabsePlatform.
If instead of getUpdateCount method we still have to examine 
  int[] updateResult = statement.executeBatch()
then (in the base method, in both prepared and not prepared statement cases and based on the comment in Statement.executeBatch) we should do something like what currently done on MySQLPlatform (for some reason only in case of prepared statement):
either mode forgiving version (used in MySQLPlatform):

int[] updateResult = statement.executeBatch();
int updateCount = 0;
for (int count : updateResult) {
    if (count == Statement.SUCCESS_NO_INFO) {
        count = 1;
    }
    updateCount += count;
}
return updateCount;

or more strict:

int[] updateResult = statement.executeBatch();
int updateCount = 0;
for (int count : updateResult) {
    if (count >= 0) {
        updateCount += count;
    }
}
return updateCount;

Comment 3 Tom Ware 2011-03-18 09:35:41 EDT

Adrian, please set the target for this bug when you know what it is.

Comment 4 Tom Ware 2011-03-18 09:39:47 EDT

Sorry... didn't notice it was already targetted.

Comment 5 Adrian Goerler 2011-05-27 02:21:55 EDT

I won't be able to deliver a patch that addresses the concerns raised by Andrei. Therefore, I am unassigning me from this bug.

Comment 6 Adrian Goerler 2011-05-27 11:36:05 EDT

Created attachment 196776 [details]
Patch skipping the tests tests with issues on Oracle10Platform and higher

until a patch for this issue is found, the tests 
TestDeleteQuery.testDeleteAllDepartments as well as 
TestBiDirectionalManyToMany.testCascadeMerge

are skipped.

Tested on Oracle 11 with Oracle10Platform

Comment 7 Adrian Goerler 2011-05-27 11:49:44 EDT

*** Bug 338727 has been marked as a duplicate of this bug. ***

Comment 8 Adrian Goerler 2011-05-30 03:45:28 EDT

Comment on attachment 196776 [details]
Patch skipping the tests tests with issues on Oracle10Platform and higher

Tested on Oracle 11.
Reviewed by Andrei
Checked in at 9474

Comment 9 Adrian Goerler 2011-05-30 03:46:49 EDT

Leaving this bug open until the issue is solved on Oracle so that the skip can be removed.

Comment 10 Tom Ware 2011-05-30 16:00:26 EDT

The checked-in code has a problem.

It adds a dependency to Oracle10Platform.  Currently the wdf tests don't depend on our Oracle-specific projects, we get a compile failure.  Can we skip them for OraclePlatform instead?

Comment 11 Adrian Goerler 2011-05-31 03:37:20 EDT

(In reply to comment #10)
> The checked-in code has a problem.
> It adds a dependency to Oracle10Platform.  Currently the wdf tests don't depend
> on our Oracle-specific projects, we get a compile failure.  Can we skip them
> for OraclePlatform instead?

Hm, it compiled fine on my side. I think the WDF test build against the ecliselink.jar, which contains the OraclePlatform. 

Actually, the test pass with the generic OraclePlatform as the generic OraclePlatform does not override the canBatchWriteWithOptimisticLocking method. 

So I am proposing to skip the test on the platform by name and not by class.

Comment 12 Adrian Goerler 2011-05-31 03:41:09 EDT

Created attachment 196962 [details]
updated patch skipping the test on Oracle10 and Oracle11 (by platform class name)

Comment 13 Adrian Goerler 2011-05-31 05:23:21 EDT

Comment on attachment 196962 [details]
updated patch skipping the test on Oracle10 and Oracle11 (by platform class name)

Tested with plain OracelPlatform

checked in at 8979

Comment 14 Rama Krishna Lagisetti 2012-05-24 05:55:13 EDT

*** Bug 380506 has been marked as a duplicate of this bug. ***

Comment 15 James Sutherland 2012-11-20 09:22:53 EST

Fixed,

See, bug#328714

Comment 16 James Sutherland 2012-11-26 11:04:55 EST

fixed in 2.5

Comment 17 Eclipse Webmaster 2022-06-09 10:25:23 EDT

The Eclipselink project has moved to Github: https://github.com/eclipse-ee4j/eclipselink