|
Just checked JDBC 4.0 spec... It confirms CallableStatement.executeQuery() should be supported for Stored Procedures that return a single resultset. See section 13.3.3.1.
I don't think executeQuery() is correct,. a definition of a single result set does not preclude the procedure returning one or more update counts, which cannot be handled by executeQuery. Even though Derby doesn't support upfates counts with procedures it might in the future.
The code in 13.3.3.1 (also in JDBC 3) is an example, not part of the spec. We don't see how the called function/procedure GETINFO is defined, maybe in that system it is defined to only return a single result set and no update counts. 13.3.3.1 is only true if it is guaranteed that the statement only returns a single result set, I don't see how in Derby that guarantee can be made. Section 13.3.3.3 is the true case (I believe here) quote: 'If the type or number of results returned by a CallableStatement object are not known until run time, the CallableStatement object should be executed with the method execute' Note, 'type or number of results', not number of result sets. Thanks Dan for your comments. You did raise an important issue about attempting to execute a procedure using executeQuery() when the procedure might also return an update count. I think this case can be handled at run-time... It would be possible to throw an exception if the procedure also returns an update count. (which Derby doesn't currently.)
So, I think it should still be possible to use executeQuery() to retrieve results of a single resultset stored procedure. JDBC spec seems to indicate this should be allowed and Derby Client already does allow this. We should, however, catch incorrect invocations when the procedure might return an update count. I am not working on this bug. It would be good to address this, but busy with other activities.
It turns out that the issue can be fixed just by not throwing the
exception when exactly one result set is produced by the stored procedure. EmbedStatement.executeStatement() will do the right thing if the exception is not raised. The attached patch fixes it by counting the number of returned result sets and raising an exception if it is not one. It also adds a new test, jdbcapi/ProcedureTest.junit, which tests executeQuery() with stored procedures in Statement, PreparedStatement and CallableStatement. Derbyall ran successfully. The patch is ready for review. Thanks. Is the rollback behaviour correct with this patch?
See the comment in GenericPreparedStatement around line 392 where the current exceptions are thrown for a mismatch of executeQuery/Update with a DML/query statement. Thanks for looking at the patch, Dan! I'm pretty sure the rollback behaviour is correct (if it was correct before). I didn't touch the executeUpdate part, only executeQuery. If you execute a DDL/insert/update/delete statement with executeQuery(), the exception will be thrown at the same point as before. I'll look more into it on Monday.
The patch is not correct. It is obtaining the count of result sets directly from the raw data using:
resultSet.getActivation().getDynamicResults() The list of returned result sets may include result sets that will not be returned to the application. Thus the count in the new code could be three, whereas the actual return count to the application would be one. See EmbedStatement.processDynamicResults for the various reasons a result set may not be returned to the application. The rollback, I agree matches the previous behaviour, the case I was thinking of was a DML statement executed within a procedure that returned multiple result sets and was executed using executeQuery. In that case an exception is thrown and the results of the DML within the procedure should be rolled back. Thanks, great catch! I had missed that.
This means that a stored procedure is free to return any ResultSet object, but only the ones that are still open and were created by the connection which called the stored proc are visible to the user? I find this, um, fascinating... For instance, I tried to execute this procedure public static void myProc(ResultSet[] rs1, ResultSet rs2[]) throws SQLException { rs1[0] = DriverManager.getConnection("jdbc:postgresql://localhost/mydb", "test", "test"). createStatement().executeQuery("select * from t"); rs2[0] = DriverManager.getConnection("jdbc:default:connection"). createStatement().executeQuery("select * from t"); } and it succeeded, but only returned the result set created by jdbc:default:connection. Is it intentional that we silently ignore the result sets from other connections and closed result sets? Isn't it more appropriate to raise an exception, or at least a warning? Anyway, it seems like the test for the result set count has to be moved from GenericPreparedStatement.execute() to EmbedStatement.executeStatement(). Otherwise, we would have to import impl.jdbc classes into impl.sql, which does not sound like a good idea. According to the comment in GenericPreparedStatement, moving the test could affect rollback, but I believe it is unaffected as long as the test is performed inside the try block in ES.executeStatement(), and before the commit logic. I also think moving the test from the sql execution code to the jdbc code will make the code cleaner, since we then get rid of the executeQuery/executeUpdate flags that currently have to be passed to GPS.execute(). I'll add test cases for the other scenarios you mentioned as well. Yes, the behaviour on result sets not created by jdbc:default:connection is intentional.
SQL part 13 states: Section 8.3, clause 17b "b) If any element of RS is not an object returned by a connection to the current SQL system and SQL session, then the effect is implementation-defined." Derby's implementation defined behaviour is to discard such result sets. Then for the closed ones: SQL part 13 states: Section 8.3, clause 19 "If R is an external Java routine, then let FRC be a copy of the elements of RS that remain open in the order that they were opened in SQL." I can't see anything in the spec that indicates an exception or warning should be raised if the JDBC ResultSet is closed when it is returned to the SQL engine. I wrote a test where a stored procedure created a table and did not
return any result set. It was executed with executeQuery() and auto-commit was enabled. The different drivers did this: Embedded: The query failed with an SQLException, and the creation of the table was rolled back. Client driver and JCC: The query failed with an SQLException, but the creation of the table was not rolled back. I believe the embedded driver behaves correctly, and the client driver (and JCC) should be changed to match embedded. Attached new patch (derby-501-v2.diff).
This patch modifies EmbedStatement.processDynamicResults() so that it returns the number of dynamic results instead of a boolean. EmbedStatement.executeStatement() uses this number to decide whether an exception is to be raised. With this change, the executeQuery and executeUpdate parameters are no longer needed in GenericPreparedStatement.execute(). Many new test cases have been added to ProcedureTest. The new test cases test executeUpdate() (which the previous patch didn't touch) and the rollback behaviour. Seven of the test cases fail with the client driver and JCC, but I expect all of them to succeed with the client driver after Derbyall ran cleanly on Sun JVM 1.5.0 / Solaris 10 x64. Please review the patch. Thanks! Committed the new test class in the v2 patch with revision 412711 (to make it easier to work on
I have uploaded a v3 patch which is identical to v2, but without the test class. I'd appreciate if someone could review it. Thanks! Might this change affect existing applications?
Could you describe any behaviour change that might impact users as it might be documented in the Release Notes. I don't think it is very likely that existing applications are
affected, but they *might* be. To be on the safe side, we could add a line to the release notes saying: The behaviour of executeQuery() and executeUpdate() has been modified to follow the JDBC standard when executing stored procedures. It is now possible to use executeQuery() to execute a stored procedure that returns exactly one ResultSet (this would fail in previous releases of Derby). executeUpdate() will raise an exception if it is used to execute a stored procedure that returns one or more ResultSets (this would succeed in previous releases of Derby). Note that this only applies to the embedded driver. There will be some changes to the client driver in changes. When writing the release notes, it is probably best to write one note describing both Does anyone have comments to the approach used in the latest patch? I intend to commit if I don't hear anything in a couple of days. Thanks!
Committed revision 414795.
This issue has been resolved for over a year with no further movement. Closing.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
Client and embedded drivers differ on invoking a procedure that returns a single Dynamic resultSet using CallableStatement.executeQuery()
DERBY-310, though in this case, we probably want to change the embedded driver to match network client.