Derby
  1. Derby
  2. DERBY-4869

Implement JDBC 4.1, the api increment introduced by Java 7

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.9.1.0
    • Component/s: JDBC
    • Labels:
      None

      Description

      This is a master issue logged to track our work implementing JDBC 4.1, the changes to the java.sql and javax.sql packages introduced by Java 7.

      1. derby-4869_Statement_addBatch_execute_etc.dif
        2 kB
        Lily Wei
      2. derby-4869-01-ac-rs-getObject.diff
        31 kB
        Rick Hillegas
      3. derby-4869-01-ad-rs-getObject.diff
        32 kB
        Rick Hillegas
      4. derby-4869-02-aa-cs-ps-addBatch.diff
        3 kB
        Rick Hillegas
      5. derby-4869-02-ab-cs-ps-addBatch.diff
        3 kB
        Rick Hillegas
      6. derby-4869-03-aa-rs-getObject-errorCleanup.diff
        4 kB
        Rick Hillegas
      7. derby-4869-04-aa-rs-getObject-simplification.diff
        2 kB
        Rick Hillegas
      8. derby-4869-05-aa-rs-getObject-exception.diff
        2 kB
        Rick Hillegas
      9. derby-4869-06-aa-cs-getObject.diff
        60 kB
        Rick Hillegas
      10. derby-4869-06-ab-cs-getObject.diff
        64 kB
        Rick Hillegas
      11. derby-4869-07-aa-timedisplacement.diff
        0.6 kB
        Rick Hillegas
      12. derby-4869-08-ac-nullHandling.diff
        35 kB
        Rick Hillegas
      13. derby-4869-09-ac-abort.diff
        27 kB
        Rick Hillegas
      14. derby-4869-10-aa-abortSecurityTest.diff
        30 kB
        Rick Hillegas
      15. derby-4869-11-aa-abortHidePrivates.diff
        5 kB
        Rick Hillegas
      16. derby-4869-12-aa-xaAndPooledAborts.diff
        8 kB
        Rick Hillegas
      17. derby-4869-13-aa-xaAndPooledAbortsSecurityManager.diff
        5 kB
        Rick Hillegas
      18. derby-4869-14-ac-closeOnCompletion.diff
        24 kB
        Rick Hillegas
      19. derby-4869-15-aa-autoGenKeys.diff
        4 kB
        Rick Hillegas
      20. derby-4869-16-aa-clarifyStatementSpec.diff
        4 kB
        Rick Hillegas
      21. derby-4869-17-aa-statementTimeoutException.diff
        6 kB
        Rick Hillegas
      22. derby-4869-18-aa-getSetSchema.diff
        10 kB
        Rick Hillegas
      23. derby-4869-19-aa-getSetNetworkTimeout.diff
        8 kB
        Rick Hillegas
      24. derby-4869-20-aa-setSchemaFastPath.diff
        0.6 kB
        Rick Hillegas
      25. derby-4869-21-aa-implicitlyClosedResultSets.diff
        5 kB
        Rick Hillegas
      26. derby-4869-21-ab-implicitlyClosedResultSets.diff
        7 kB
        Rick Hillegas
      27. derby-4869-22-aa-unstableStatementTest.diff
        2 kB
        Rick Hillegas
      28. derby-4869-23-aa-dbmd.diff
        18 kB
        Rick Hillegas
      29. derby-4869-24-ab-getParentLogger.diff
        31 kB
        Rick Hillegas
      30. derby-4869-25-aa-removeClosureCheck.diff
        3 kB
        Rick Hillegas
      31. derby-4869-26-aa-signatureTests.diff
        9 kB
        Rick Hillegas
      32. derby-4869-27-aa-driver40.diff
        4 kB
        Rick Hillegas
      33. derby-4869-28-ab-autoloadExceptionFactory.diff
        7 kB
        Rick Hillegas
      34. derby-4869-29-aa-fixAutoloadTest.diff
        2 kB
        Rick Hillegas
      35. derby-4869-30-aa-unstableStatementTest.diff
        1 kB
        Rick Hillegas
      36. derby-4869-31-aa-unstableStatementTest.diff
        2 kB
        Rick Hillegas
      37. derby-4869-31-ab-unstableStatementTest.diff
        2 kB
        Rick Hillegas
      38. derby-4869-32_add_CallableStatement_Connection_getTypeMap_test.diff
        4 kB
        Lily Wei
      39. derby-4869-33_add_CallableStatement_Connection_getTypeMap_test.diff
        1 kB
        Lily Wei
      40. derby-4869-exp-01-aa-noTryCatch.diff
        16 kB
        Rick Hillegas
      41. disable-tests.diff
        5 kB
        Knut Anders Hatlen
      42. Drv41.java
        0.9 kB
        Knut Anders Hatlen
      43. fix-compiler-warning.diff
        2 kB
        Knut Anders Hatlen
      44. JDBC_4.1_Changes.html
        16 kB
        Rick Hillegas
      45. PSTimeout_execute.java
        3 kB
        Lily Wei
      46. PSTimeout.java
        4 kB
        Lily Wei
      47. timezone.diff
        7 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Rick Hillegas added a comment -

          Attaching JDBC_4.1_Changes.html, a page describing what we need to do to implement JDBC 4.1. The spec for these changes is contained in the Java 7 javadoc, which can be browsed here: http://download.java.net/jdk7/docs/api/

          Show
          Rick Hillegas added a comment - Attaching JDBC_4.1_Changes.html, a page describing what we need to do to implement JDBC 4.1. The spec for these changes is contained in the Java 7 javadoc, which can be browsed here: http://download.java.net/jdk7/docs/api/
          Hide
          Knut Anders Hatlen added a comment -

          I ran suites.All with 1.7.0-ea-b115 and saw that the following tests failed because they couldn't find implementations of all the methods in the java.sql.* interfaces:

          org.apache.derbyTesting.functionTests.tests.jdbc4.UnsupportedVetter
          org.apache.derbyTesting.functionTests.tests.jdbc4.VerifySignatures
          org.apache.derbyTesting.functionTests.tests.jdbcapi.ClosedObjectTest

          Show
          Knut Anders Hatlen added a comment - I ran suites.All with 1.7.0-ea-b115 and saw that the following tests failed because they couldn't find implementations of all the methods in the java.sql.* interfaces: org.apache.derbyTesting.functionTests.tests.jdbc4.UnsupportedVetter org.apache.derbyTesting.functionTests.tests.jdbc4.VerifySignatures org.apache.derbyTesting.functionTests.tests.jdbcapi.ClosedObjectTest
          Hide
          Knut Anders Hatlen added a comment -

          Attaching a patch that disables the failing tests on JDK 7 for now, so that we won't see all this noise when testing the 10.7.1 release candidate. The tests should be re-enabled when we have implemented the missing methods.

          Committed revision 1035164.

          Show
          Knut Anders Hatlen added a comment - Attaching a patch that disables the failing tests on JDK 7 for now, so that we won't see all this noise when testing the 10.7.1 release candidate. The tests should be re-enabled when we have implemented the missing methods. Committed revision 1035164.
          Hide
          Rick Hillegas added a comment -

          The following wiki page tracks the progress of this master task: http://wiki.apache.org/db-derby/JdbcFourOne

          Show
          Rick Hillegas added a comment - The following wiki page tracks the progress of this master task: http://wiki.apache.org/db-derby/JdbcFourOne
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-01-ac-rs-getObject.diff. This adds new JDBC 4.1 methods to ResultSet. I added a test case for these methods to the JDBC 4.0 test of ResultSets. That test runs cleanly for me on Java 6. I haven't run the test on Java 7 yet because the Linux installers for JDK 7 don't work. But I will look for another platform to test-drive these changes on Java 7. I am running the full regression test suites now.

          Adds the following new methods to Derby's JDBC 4.0 implementations of ResultSet:

          public <T> T getObject( int columnIndex, Class<T> type ) throws SQLException;
          public <T> T getObject( String columnName, Class<T> type ) throws SQLException;

          The implementations simply switch on the Class type and forward calls to the appropriate pre-existing getters.

          I added a test for these methods to the test for JDBC 4.0 ResultSets. To simplify the code, I wrapped the Derby ResultSets in a class which exposes the new methods. We may want to revisit this test after Java 7 goes GA and we are allowed to ship a derbyTesting.jar which is built with the Java 7 compiler.

          Touches the following files:

          ---------

          M java/engine/org/apache/derby/impl/jdbc/EmbedResultSet40.java

          Added new forwarding methods to the embedded ResultSet.

          ---------

          M java/client/org/apache/derby/client/am/ResultSet.java
          M java/client/org/apache/derby/client/net/NetResultSet40.java

          Added new forwarding methods to the network ResultSet.

          ---------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java

          Added a test for the new ResultSet methods.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-01-ac-rs-getObject.diff. This adds new JDBC 4.1 methods to ResultSet. I added a test case for these methods to the JDBC 4.0 test of ResultSets. That test runs cleanly for me on Java 6. I haven't run the test on Java 7 yet because the Linux installers for JDK 7 don't work. But I will look for another platform to test-drive these changes on Java 7. I am running the full regression test suites now. Adds the following new methods to Derby's JDBC 4.0 implementations of ResultSet: public <T> T getObject( int columnIndex, Class<T> type ) throws SQLException; public <T> T getObject( String columnName, Class<T> type ) throws SQLException; The implementations simply switch on the Class type and forward calls to the appropriate pre-existing getters. I added a test for these methods to the test for JDBC 4.0 ResultSets. To simplify the code, I wrapped the Derby ResultSets in a class which exposes the new methods. We may want to revisit this test after Java 7 goes GA and we are allowed to ship a derbyTesting.jar which is built with the Java 7 compiler. Touches the following files: --------- M java/engine/org/apache/derby/impl/jdbc/EmbedResultSet40.java Added new forwarding methods to the embedded ResultSet. --------- M java/client/org/apache/derby/client/am/ResultSet.java M java/client/org/apache/derby/client/net/NetResultSet40.java Added new forwarding methods to the network ResultSet. --------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java Added a test for the new ResultSet methods.
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-4869-01-ac-rs-getObject.diff.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-4869-01-ac-rs-getObject.diff.
          Hide
          Rick Hillegas added a comment -

          ResultSetTest passed cleanly for me on JDK 7 (build 122) on 64-bit x86 Solaris.

          Show
          Rick Hillegas added a comment - ResultSetTest passed cleanly for me on JDK 7 (build 122) on 64-bit x86 Solaris.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-02-aa-cs-ps-addBatch.diff. This patch enforces the spec clarification that PreparedStatement.addBatch(String) and CallableStatement.addBatch(String) should raise exceptions. I will run regression tests.

          This behavior was already enforced by the embedded driver. This patch makes the network driver enforce this behavior. In addition, a test case is added to verify this behavior.

          Touches the following files:

          ---------

          M java/client/org/apache/derby/client/am/PreparedStatement.java

          Make the network driver conform to the spec.

          ---------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/StatementJdbc20Test.java

          Add a test case verifying the correct behavior.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-02-aa-cs-ps-addBatch.diff. This patch enforces the spec clarification that PreparedStatement.addBatch(String) and CallableStatement.addBatch(String) should raise exceptions. I will run regression tests. This behavior was already enforced by the embedded driver. This patch makes the network driver enforce this behavior. In addition, a test case is added to verify this behavior. Touches the following files: --------- M java/client/org/apache/derby/client/am/PreparedStatement.java Make the network driver conform to the spec. --------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/StatementJdbc20Test.java Add a test case verifying the correct behavior.
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-4869-02-aa-cs-ps-addBatch.diff except for the testPing heisenbug.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-4869-02-aa-cs-ps-addBatch.diff except for the testPing heisenbug.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Rick,

          I looked at the getObject() patch. Some possible improvements:

          • For byte[], instead of doing a Class.forName("[B"), I think you
            could check (type.isArray() && type.getComponentType().equals(byte.class)),
            which would remove the need for catching/ignoring ClassNotFoundException.
          • Would it make sense to try getObject() unconditionally if no other
            types match? Then the following code would work (I think) for most
            numeric column types:

          Number num = rs.getObject("VALUE", Number.class);

          • The API javadoc explicitly mentions that type==null should result in
            an SQLException. For completeness, add a test case for that?
          • The API javadoc for Boolean, Byte, Short, Integer, Long, Float and
            Double recommend the use of valueOf() instead of the constructor, as
            valueOf() "is likely to yield significantly better space and time
            performance".
          • If the type conversion is not supported, an
            SQLFeatureNotSupportedException is thrown. As far as I understand
            the javadoc, this exception is reserved for the situation where the
            method is not supported by the JDBC driver, which is not the case
            here.
          • The client implementation makes closeCloseFilterInputStream()
            non-private in order to call it from the new method. I think all the
            getters called from the new getObject() method will call
            closeCloseFilterInputStream(), so this code is probably not
            necessary.
          Show
          Knut Anders Hatlen added a comment - Hi Rick, I looked at the getObject() patch. Some possible improvements: For byte[], instead of doing a Class.forName("[B"), I think you could check (type.isArray() && type.getComponentType().equals(byte.class)), which would remove the need for catching/ignoring ClassNotFoundException. Would it make sense to try getObject() unconditionally if no other types match? Then the following code would work (I think) for most numeric column types: Number num = rs.getObject("VALUE", Number.class); The API javadoc explicitly mentions that type==null should result in an SQLException. For completeness, add a test case for that? The API javadoc for Boolean, Byte, Short, Integer, Long, Float and Double recommend the use of valueOf() instead of the constructor, as valueOf() "is likely to yield significantly better space and time performance". If the type conversion is not supported, an SQLFeatureNotSupportedException is thrown. As far as I understand the javadoc, this exception is reserved for the situation where the method is not supported by the JDBC driver, which is not the case here. The client implementation makes closeCloseFilterInputStream() non-private in order to call it from the new method. I think all the getters called from the new getObject() method will call closeCloseFilterInputStream(), so this code is probably not necessary.
          Hide
          Knut Anders Hatlen added a comment -

          The addBatch() patch looks fine to me. One minor nit: The test could use the helper methods in prepareStatement() and prepareCall() in BaseJDBCTestCase. That would remove the need for calls to getConnection() and close().

          Show
          Knut Anders Hatlen added a comment - The addBatch() patch looks fine to me. One minor nit: The test could use the helper methods in prepareStatement() and prepareCall() in BaseJDBCTestCase. That would remove the need for calls to getConnection() and close().
          Hide
          Rick Hillegas added a comment -

          Thanks for the quick review of the addBatch() patch, Knut. I polished the test as you suggested. Committed a second rev of the patch, derby-4869-02-ab-cs-ps-addBatch.diff, at subversion revision 1051890.

          Show
          Rick Hillegas added a comment - Thanks for the quick review of the addBatch() patch, Knut. I polished the test as you suggested. Committed a second rev of the patch, derby-4869-02-ab-cs-ps-addBatch.diff, at subversion revision 1051890.
          Hide
          Rick Hillegas added a comment -

          Thanks for the great suggestions on the getObject() patch, Knut. And thanks to Lance for the additional responses on derby-dev. I have incorporated your feedback in a new version of the patch, derby-4869-01-ad-rs-getObject.diff. Committed at subversion revision 1051937. I notice that I have introduced a new warning in the build. I will fix that.

          Show
          Rick Hillegas added a comment - Thanks for the great suggestions on the getObject() patch, Knut. And thanks to Lance for the additional responses on derby-dev. I have incorporated your feedback in a new version of the patch, derby-4869-01-ad-rs-getObject.diff. Committed at subversion revision 1051937. I notice that I have introduced a new warning in the build. I will fix that.
          Hide
          Rick Hillegas added a comment -

          Compiler warning suppressed at subversion revision 1051947.

          Show
          Rick Hillegas added a comment - Compiler warning suppressed at subversion revision 1051947.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Rick,

          In the latest getObject() patch, SQLExceptions are created by calling the SQLException constructor directly. Shouldn't we use SQLExceptionFactory to get the correct subclass of SQLException?

          Show
          Knut Anders Hatlen added a comment - Hi Rick, In the latest getObject() patch, SQLExceptions are created by calling the SQLException constructor directly. Shouldn't we use SQLExceptionFactory to get the correct subclass of SQLException?
          Hide
          Knut Anders Hatlen added a comment -

          Not only to get the correct subclass, by the way, but also to get the correct message text.

          Show
          Knut Anders Hatlen added a comment - Not only to get the correct subclass, by the way, but also to get the correct message text.
          Hide
          Rick Hillegas added a comment -

          Thanks for spotting that howler, Knut. It turns out that the error handling was equally goofy in the embedded case. Attaching derby-4869-03-aa-rs-getObject-errorCleanup.diff. This patch corrects the error messages returned when coercion errors occur in ResultSet.getObject(int,Class). Committed at subversion revision 1052044.

          Touches the following files:

          ---------

          M java/engine/org/apache/derby/impl/jdbc/EmbedResultSet40.java
          M java/client/org/apache/derby/client/net/NetResultSet40.java

          Show
          Rick Hillegas added a comment - Thanks for spotting that howler, Knut. It turns out that the error handling was equally goofy in the embedded case. Attaching derby-4869-03-aa-rs-getObject-errorCleanup.diff. This patch corrects the error messages returned when coercion errors occur in ResultSet.getObject(int,Class). Committed at subversion revision 1052044. Touches the following files: --------- M java/engine/org/apache/derby/impl/jdbc/EmbedResultSet40.java M java/client/org/apache/derby/client/net/NetResultSet40.java
          Hide
          Knut Anders Hatlen added a comment -

          In the last patch, NetResultSet40 preserves the ClassCastException whereas EmbedResultSet40 doesn't. Any reason for this difference?

          The following code...

          + Object result = getObject( columnIndex );
          + if ( !type.isInstance( result ) )

          { throw new ClassCastException( type.getName() ); }

          + return (T) result;

          ...could be simplified to:

          return type.cast(getObject(columnIndex));

          The latter variant will also give a clearer error message (example: "Cannot cast java.lang.Boolean to java.lang.Integer" vs just "java.lang.Integer").

          Show
          Knut Anders Hatlen added a comment - In the last patch, NetResultSet40 preserves the ClassCastException whereas EmbedResultSet40 doesn't. Any reason for this difference? The following code... + Object result = getObject( columnIndex ); + if ( !type.isInstance( result ) ) { throw new ClassCastException( type.getName() ); } + return (T) result; ...could be simplified to: return type.cast(getObject(columnIndex)); The latter variant will also give a clearer error message (example: "Cannot cast java.lang.Boolean to java.lang.Integer" vs just "java.lang.Integer").
          Hide
          Rick Hillegas added a comment -

          I will try the simplification you recommend.

          In the network case, it was easy to pass the error from the getter to the factory method which makes the SQLException. In the embedded case, this would involve adding some new overloads of the corresponding factory methods. Those overloads didn't seem justified by this use case: the information in the swallowed ClassCastException seemed pretty vacuous.

          Show
          Rick Hillegas added a comment - I will try the simplification you recommend. In the network case, it was easy to pass the error from the getter to the factory method which makes the SQLException. In the embedded case, this would involve adding some new overloads of the corresponding factory methods. Those overloads didn't seem justified by this use case: the information in the swallowed ClassCastException seemed pretty vacuous.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Rick. I agree that the ClassCastException doesn't provide much additional information here.

          One more issue: The catch clause in NetResultSet40 now catches all exceptions, not only ClassCastException, which means it will handle all errors in the getters as conversion errors. Take this example:

          ResultSet rs = s.executeQuery("values cast ('abc' as clob)");
          while (rs.next())

          { rs.getObject(1, Clob.class); rs.getObject(1, Clob.class); }

          Here, the problem is that getClob() is called twice on the same column, but the error reported is:

          java.sql.SQLDataException: An attempt was made to get a data value of type 'java.sql.Clob' from a data value of type 'CLOB'.

          The real problem is also reported, but that's hidden further down in the exception chain:

          ...
          Caused by: org.apache.derby.client.am.SqlException: An attempt was made to get a data value of type 'java.sql.Clob' from a data value of type 'CLOB'.
          ...
          Caused by: java.sql.SQLException: Stream or LOB value cannot be retrieved more than once
          ...

          Show
          Knut Anders Hatlen added a comment - Thanks, Rick. I agree that the ClassCastException doesn't provide much additional information here. One more issue: The catch clause in NetResultSet40 now catches all exceptions, not only ClassCastException, which means it will handle all errors in the getters as conversion errors. Take this example: ResultSet rs = s.executeQuery("values cast ('abc' as clob)"); while (rs.next()) { rs.getObject(1, Clob.class); rs.getObject(1, Clob.class); } Here, the problem is that getClob() is called twice on the same column, but the error reported is: java.sql.SQLDataException: An attempt was made to get a data value of type 'java.sql.Clob' from a data value of type 'CLOB'. The real problem is also reported, but that's hidden further down in the exception chain: ... Caused by: org.apache.derby.client.am.SqlException: An attempt was made to get a data value of type 'java.sql.Clob' from a data value of type 'CLOB'. ... Caused by: java.sql.SQLException: Stream or LOB value cannot be retrieved more than once ...
          Hide
          Rick Hillegas added a comment -

          Thanks for pointing that out, Knut. I guess that means that I'll have to fix DERBY-4949 before I declare victory on this one.

          Show
          Rick Hillegas added a comment - Thanks for pointing that out, Knut. I guess that means that I'll have to fix DERBY-4949 before I declare victory on this one.
          Hide
          Rick Hillegas added a comment -

          Attaching the cast simplification suggested by Knut: derby-4869-04-aa-rs-getObject-simplification.diff. Committed at subversion revision 1052268.

          Show
          Rick Hillegas added a comment - Attaching the cast simplification suggested by Knut: derby-4869-04-aa-rs-getObject-simplification.diff. Committed at subversion revision 1052268.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-05-aa-rs-getObject-exception.diff, which removes the over-aggressive exception swallowing in NetResultSet40.getObject(int,Class). The coercion errors now look goofy, so I will go tackle derby-4949. Committed at subversion revision 1052271.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-05-aa-rs-getObject-exception.diff, which removes the over-aggressive exception swallowing in NetResultSet40.getObject(int,Class). The coercion errors now look goofy, so I will go tackle derby-4949. Committed at subversion revision 1052271.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-06-aa-cs-getObject.diff. This is a preliminary patch for adding the new getObject() overloads to our CallableStatement implementations. The production code is ready for review. However, I am still writing the tests.

          Touches the following files:

          ---------

          M java/engine/build.xml
          M java/engine/org/apache/derby/impl/jdbc/EmbedCallableStatement40.java
          M java/engine/org/apache/derby/iapi/jdbc/BrokeredCallableStatement40.java

          Changes to embedded driver.

          ---------

          M java/client/org/apache/derby/client/am/LogicalCallableStatement40.java
          M java/client/org/apache/derby/client/am/CallableStatement.java
          M java/client/org/apache/derby/client/am/CallableStatement40.java

          Changes to network driver.

          ---------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java
          A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41.java
          A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Test.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java

          Tests. I am trying to use the same verification code on ResultSet.getObject() and CallableStatement.getObject().

          Show
          Rick Hillegas added a comment - Attaching derby-4869-06-aa-cs-getObject.diff. This is a preliminary patch for adding the new getObject() overloads to our CallableStatement implementations. The production code is ready for review. However, I am still writing the tests. Touches the following files: --------- M java/engine/build.xml M java/engine/org/apache/derby/impl/jdbc/EmbedCallableStatement40.java M java/engine/org/apache/derby/iapi/jdbc/BrokeredCallableStatement40.java Changes to embedded driver. --------- M java/client/org/apache/derby/client/am/LogicalCallableStatement40.java M java/client/org/apache/derby/client/am/CallableStatement.java M java/client/org/apache/derby/client/am/CallableStatement40.java Changes to network driver. --------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/CallableStatementTest.java A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41.java A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Test.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java Tests. I am trying to use the same verification code on ResultSet.getObject() and CallableStatement.getObject().
          Hide
          Rick Hillegas added a comment -

          Attaching the second rev of the CallableStatement.getObject() patch: derby-4869-06-ab-cs-getObject.diff. This version adds regression tests for the new behavior. I am running the regression suite now.

          Touches the same files as the previous rev.

          Show
          Rick Hillegas added a comment - Attaching the second rev of the CallableStatement.getObject() patch: derby-4869-06-ab-cs-getObject.diff. This version adds regression tests for the new behavior. I am running the regression suite now. Touches the same files as the previous rev.
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me except for the heisenbug in NetworkServerMBeanTest. Committed derby-4869-06-ab-cs-getObject.diff at subversion revision 1054706.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me except for the heisenbug in NetworkServerMBeanTest. Committed derby-4869-06-ab-cs-getObject.diff at subversion revision 1054706.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-07-aa-timedisplacement.diff, an attempt to correct what seems to be a time-displacement bug in the regression test for CallableStatement.getObject(). Committed at subversion revision 1054746.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-07-aa-timedisplacement.diff, an attempt to correct what seems to be a time-displacement bug in the regression test for CallableStatement.getObject(). Committed at subversion revision 1054746.
          Hide
          Knut Anders Hatlen added a comment -

          The getObject() methods don't seem to handle NULL values correctly. Take this code:

          ResultSet rs = s.executeQuery("values cast(null as integer)");
          rs.next();
          System.out.println(rs.getObject(1, Integer.class));

          It prints "0", but I think it should have printed "null".

          Show
          Knut Anders Hatlen added a comment - The getObject() methods don't seem to handle NULL values correctly. Take this code: ResultSet rs = s.executeQuery("values cast(null as integer)"); rs.next(); System.out.println(rs.getObject(1, Integer.class)); It prints "0", but I think it should have printed "null".
          Hide
          Knut Anders Hatlen added a comment -

          All the new getObject() methods are annotated with @SuppressWarnings("unchecked"), presumably because they contain unchecked casts like this:

          if ( String.class.equals( type ) )

          { return (T) getString( parameterIndex ); }

          I'm wondering if it might be worthwhile to restructure the methods slightly and make the casts checked, so that we don't need to suppress warnings. Something like this would silence the warnings, I think:

          Object ret;

          if (String.class.equals(type))

          { ret = getString(parameterIndex); }

          else if (Integer.class.equals(type))

          { ret = getInt(parameterIndex); }

          else if (....)

          { .... }

          else

          { ret = getObject(parameterIndex); }

          return type.cast(ret);

          Show
          Knut Anders Hatlen added a comment - All the new getObject() methods are annotated with @SuppressWarnings("unchecked"), presumably because they contain unchecked casts like this: if ( String.class.equals( type ) ) { return (T) getString( parameterIndex ); } I'm wondering if it might be worthwhile to restructure the methods slightly and make the casts checked, so that we don't need to suppress warnings. Something like this would silence the warnings, I think: Object ret; if (String.class.equals(type)) { ret = getString(parameterIndex); } else if (Integer.class.equals(type)) { ret = getInt(parameterIndex); } else if (....) { .... } else { ret = getObject(parameterIndex); } return type.cast(ret);
          Hide
          Knut Anders Hatlen added a comment -

          The tests still fail on this side of the Atlantic, so I took the liberty of adding some code to the tests to calculate the expected timestamps in a way that takes the current time zone into account. The tests ran cleanly for me with these changes. I also changed the timezone on my machine to PST and ran jdbc4.ResultSetTest/jdbc4.CallableStatementTest with no failures.

          Committed timezone.diff to trunk with revision 1054933.

          Show
          Knut Anders Hatlen added a comment - The tests still fail on this side of the Atlantic, so I took the liberty of adding some code to the tests to calculate the expected timestamps in a way that takes the current time zone into account. The tests ran cleanly for me with these changes. I also changed the timezone on my machine to PST and ran jdbc4.ResultSetTest/jdbc4.CallableStatementTest with no failures. Committed timezone.diff to trunk with revision 1054933.
          Hide
          Rick Hillegas added a comment -

          Thanks for the comments and the test-tweaking, Knut. I agree that the treatment of nulls looks wrong. I will try your suggestion for suppressing the suppression annotation too.

          Show
          Rick Hillegas added a comment - Thanks for the comments and the test-tweaking, Knut. I agree that the treatment of nulls looks wrong. I will try your suggestion for suppressing the suppression annotation too.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-08-ac-nullHandling.diff. This is very close to the patch which I just committed at subversion revision 1055872. I made a couple additional changes to CallableStatementTest to remove some cruft and re-enable a test which I had disabled while working on DERBY-4959. Now that DERBY-4958 and DERBY-4959 are fixed, I am able to cleanly run new test cases which verify the following fixes which Knut suggested:

          1) Fix the handling of null values in the new getObject() overloads.

          2) Rework the getObject() logic to remove the @SuppressWarnings annotation.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-08-ac-nullHandling.diff. This is very close to the patch which I just committed at subversion revision 1055872. I made a couple additional changes to CallableStatementTest to remove some cruft and re-enable a test which I had disabled while working on DERBY-4959 . Now that DERBY-4958 and DERBY-4959 are fixed, I am able to cleanly run new test cases which verify the following fixes which Knut suggested: 1) Fix the handling of null values in the new getObject() overloads. 2) Rework the getObject() logic to remove the @SuppressWarnings annotation.
          Hide
          Knut Anders Hatlen added a comment -

          The null-handling patch looks fine to me. Thanks, Rick!

          Two tiny nits:

          • In the catch-all case where we call getObject(int), we now have a redundant call to Class.cast()
          • Since there's only one place where we do a cast now, the try/catch that ignores ClassCastException could be narrowed down to enclose that single line only. And probably a little comment explaining why we ignore the exception is in order. Alternatively, the intention of the code may be clearer if we remove the try/catch altogether and instead do an explicit check, something like

          if (retval == null || type.isInstance(retval))

          { return type.cast(retval); }
          Show
          Knut Anders Hatlen added a comment - The null-handling patch looks fine to me. Thanks, Rick! Two tiny nits: In the catch-all case where we call getObject(int), we now have a redundant call to Class.cast() Since there's only one place where we do a cast now, the try/catch that ignores ClassCastException could be narrowed down to enclose that single line only. And probably a little comment explaining why we ignore the exception is in order. Alternatively, the intention of the code may be clearer if we remove the try/catch altogether and instead do an explicit check, something like if (retval == null || type.isInstance(retval)) { return type.cast(retval); }
          Hide
          Rick Hillegas added a comment -

          Thanks for those suggestions, Knut. I am attaching derby-4869-exp-01-aa-noTryCatch.diff. This patch eliminates the try-catch block. With this patch in place, the new getObject() overload no longer catches cast exceptions raised by the forwarded getXXX() methods. This causes an error in CallableStatementTest because getObject() raises a ClassCastException rather than the more informative type-mismatch error. I do not see that as an improvement. You are welcome to continue polishing this patch if you want to. Thanks.

          Show
          Rick Hillegas added a comment - Thanks for those suggestions, Knut. I am attaching derby-4869-exp-01-aa-noTryCatch.diff. This patch eliminates the try-catch block. With this patch in place, the new getObject() overload no longer catches cast exceptions raised by the forwarded getXXX() methods. This causes an error in CallableStatementTest because getObject() raises a ClassCastException rather than the more informative type-mismatch error. I do not see that as an improvement. You are welcome to continue polishing this patch if you want to. Thanks.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for trying it out, Rick. It sounds like a bug that a getXXX() method throws a ClassCastException instead of an SQLException, so I filed DERBY-4970 to track it.

          Show
          Knut Anders Hatlen added a comment - Thanks for trying it out, Rick. It sounds like a bug that a getXXX() method throws a ClassCastException instead of an SQLException, so I filed DERBY-4970 to track it.
          Hide
          Knut Anders Hatlen added a comment -

          Now that DERBY-4970 has been fixed I don't see any failures with the noTryCatch patch, so I think it would be fine to check it in. Thanks.

          Show
          Knut Anders Hatlen added a comment - Now that DERBY-4970 has been fixed I don't see any failures with the noTryCatch patch, so I think it would be fine to check it in. Thanks.
          Hide
          Rick Hillegas added a comment -

          Thanks, Knut. ResultSetTest and CallableStatementTest pass cleanly now. Committed derby-4869-exp-01-aa-noTryCatch.diff at subversion revision 1058554.

          Show
          Rick Hillegas added a comment - Thanks, Knut. ResultSetTest and CallableStatementTest pass cleanly now. Committed derby-4869-exp-01-aa-noTryCatch.diff at subversion revision 1058554.
          Hide
          Rick Hillegas added a comment -

          JDBC 4.1 adds a new method to Connecton: abort(Executor). This method attempts to terminate the Connection and release its resources. If a SecurityManager is in place, the new method expects that SQLPermission( "callAbort" ) ) will have been granted to the abort() method. I believe that the motivation for this permission is to prevent an ordinary user of a pooled connection from accidentally destroying the underlying physical connection to the database, since that is one of the consequences of calling abort(). Only privileged code would be allowed to intervene and abort runaway connections.

          I think that we should document how to grant this permission, probably in the JDBC section of the Reference Guide.

          I also think that our default and template security policies should not grant this permission to any of the Derby code domains. My sense right now is that the permission should be granted to an outer application code domain only accessed by superusers.

          Other or concurring opinions?

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - JDBC 4.1 adds a new method to Connecton: abort(Executor). This method attempts to terminate the Connection and release its resources. If a SecurityManager is in place, the new method expects that SQLPermission( "callAbort" ) ) will have been granted to the abort() method. I believe that the motivation for this permission is to prevent an ordinary user of a pooled connection from accidentally destroying the underlying physical connection to the database, since that is one of the consequences of calling abort(). Only privileged code would be allowed to intervene and abort runaway connections. I think that we should document how to grant this permission, probably in the JDBC section of the Reference Guide. I also think that our default and template security policies should not grant this permission to any of the Derby code domains. My sense right now is that the permission should be granted to an outer application code domain only accessed by superusers. Other or concurring opinions? Thanks, -Rick
          Hide
          Rick Hillegas added a comment -

          All righty then. My previous comment was a little muddled. This is the shape of something that actually works:

          1) SQLPermission( "callAbort" ) must be granted to derby.jar and to derbyclient.jar. Note, however, that Derby's implementations of abort(Executor) are not wrapped in doPrivileged blocks. That means that outer (application) code domains are NOT absolved of the need to be granted this permission also. The application designer controls who can call this method by only granting this permission to tools used by superusers.

          2) Our JDBC documentation in the Reference Guide should explain the need to grant this permission both to Derby and to the superuser tools.

          3) We will need to make similar statements in the Developer's Guide in the section titled "Running Derby under a security manager".

          Show
          Rick Hillegas added a comment - All righty then. My previous comment was a little muddled. This is the shape of something that actually works: 1) SQLPermission( "callAbort" ) must be granted to derby.jar and to derbyclient.jar. Note, however, that Derby's implementations of abort(Executor) are not wrapped in doPrivileged blocks. That means that outer (application) code domains are NOT absolved of the need to be granted this permission also. The application designer controls who can call this method by only granting this permission to tools used by superusers. 2) Our JDBC documentation in the Reference Guide should explain the need to grant this permission both to Derby and to the superuser tools. 3) We will need to make similar statements in the Developer's Guide in the section titled "Running Derby under a security manager".
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-09-ac-abort.diff. This is the first increment of support for Connection.abort(Executor). I will run regression tests.

          This patch adds the new abort() method to the JDBC 4.0 physical connections (EmbedConnection40 and NetConnection40). The abort method does the following:

          1) Checks for SQLPermission( "callAbort" ).

          2) Marks the physical connection as closed so that new work cannot start in it.

          3) Runs the rollback() and close() methods inside the Executor.

          This patch also includes a basic test of the physical connections in an environment which enjoys the correct SQLPermission.

          Follow-on patches should supply additional tests. In particular, the following areas should be explored:

          1) Security concerns. Exercise abort() when there is no SecurityManager and when there is a SecurityManager but the correct SQLPermission has not been granted to the caller.

          2) Calling abort() on pooled connections.

          3) Calling abort() on XA connections.

          Touches the following files:

          -----------

          M java/engine/org/apache/derby/impl/jdbc/EmbedConnection40.java
          M java/engine/org/apache/derby/impl/jdbc/EmbedConnection.java
          M java/engine/org/apache/derby/impl/jdbc/Util.java

          Add abort() logic to the embedded physical connection.

          -----------

          M java/client/org/apache/derby/client/net/NetConnection40.java
          M java/client/org/apache/derby/client/am/Connection.java

          Add abort() logic to the network physical connection.

          -----------

          M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection40.java
          M java/client/org/apache/derby/client/am/LogicalConnection40.java

          Add abort() forwarding to the wrapper connections.

          -----------

          M java/drda/org/apache/derby/drda/server.policy
          M java/drda/org/apache/derby/drda/template.policy

          Add the appropriate new SQLPermission to Derby's policy files.

          -----------

          A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Conn.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionMethodsTest.java
          M java/testing/org/apache/derbyTesting/functionTests/util/derby_tests.policy

          Basic test for abort().

          Show
          Rick Hillegas added a comment - Attaching derby-4869-09-ac-abort.diff. This is the first increment of support for Connection.abort(Executor). I will run regression tests. This patch adds the new abort() method to the JDBC 4.0 physical connections (EmbedConnection40 and NetConnection40). The abort method does the following: 1) Checks for SQLPermission( "callAbort" ). 2) Marks the physical connection as closed so that new work cannot start in it. 3) Runs the rollback() and close() methods inside the Executor. This patch also includes a basic test of the physical connections in an environment which enjoys the correct SQLPermission. Follow-on patches should supply additional tests. In particular, the following areas should be explored: 1) Security concerns. Exercise abort() when there is no SecurityManager and when there is a SecurityManager but the correct SQLPermission has not been granted to the caller. 2) Calling abort() on pooled connections. 3) Calling abort() on XA connections. Touches the following files: ----------- M java/engine/org/apache/derby/impl/jdbc/EmbedConnection40.java M java/engine/org/apache/derby/impl/jdbc/EmbedConnection.java M java/engine/org/apache/derby/impl/jdbc/Util.java Add abort() logic to the embedded physical connection. ----------- M java/client/org/apache/derby/client/net/NetConnection40.java M java/client/org/apache/derby/client/am/Connection.java Add abort() logic to the network physical connection. ----------- M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection40.java M java/client/org/apache/derby/client/am/LogicalConnection40.java Add abort() forwarding to the wrapper connections. ----------- M java/drda/org/apache/derby/drda/server.policy M java/drda/org/apache/derby/drda/template.policy Add the appropriate new SQLPermission to Derby's policy files. ----------- A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Conn.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionMethodsTest.java M java/testing/org/apache/derbyTesting/functionTests/util/derby_tests.policy Basic test for abort().
          Hide
          Rick Hillegas added a comment -

          Tests ran cleanly for me. Committed derby-4869-09-ac-abort.diff at subversion revision 1060422.

          Show
          Rick Hillegas added a comment - Tests ran cleanly for me. Committed derby-4869-09-ac-abort.diff at subversion revision 1060422.
          Hide
          Knut Anders Hatlen added a comment -

          With the public run() and beginAborting() methods, couldn't malicious code bypass the SQLPermission check by doing something like

          ((EmbedConnection) conn).beginAborting();
          executor.execute((Runnable) conn);

          ?

          Show
          Knut Anders Hatlen added a comment - With the public run() and beginAborting() methods, couldn't malicious code bypass the SQLPermission check by doing something like ((EmbedConnection) conn).beginAborting(); executor.execute((Runnable) conn); ?
          Hide
          Rick Hillegas added a comment -

          Thanks for noticing that Knut. I'll give some thought to hiding that behavior.

          Show
          Rick Hillegas added a comment - Thanks for noticing that Knut. I'll give some thought to hiding that behavior.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-10-aa-abortSecurityTest.diff. This adds more tests for Connection.abort(Executor). Committed at subversion revision 1060509.

          1) Test that abort() works when there is no SecurityManager installed.

          2) Test that abort() fails when there is a SecurityManager but the correct permission has not been granted by the policy file.

          Touches the following files:

          A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/AbortTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/_Suite.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/copyfiles.ant
          A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/noAbortPermission.policy

          Show
          Rick Hillegas added a comment - Attaching derby-4869-10-aa-abortSecurityTest.diff. This adds more tests for Connection.abort(Executor). Committed at subversion revision 1060509. 1) Test that abort() works when there is no SecurityManager installed. 2) Test that abort() fails when there is a SecurityManager but the correct permission has not been granted by the policy file. Touches the following files: A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/AbortTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/_Suite.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/copyfiles.ant A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/noAbortPermission.policy
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-11-aa-abortHidePrivates.diff. This is an attempt to address Knut's concerns about exposing dangerous behavior to applications. Committed at subversion revision 1060535.

          Touches the following files:

          M java/engine/org/apache/derby/impl/jdbc/EmbedConnection40.java
          M java/engine/org/apache/derby/impl/jdbc/EmbedConnection.java
          M java/client/org/apache/derby/client/net/NetConnection40.java
          M java/client/org/apache/derby/client/am/Connection.java

          Show
          Rick Hillegas added a comment - Attaching derby-4869-11-aa-abortHidePrivates.diff. This is an attempt to address Knut's concerns about exposing dangerous behavior to applications. Committed at subversion revision 1060535. Touches the following files: M java/engine/org/apache/derby/impl/jdbc/EmbedConnection40.java M java/engine/org/apache/derby/impl/jdbc/EmbedConnection.java M java/client/org/apache/derby/client/net/NetConnection40.java M java/client/org/apache/derby/client/am/Connection.java
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-12-aa-xaAndPooledAborts.diff. This patch adds tests for calling abort() on pooled and xa connections. Committed at subversion revision 1060557.

          The tests disclosed bugs in the forwarding of the abort() call. Those bugs are fixed here.

          Touches the following files:

          ----------

          M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java
          M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection40.java
          M java/client/org/apache/derby/client/am/LogicalConnection40.java

          Fixes the wrappers to check for the liveness of the physical connection before forwarding the abort() call.

          ----------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionMethodsTest.java
          M java/testing/org/apache/derbyTesting/junit/TestConfiguration.java

          New tests for calling abort() on pooled and xa connections.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-12-aa-xaAndPooledAborts.diff. This patch adds tests for calling abort() on pooled and xa connections. Committed at subversion revision 1060557. The tests disclosed bugs in the forwarding of the abort() call. Those bugs are fixed here. Touches the following files: ---------- M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection40.java M java/client/org/apache/derby/client/am/LogicalConnection40.java Fixes the wrappers to check for the liveness of the physical connection before forwarding the abort() call. ---------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionMethodsTest.java M java/testing/org/apache/derbyTesting/junit/TestConfiguration.java New tests for calling abort() on pooled and xa connections.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-13-aa-xaAndPooledAbortsSecurityManager.diff. This patch adds pooled and xa connections to the tests of Connection.abort() with and without a SecurityManager. Committed at subversion revision 1060570.

          Touches the following files:

          ----------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/AbortTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Conn.java

          Show
          Rick Hillegas added a comment - Attaching derby-4869-13-aa-xaAndPooledAbortsSecurityManager.diff. This patch adds pooled and xa connections to the tests of Connection.abort() with and without a SecurityManager. Committed at subversion revision 1060570. Touches the following files: ---------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/AbortTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Conn.java
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-14-ac-closeOnCompletion.diff. This implements the new JDBC 4.1 methods Statement.closeOnCompletion() and Statement.isCloseOnCompletion(). I will run regression tests.

          The patch takes advantage of the fact that the physical statements already keep pointers to their open ResultSets. Both physical statements already have a method which closes all of the dependent ResultSets before advancing to the next operation which creates ResultSets. The new closeOnCompletion logic simply follows the same roster of dependent objects. This might be brittle and worth encapsulating in an object which explicitly tracks dependencies. Or, that alternative approach might be seen as over-engineering the problem. I don't have strong religion on this point and welcome your opinions.

          I have written some basic regression tests for this functionality. The following areas could use more testing:

          1) Statements with generated keys.

          2) Situations in which mere navigation implicitly closes ResultSets.

          Touches the following files:

          --------------

          M java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java
          M java/engine/org/apache/derby/impl/jdbc/EmbedStatement.java

          Changes to the embedded physical statement.

          --------------

          M java/engine/org/apache/derby/iapi/jdbc/BrokeredStatement.java
          M java/engine/org/apache/derby/iapi/jdbc/EngineStatement.java

          Changes to the wrapped embedded statement.

          --------------

          M java/client/org/apache/derby/client/am/Statement.java
          M java/client/org/apache/derby/client/am/ResultSet.java

          Changes to the network physical statement.

          --------------

          M java/client/org/apache/derby/client/am/LogicalStatementEntity.java

          Changes to the wrapped network statement.

          --------------

          A java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/Wrapper41Statement.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/StatementJdbc20Test.java

          Basic tests.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-14-ac-closeOnCompletion.diff. This implements the new JDBC 4.1 methods Statement.closeOnCompletion() and Statement.isCloseOnCompletion(). I will run regression tests. The patch takes advantage of the fact that the physical statements already keep pointers to their open ResultSets. Both physical statements already have a method which closes all of the dependent ResultSets before advancing to the next operation which creates ResultSets. The new closeOnCompletion logic simply follows the same roster of dependent objects. This might be brittle and worth encapsulating in an object which explicitly tracks dependencies. Or, that alternative approach might be seen as over-engineering the problem. I don't have strong religion on this point and welcome your opinions. I have written some basic regression tests for this functionality. The following areas could use more testing: 1) Statements with generated keys. 2) Situations in which mere navigation implicitly closes ResultSets. Touches the following files: -------------- M java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java M java/engine/org/apache/derby/impl/jdbc/EmbedStatement.java Changes to the embedded physical statement. -------------- M java/engine/org/apache/derby/iapi/jdbc/BrokeredStatement.java M java/engine/org/apache/derby/iapi/jdbc/EngineStatement.java Changes to the wrapped embedded statement. -------------- M java/client/org/apache/derby/client/am/Statement.java M java/client/org/apache/derby/client/am/ResultSet.java Changes to the network physical statement. -------------- M java/client/org/apache/derby/client/am/LogicalStatementEntity.java Changes to the wrapped network statement. -------------- A java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/Wrapper41Statement.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/StatementJdbc20Test.java Basic tests.
          Hide
          Rick Hillegas added a comment -

          The tests ran cleanly for me except for 3 errors. 2 of the errors were familiar heisenbugs in testPing and ManagementMBeanTest. The third error I have not seen before:

          2) testInvalidLDAPServerConnectionError(org.apache.derbyTesting.functionTests.tests.jdbcapi.InvalidLDAPServerAuthenticationTest)junit.framework.AssertionFailedError
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.InvalidLDAPServerAuthenticationTest.testInvalidLDAPServerConnectionError(InvalidLDAPServerAuthenticationTest.java:122)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:110)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)

          This error seems very far away from the code I touched. The test ran cleanly when I ran it standalone. I am inclined to regard this as another heisenbug in the tests.

          Committed derby-4869-14-ac-closeOnCompletion.diff at subversion revision 1061824.

          Show
          Rick Hillegas added a comment - The tests ran cleanly for me except for 3 errors. 2 of the errors were familiar heisenbugs in testPing and ManagementMBeanTest. The third error I have not seen before: 2) testInvalidLDAPServerConnectionError(org.apache.derbyTesting.functionTests.tests.jdbcapi.InvalidLDAPServerAuthenticationTest)junit.framework.AssertionFailedError at org.apache.derbyTesting.functionTests.tests.jdbcapi.InvalidLDAPServerAuthenticationTest.testInvalidLDAPServerConnectionError(InvalidLDAPServerAuthenticationTest.java:122) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:110) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) This error seems very far away from the code I touched. The test ran cleanly when I ran it standalone. I am inclined to regard this as another heisenbug in the tests. Committed derby-4869-14-ac-closeOnCompletion.diff at subversion revision 1061824.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-15-aa-autoGenKeys.diff. This patch fixes the handling of the autogenerated keys ResultSet for auto-completion of network Statements. I will run regression tests.

          One of the dependent ResultSets which a Statement can return is the ResultSet which describes the identity values generated while processing the statement. If the application has requested these values, then the Statement should not be auto-closed until the application has had a chance to inspect them. But when the ResultSet of generated values is closed, then the Statement should be cleaned up if it has requested closeOnCompletion().

          For network Statements, a separate dependent PreparedStatement is generated to help return these keys. I have some reservations about this approach. I think that the returned ResultSet of identity values will not have the correct Statement associated with it. I believe that the Statement returned by ResultSet.getStatement() should be the original, outer Statement. However, it looks like the Statement associated with this ResultSet is the special internal Statement cooked up to retrieve the identity values.

          I am not going to fix that oddity in this patch. But we may want to clean it up later.

          One consequence of this oddity is that closing the ResultSet of generated keys did not trigger the closing of the outer, original Statement. The fix was to make the network ResultSet track its attachment to the outer Statement. This makes the cleanup logic for the network ResultSet look more like the cleanup logic for the embedded ResultSet, which also tracks an outer statement.

          Touches the following files:

          --------------

          M java/client/org/apache/derby/client/am/Statement.java
          M java/client/org/apache/derby/client/am/ResultSet.java

          Changes to make the ResultSet of generated keys track its attachment
          to the original, outer Statement.

          --------------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/StatementJdbc20Test.java

          Regression test to verify that closing the ResultSet of generated keys triggers Statement closure when closeOnCompletion() has been requested.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-15-aa-autoGenKeys.diff. This patch fixes the handling of the autogenerated keys ResultSet for auto-completion of network Statements. I will run regression tests. One of the dependent ResultSets which a Statement can return is the ResultSet which describes the identity values generated while processing the statement. If the application has requested these values, then the Statement should not be auto-closed until the application has had a chance to inspect them. But when the ResultSet of generated values is closed, then the Statement should be cleaned up if it has requested closeOnCompletion(). For network Statements, a separate dependent PreparedStatement is generated to help return these keys. I have some reservations about this approach. I think that the returned ResultSet of identity values will not have the correct Statement associated with it. I believe that the Statement returned by ResultSet.getStatement() should be the original, outer Statement. However, it looks like the Statement associated with this ResultSet is the special internal Statement cooked up to retrieve the identity values. I am not going to fix that oddity in this patch. But we may want to clean it up later. One consequence of this oddity is that closing the ResultSet of generated keys did not trigger the closing of the outer, original Statement. The fix was to make the network ResultSet track its attachment to the outer Statement. This makes the cleanup logic for the network ResultSet look more like the cleanup logic for the embedded ResultSet, which also tracks an outer statement. Touches the following files: -------------- M java/client/org/apache/derby/client/am/Statement.java M java/client/org/apache/derby/client/am/ResultSet.java Changes to make the ResultSet of generated keys track its attachment to the original, outer Statement. -------------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/StatementJdbc20Test.java Regression test to verify that closing the ResultSet of generated keys triggers Statement closure when closeOnCompletion() has been requested.
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-4869-15-aa-autoGenKeys.diff. Committed at subversion revision 1061977.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-4869-15-aa-autoGenKeys.diff. Committed at subversion revision 1061977.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-16-aa-clarifyStatementSpec.diff. This patch adds a test case to verify a spec clarification: an exception should be raised if a PreparedStatement or CallableStatement is used to compile SQL text. Committed at subversion revision 1062822.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/StatementJdbc20Test.java

          Show
          Rick Hillegas added a comment - Attaching derby-4869-16-aa-clarifyStatementSpec.diff. This patch adds a test case to verify a spec clarification: an exception should be raised if a PreparedStatement or CallableStatement is used to compile SQL text. Committed at subversion revision 1062822. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/StatementJdbc20Test.java
          Hide
          Dag H. Wanvik added a comment -

          Looked at the spec vs derby-4869-16-aa-clarifyStatementSpec.diff. Looks good to me. +1

          Show
          Dag H. Wanvik added a comment - Looked at the spec vs derby-4869-16-aa-clarifyStatementSpec.diff. Looks good to me. +1
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-17-aa-statementTimeoutException.diff. This patch makes query timeouts raise a SQLTimeoutException per the JDBC 4.1 spec clarification. I will run regression tests.

          The SQLExceptionFactories were updated to look for the timeout SQLState and wrap the timeout SQLState with a SQLTimeoutException.

          Touches the following files:

          -----------

          M java/engine/org/apache/derby/impl/jdbc/SQLExceptionFactory40.java
          M java/client/org/apache/derby/client/am/SQLExceptionFactory40.java

          JDBC 4.0 introduced SQLTimeoutException and other refined exceptions, which are supposed to be raised instead of plain SQLExceptions. Updated the factories which map SQLStates to refined exception classes.

          -----------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementTestSetup.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/SetQueryTimeoutTest.java

          Added a test case to verify that SQLTimeoutException is raised.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-17-aa-statementTimeoutException.diff. This patch makes query timeouts raise a SQLTimeoutException per the JDBC 4.1 spec clarification. I will run regression tests. The SQLExceptionFactories were updated to look for the timeout SQLState and wrap the timeout SQLState with a SQLTimeoutException. Touches the following files: ----------- M java/engine/org/apache/derby/impl/jdbc/SQLExceptionFactory40.java M java/client/org/apache/derby/client/am/SQLExceptionFactory40.java JDBC 4.0 introduced SQLTimeoutException and other refined exceptions, which are supposed to be raised instead of plain SQLExceptions. Updated the factories which map SQLStates to refined exception classes. ----------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementTestSetup.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/SetQueryTimeoutTest.java Added a test case to verify that SQLTimeoutException is raised.
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-4869-17-aa-statementTimeoutException.diff except for Heisenbugs in testPing and InvalidLDAPServerAuthenticationTest. Committed at subversion revision 1063295.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-4869-17-aa-statementTimeoutException.diff except for Heisenbugs in testPing and InvalidLDAPServerAuthenticationTest. Committed at subversion revision 1063295.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-18-aa-getSetSchema.diff. This patch implements the new JDBC 4.1 methods Connection.getSchema() and Connection.setSchema(). I am running regression tests now.

          This patch takes advantage of existing shortcuts for getSchema() in both the embedded and network cases. The setSchema() method has more error paths so I opted for implementing it on top of the SQL layer. We may want to revisit this implementation if it causes a performance drag in the wild.

          Touches the following files:

          -----------

          M java/engine/org/apache/derby/impl/jdbc/EmbedConnection.java
          M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java
          M java/engine/org/apache/derby/iapi/jdbc/EngineConnection.java

          Embedded implementation.

          -----------

          M java/client/org/apache/derby/client/am/Connection.java
          M java/client/org/apache/derby/client/am/LogicalConnection.java

          Network implementation.

          -----------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Conn.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionMethodsTest.java

          Tests.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-18-aa-getSetSchema.diff. This patch implements the new JDBC 4.1 methods Connection.getSchema() and Connection.setSchema(). I am running regression tests now. This patch takes advantage of existing shortcuts for getSchema() in both the embedded and network cases. The setSchema() method has more error paths so I opted for implementing it on top of the SQL layer. We may want to revisit this implementation if it causes a performance drag in the wild. Touches the following files: ----------- M java/engine/org/apache/derby/impl/jdbc/EmbedConnection.java M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java M java/engine/org/apache/derby/iapi/jdbc/EngineConnection.java Embedded implementation. ----------- M java/client/org/apache/derby/client/am/Connection.java M java/client/org/apache/derby/client/am/LogicalConnection.java Network implementation. ----------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Conn.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionMethodsTest.java Tests.
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me derby-4869-18-aa-getSetSchema.diff except for the Heisenbug in InvalidLDAPServerAuthenticationTest.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me derby-4869-18-aa-getSetSchema.diff except for the Heisenbug in InvalidLDAPServerAuthenticationTest.
          Hide
          Rick Hillegas added a comment -

          Committed derby-4869-18-aa-getSetSchema.diff at subversion revision 1063723.

          Show
          Rick Hillegas added a comment - Committed derby-4869-18-aa-getSetSchema.diff at subversion revision 1063723.
          Hide
          Knut Anders Hatlen added a comment -

          On the client side, the connection has the current schema cached in most cases. Would it make sense to let the client version of setSchema() check the cached value if it exists and do nothing if the new schema is the same as the old one? The use case I'm thinking about is applications that start each chunk of work with setting the schema.

          And some nits:

          • The try-finally-close code in the setSchema() methods could be simplified by moving the call to prepareStatement() out of the try block and up to the declaration of the ps variable. Then the ps could be closed unconditionally in the finally block.
          • The new methods are indented with a mix of tabs and blanks.
          Show
          Knut Anders Hatlen added a comment - On the client side, the connection has the current schema cached in most cases. Would it make sense to let the client version of setSchema() check the cached value if it exists and do nothing if the new schema is the same as the old one? The use case I'm thinking about is applications that start each chunk of work with setting the schema. And some nits: The try-finally-close code in the setSchema() methods could be simplified by moving the call to prepareStatement() out of the try block and up to the declaration of the ps variable. Then the ps could be closed unconditionally in the finally block. The new methods are indented with a mix of tabs and blanks.
          Hide
          Rick Hillegas added a comment -

          Thanks, Knut. Those sound like good improvements to me. I will address them in a follow-on patch.

          Show
          Rick Hillegas added a comment - Thanks, Knut. Those sound like good improvements to me. I will address them in a follow-on patch.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-19-aa-getSetNetworkTimeout.diff. This patch provides stubs which raise SQLFeatureNotSupportedException for the optional Connection.getNetworkTimeout() and setNetworkTimeout() methods added by JDBC 4.1. I will run regression tests.

          Touches the following files:

          -----------

          M java/engine/org/apache/derby/impl/jdbc/EmbedConnection40.java
          M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection40.java

          Embedded stubs.

          -----------

          M java/client/org/apache/derby/client/net/NetConnection40.java
          M java/client/org/apache/derby/client/am/LogicalConnection40.java

          Network stubs.

          -----------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Conn.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionMethodsTest.java

          Tests.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-19-aa-getSetNetworkTimeout.diff. This patch provides stubs which raise SQLFeatureNotSupportedException for the optional Connection.getNetworkTimeout() and setNetworkTimeout() methods added by JDBC 4.1. I will run regression tests. Touches the following files: ----------- M java/engine/org/apache/derby/impl/jdbc/EmbedConnection40.java M java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection40.java Embedded stubs. ----------- M java/client/org/apache/derby/client/net/NetConnection40.java M java/client/org/apache/derby/client/am/LogicalConnection40.java Network stubs. ----------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Conn.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionMethodsTest.java Tests.
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-4869-19-aa-getSetNetworkTimeout.diff. Committed at subversion revision 1063822.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-4869-19-aa-getSetNetworkTimeout.diff. Committed at subversion revision 1063822.
          Hide
          Rick Hillegas added a comment - - edited

          Attaching derby-4869-20-aa-setSchemaFastPath.diff. This adds the optimization Knut suggested: In the network case, don't bother trying to change the schema if session data is being cached and the old schema name is the same as the new name passed to Connection.setSchema(). Committed at subversion revision 1063831.

          Touches the following file:

          M java/client/org/apache/derby/client/am/Connection.java

          Show
          Rick Hillegas added a comment - - edited Attaching derby-4869-20-aa-setSchemaFastPath.diff. This adds the optimization Knut suggested: In the network case, don't bother trying to change the schema if session data is being cached and the old schema name is the same as the new name passed to Connection.setSchema(). Committed at subversion revision 1063831. Touches the following file: M java/client/org/apache/derby/client/am/Connection.java
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-22-aa-unstableStatementTest.diff. This patch attempts to reduce the risk of instability in StatementTest.test_jdbc4_1_queryTimeoutException(). Committed at subversion revision 1064281.

          The test case tries to force a query timeout. The test failed to induce a timeout on the tinderbox test run at 2011-01-27 17:12:14 CET: http://dbtg.foundry.sun.com/derby/test/tinderbox_trunk16/jvm1.6/testing/testlog/SunOS-5.10_i86pc-i386/1064174-org.apache.derbyTesting.functionTests.suites.All_diff.txt

          This patch retries the timeout-inducing query ten times, hoping to force a timeout on one of those runs. If the test continues to fail, I will have to do something smarter.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-4869-22-aa-unstableStatementTest.diff. This patch attempts to reduce the risk of instability in StatementTest.test_jdbc4_1_queryTimeoutException(). Committed at subversion revision 1064281. The test case tries to force a query timeout. The test failed to induce a timeout on the tinderbox test run at 2011-01-27 17:12:14 CET: http://dbtg.foundry.sun.com/derby/test/tinderbox_trunk16/jvm1.6/testing/testlog/SunOS-5.10_i86pc-i386/1064174-org.apache.derbyTesting.functionTests.suites.All_diff.txt This patch retries the timeout-inducing query ten times, hoping to force a timeout on one of those runs. If the test continues to fail, I will have to do something smarter. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementTest.java
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-21-aa-implicitlyClosedResultSets.diff. This patch causes closeOnCompletion() Statements to close when their forward-only ResultSets implicitly close after being drained in autocommit mode.

          The change to EmbeddedResultSet seemed straightforward.

          I have misgivings about the change to the network ResultSet. I think that the Statement-closing trigger should be placed in the markClosed() method. However, when I put the trigger there, I see protocol errors. It seems that that method is called from inside the network protocol itself. That looks like a layering problem to me: I don't think that the network layer should be calling back up into the JDBC layer. Fixing that layering problem looks like a mini-project outside the scope of the current issue.

          Touches the following files:

          -------------

          M java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java

          Change to embedded behavior.

          -------------

          M java/client/org/apache/derby/client/am/ResultSet.java

          Change to network behavior.

          -------------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementTest.java

          Test to verify Statement closure when a ResultSet implicitly closes.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-21-aa-implicitlyClosedResultSets.diff. This patch causes closeOnCompletion() Statements to close when their forward-only ResultSets implicitly close after being drained in autocommit mode. The change to EmbeddedResultSet seemed straightforward. I have misgivings about the change to the network ResultSet. I think that the Statement-closing trigger should be placed in the markClosed() method. However, when I put the trigger there, I see protocol errors. It seems that that method is called from inside the network protocol itself. That looks like a layering problem to me: I don't think that the network layer should be calling back up into the JDBC layer. Fixing that layering problem looks like a mini-project outside the scope of the current issue. Touches the following files: ------------- M java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java Change to embedded behavior. ------------- M java/client/org/apache/derby/client/am/ResultSet.java Change to network behavior. ------------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementTest.java Test to verify Statement closure when a ResultSet implicitly closes.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-23-aa-dbmd.diff. This patch makes the JDBC 4.1 changes to DatabaseMetaData. I am running regression tests now.

          This patch makes the following changes to Derby's implementations of DatabaseMetaData:

          1) Adds generatedKeyAlwaysReturned(), which returns true.

          2) Adds getPseudoColumns(), which returns an empty ResultSet.

          3) Adds an IS_GENERATEDCOLUMN column to the ResultSet returned by getColumns(). This column is "YES" for columns defined by generation clauses and "NO" otherwise.

          Touches the following files:

          -------------

          M java/engine/org/apache/derby/impl/jdbc/metadata.properties

          New query for getPseudoColumn() and new column added to the getColumns() query.

          -------------

          M java/engine/org/apache/derby/impl/jdbc/EmbedDatabaseMetaData.java

          New bits in the embedded metadata.

          -------------

          M java/client/org/apache/derby/client/am/DatabaseMetaData.java

          New bits in the network metadata.

          -------------

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/ViewsTest.java
          A java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/Wrapper41DBMD.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/DatabaseMetaDataTest.java

          Tests.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-23-aa-dbmd.diff. This patch makes the JDBC 4.1 changes to DatabaseMetaData. I am running regression tests now. This patch makes the following changes to Derby's implementations of DatabaseMetaData: 1) Adds generatedKeyAlwaysReturned(), which returns true. 2) Adds getPseudoColumns(), which returns an empty ResultSet. 3) Adds an IS_GENERATEDCOLUMN column to the ResultSet returned by getColumns(). This column is "YES" for columns defined by generation clauses and "NO" otherwise. Touches the following files: ------------- M java/engine/org/apache/derby/impl/jdbc/metadata.properties New query for getPseudoColumn() and new column added to the getColumns() query. ------------- M java/engine/org/apache/derby/impl/jdbc/EmbedDatabaseMetaData.java New bits in the embedded metadata. ------------- M java/client/org/apache/derby/client/am/DatabaseMetaData.java New bits in the network metadata. ------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/ViewsTest.java A java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/Wrapper41DBMD.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/DatabaseMetaDataTest.java Tests.
          Hide
          Dag H. Wanvik added a comment -

          >I have misgivings about the change to the network ResultSet. I think that the Statement-closing trigger should be placed in the markClosed() method. However, when I put the trigger there, I see protocol errors. It seems that that method is called from inside the network protocol itself.

          How does markClosed get called from inside the network layer? I searched for usages there, but I didn't see any..
          I found usages from Connection, ResultSet, Statement and PrepareStatement (all on am level)..

          Show
          Dag H. Wanvik added a comment - >I have misgivings about the change to the network ResultSet. I think that the Statement-closing trigger should be placed in the markClosed() method. However, when I put the trigger there, I see protocol errors. It seems that that method is called from inside the network protocol itself. How does markClosed get called from inside the network layer? I searched for usages there, but I didn't see any.. I found usages from Connection, ResultSet, Statement and PrepareStatement (all on am level)..
          Hide
          Rick Hillegas added a comment -

          Thanks for helping me puzzle through this, Dag. Here is the stack trace I saw:

          at java.lang.Thread.dumpStack(Thread.java:1230)
          at org.apache.derby.client.am.ResultSet.markClosed(ResultSet.java:4420)
          at org.apache.derby.client.am.ResultSet.markClosed(ResultSet.java:4408)
          at org.apache.derby.client.am.ResultSet.completeLocalCommit(ResultSet.java:4365)
          at org.apache.derby.client.am.Connection.completeLocalCommit(Connection.java:2026)
          at org.apache.derby.client.net.NetConnectionReply.parseENDUOWRM(NetConnectionReply.java:789)
          at org.apache.derby.client.net.NetConnectionReply.parseRDBCMMreply(NetConnectionReply.java:198)
          at org.apache.derby.client.net.NetConnectionReply.readLocalCommit(NetConnectionReply.java:135)
          at org.apache.derby.client.net.ConnectionReply.readLocalCommit(ConnectionReply.java:43)
          at org.apache.derby.client.net.NetConnection.readLocalCommit_(NetConnection.java:1507)
          at org.apache.derby.client.am.Connection.readCommit(Connection.java:640)
          at org.apache.derby.client.am.Connection.flowCommit(Connection.java:589)
          at org.apache.derby.client.am.Connection.flowAutoCommit(Connection.java:598)
          at org.apache.derby.client.am.Statement.resultSetCommitting(Statement.java:3009)
          at org.apache.derby.client.am.Statement.resultSetCommitting(Statement.java:2969)
          at org.apache.derby.client.am.ResultSet.nextX(ResultSet.java:361)
          at org.apache.derby.client.am.ResultSet.next(ResultSet.java:282)

          Show
          Rick Hillegas added a comment - Thanks for helping me puzzle through this, Dag. Here is the stack trace I saw: at java.lang.Thread.dumpStack(Thread.java:1230) at org.apache.derby.client.am.ResultSet.markClosed(ResultSet.java:4420) at org.apache.derby.client.am.ResultSet.markClosed(ResultSet.java:4408) at org.apache.derby.client.am.ResultSet.completeLocalCommit(ResultSet.java:4365) at org.apache.derby.client.am.Connection.completeLocalCommit(Connection.java:2026) at org.apache.derby.client.net.NetConnectionReply.parseENDUOWRM(NetConnectionReply.java:789) at org.apache.derby.client.net.NetConnectionReply.parseRDBCMMreply(NetConnectionReply.java:198) at org.apache.derby.client.net.NetConnectionReply.readLocalCommit(NetConnectionReply.java:135) at org.apache.derby.client.net.ConnectionReply.readLocalCommit(ConnectionReply.java:43) at org.apache.derby.client.net.NetConnection.readLocalCommit_(NetConnection.java:1507) at org.apache.derby.client.am.Connection.readCommit(Connection.java:640) at org.apache.derby.client.am.Connection.flowCommit(Connection.java:589) at org.apache.derby.client.am.Connection.flowAutoCommit(Connection.java:598) at org.apache.derby.client.am.Statement.resultSetCommitting(Statement.java:3009) at org.apache.derby.client.am.Statement.resultSetCommitting(Statement.java:2969) at org.apache.derby.client.am.ResultSet.nextX(ResultSet.java:361) at org.apache.derby.client.am.ResultSet.next(ResultSet.java:282)
          Hide
          Dag H. Wanvik added a comment -

          Right, indirect call then. So net layer is calling up into am in reply to a client commit request which was acknowledged by the server (case CodePoint.RDBCMM, so the client needs to mark its result set closed, too. Sounds like a typical "callback" action to me. Not sure how you could avoid that as long as state is kept on the am level, too.

          Show
          Dag H. Wanvik added a comment - Right, indirect call then. So net layer is calling up into am in reply to a client commit request which was acknowledged by the server (case CodePoint.RDBCMM , so the client needs to mark its result set closed, too. Sounds like a typical "callback" action to me. Not sure how you could avoid that as long as state is kept on the am level, too.
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on the JDBC 4.1 changes to DatabaseMetaData. Committed derby-4869-23-aa-dbmd.diff at subversion revision 1064868.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on the JDBC 4.1 changes to DatabaseMetaData. Committed derby-4869-23-aa-dbmd.diff at subversion revision 1064868.
          Hide
          Dag H. Wanvik added a comment - - edited

          Am I correct that this test code should work as well? It does work on embedded, but fails in c/s..

          conn.setAutoCommit( false );

          ps = conn.prepareStatement( "values ( 1 )" );
          println( "Testing implicit closure on a " +
          ps.getClass().getName() + " with autocommit==false");
          wrapper = new Wrapper41Statement( ps );
          wrapper.closeOnCompletion();
          rs = ps.executeQuery();
          rs.next();
          rs.next();
          assertFalse( rs.isClosed() );
          assertFalse( ps.isClosed() );
          conn.commit();
          assertTrue( rs.isClosed() );
          assertTrue( ps.isClosed() ); <---- FAILS in C/S mode

          Show
          Dag H. Wanvik added a comment - - edited Am I correct that this test code should work as well? It does work on embedded, but fails in c/s.. conn.setAutoCommit( false ); ps = conn.prepareStatement( "values ( 1 )" ); println( "Testing implicit closure on a " + ps.getClass().getName() + " with autocommit==false"); wrapper = new Wrapper41Statement( ps ); wrapper.closeOnCompletion(); rs = ps.executeQuery(); rs.next(); rs.next(); assertFalse( rs.isClosed() ); assertFalse( ps.isClosed() ); conn.commit(); assertTrue( rs.isClosed() ); assertTrue( ps.isClosed() ); <---- FAILS in C/S mode
          Hide
          Rick Hillegas added a comment -

          Thanks for that problem case, Dag. I am able to reproduce your results when I also direct the Connection to close cursors at commit time:

          conn.setHoldability( ResultSet.CLOSE_CURSORS_AT_COMMIT );

          Without that directive, the cursors and the statement remain open (that seems correct).

          I agree that the client/server behavior looks wrong when CLOSE_CURSORS_AT_COMMIT is set. Thanks.

          Show
          Rick Hillegas added a comment - Thanks for that problem case, Dag. I am able to reproduce your results when I also direct the Connection to close cursors at commit time: conn.setHoldability( ResultSet.CLOSE_CURSORS_AT_COMMIT ); Without that directive, the cursors and the statement remain open (that seems correct). I agree that the client/server behavior looks wrong when CLOSE_CURSORS_AT_COMMIT is set. Thanks.
          Hide
          Rick Hillegas added a comment -

          Thanks for that problem case, Dag. I am able to reproduce your results when I direct the Connection to close cursors at commit time:

          conn.setHoldability( ResultSet.CLOSE_CURSORS_AT_COMMIT );

          Without that directive, the cursors and the statement remain open (that seems correct).

          I agree that the client/server behavior looks wrong when CLOSE_CURSORS_AT_COMMIT is set. Thanks.

          Show
          Rick Hillegas added a comment - Thanks for that problem case, Dag. I am able to reproduce your results when I direct the Connection to close cursors at commit time: conn.setHoldability( ResultSet.CLOSE_CURSORS_AT_COMMIT ); Without that directive, the cursors and the statement remain open (that seems correct). I agree that the client/server behavior looks wrong when CLOSE_CURSORS_AT_COMMIT is set. Thanks.
          Hide
          Dag H. Wanvik added a comment -

          Right, sorry I forgot to specify holdability, you are correct of course. I think we need somehow to move to let statement be driven from the actual rs/cursor being closed as you tried initially..?

          Show
          Dag H. Wanvik added a comment - Right, sorry I forgot to specify holdability, you are correct of course. I think we need somehow to move to let statement be driven from the actual rs/cursor being closed as you tried initially..?
          Hide
          Rick Hillegas added a comment -

          Thanks for the encouragement, Dag. Attaching derby-4869-21-ab-implicitlyClosedResultSets.diff. This is a second rev of the patch for handling closeOnCompletion() when ResultSets close implicitly. I am running regression tests now.

          In this version of the patch, the Statement-closing logic is moved inside ResultSet.markClosed(). I found that I could make this work by factoring Statement.markClosed() into 2 pieces:

          1) A piece which closes the ResultSets.

          2) A piece which closes all of the other Statement resources.

          Now the closeOnCompletion logic calls only the second piece of the Statement.markClosed() logic. This ought to be safe since the ResultSets have already been closed.

          I have added Dag's test case to the regression test for this behavior.

          Touches the same files as the previous version of the patch.

          Show
          Rick Hillegas added a comment - Thanks for the encouragement, Dag. Attaching derby-4869-21-ab-implicitlyClosedResultSets.diff. This is a second rev of the patch for handling closeOnCompletion() when ResultSets close implicitly. I am running regression tests now. In this version of the patch, the Statement-closing logic is moved inside ResultSet.markClosed(). I found that I could make this work by factoring Statement.markClosed() into 2 pieces: 1) A piece which closes the ResultSets. 2) A piece which closes all of the other Statement resources. Now the closeOnCompletion logic calls only the second piece of the Statement.markClosed() logic. This ought to be safe since the ResultSets have already been closed. I have added Dag's test case to the regression test for this behavior. Touches the same files as the previous version of the patch.
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-4869-21-ab-implicitlyClosedResultSets.diff. Committed at subversion revision 1066127.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-4869-21-ab-implicitlyClosedResultSets.diff. Committed at subversion revision 1066127.
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Rick! Looks like a good solution to me. +1
          Nit: some lines > 80 columns.

          Show
          Dag H. Wanvik added a comment - Thanks, Rick! Looks like a good solution to me. +1 Nit: some lines > 80 columns.
          Hide
          Knut Anders Hatlen added a comment -

          The set/getNetworkTimeout patch makes the new unsupported methods throw different exceptions depending on whether or not the connection is closed. Other methods that we don't implement throw SQLFeatureNotSupportedException unconditionally, see for example createArrayOf(), createNClob(), createSQLXML() and createStruct() in the Connection classes. For consistency, we should probably make setNetworkTimeout() and getNetworkTimeout() do the same.

          Show
          Knut Anders Hatlen added a comment - The set/getNetworkTimeout patch makes the new unsupported methods throw different exceptions depending on whether or not the connection is closed. Other methods that we don't implement throw SQLFeatureNotSupportedException unconditionally, see for example createArrayOf(), createNClob(), createSQLXML() and createStruct() in the Connection classes. For consistency, we should probably make setNetworkTimeout() and getNetworkTimeout() do the same.
          Hide
          Rick Hillegas added a comment -

          Thanks, Knut. I will confirm with Lance Andersen about whether the Derby behavior for the other methods is acceptable. If so, then I agree that consistency is a good idea.

          Show
          Rick Hillegas added a comment - Thanks, Knut. I will confirm with Lance Andersen about whether the Derby behavior for the other methods is acceptable. If so, then I agree that consistency is a good idea.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-24-ab-getParentLogger.diff. This patch adds the new getParentLogger() method to Derby's JDBC 4.0 implementations of java.sql.Driver and javax.sql.CommonDataSource. I will run regression tests.

          For the moment, these methods throw SQLFeatureNotSupportedException. Some day we may decide to implement JDBC support for the Logger idiom.

          The two drivers which we actually register with DriverManager are AutoloadedDriver (in the embedded case) and ClientDriver (in the client/server case). The new method could not be put into these existing classes because the new method must raise a Java 6 exception. This patch introduces subclasses of these drivers for use on Java 6 and higher platforms. A little static inititialization sorts out which drivers to load based on the platform.

          Touches the following files:

          --------------

          M java/engine/org/apache/derby/jdbc/AutoloadedDriver.java
          A java/engine/org/apache/derby/jdbc/AutoloadedDriver40.java
          M tools/jar/extraDBMSclasses.properties

          Changes to the embedded driver.

          --------------

          M java/client/org/apache/derby/jdbc/ClientDriver.java
          A java/client/org/apache/derby/jdbc/ClientDriver40.java
          M tools/jar/dnc.properties

          Changes to the client/server driver.

          --------------

          M java/engine/org/apache/derby/jdbc/EmbeddedXADataSource40.java
          M java/engine/org/apache/derby/jdbc/EmbeddedDataSource40.java
          M java/engine/org/apache/derby/jdbc/EmbeddedConnectionPoolDataSource40.java

          Changes to the embedded DataSources.

          --------------

          M java/client/org/apache/derby/jdbc/ClientXADataSource40.java
          M java/client/org/apache/derby/jdbc/ClientConnectionPoolDataSource40.java
          M java/client/org/apache/derby/jdbc/ClientDataSource40.java

          Changes to the client/server DataSources

          --------------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/_Suite.java
          A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Driver40Test.java
          A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41DataSource.java
          A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Driver.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/DataSourceTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/DriverTest.java

          New tests.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-24-ab-getParentLogger.diff. This patch adds the new getParentLogger() method to Derby's JDBC 4.0 implementations of java.sql.Driver and javax.sql.CommonDataSource. I will run regression tests. For the moment, these methods throw SQLFeatureNotSupportedException. Some day we may decide to implement JDBC support for the Logger idiom. The two drivers which we actually register with DriverManager are AutoloadedDriver (in the embedded case) and ClientDriver (in the client/server case). The new method could not be put into these existing classes because the new method must raise a Java 6 exception. This patch introduces subclasses of these drivers for use on Java 6 and higher platforms. A little static inititialization sorts out which drivers to load based on the platform. Touches the following files: -------------- M java/engine/org/apache/derby/jdbc/AutoloadedDriver.java A java/engine/org/apache/derby/jdbc/AutoloadedDriver40.java M tools/jar/extraDBMSclasses.properties Changes to the embedded driver. -------------- M java/client/org/apache/derby/jdbc/ClientDriver.java A java/client/org/apache/derby/jdbc/ClientDriver40.java M tools/jar/dnc.properties Changes to the client/server driver. -------------- M java/engine/org/apache/derby/jdbc/EmbeddedXADataSource40.java M java/engine/org/apache/derby/jdbc/EmbeddedDataSource40.java M java/engine/org/apache/derby/jdbc/EmbeddedConnectionPoolDataSource40.java Changes to the embedded DataSources. -------------- M java/client/org/apache/derby/jdbc/ClientXADataSource40.java M java/client/org/apache/derby/jdbc/ClientConnectionPoolDataSource40.java M java/client/org/apache/derby/jdbc/ClientDataSource40.java Changes to the client/server DataSources -------------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/_Suite.java A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Driver40Test.java A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41DataSource.java A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Driver.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/DataSourceTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/DriverTest.java New tests.
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-4869-24-ab-getParentLogger.diff except for known heisenbugs in testPing and InvalidLDAPServerAuthenticationTest.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-4869-24-ab-getParentLogger.diff except for known heisenbugs in testPing and InvalidLDAPServerAuthenticationTest.
          Hide
          Rick Hillegas added a comment -

          Committed derby-4869-24-ab-getParentLogger.diff at subversion revision 1067954.

          Show
          Rick Hillegas added a comment - Committed derby-4869-24-ab-getParentLogger.diff at subversion revision 1067954.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Rick,

          The following code causes a ClassCastException in my environment:

          Driver d1 = DriverManager.getDriver("jdbc:derby:db");
          System.out.println(d1);
          try

          { d1.getParentLogger(); System.out.println("what?!?"); }

          catch (SQLFeatureNotSupportedException ex)

          { // expected }

          Exception in thread "main" java.lang.ClassCastException: org.apache.derby.impl.jdbc.EmbedSQLException cannot be cast to java.sql.SQLFeatureNotSupportedException
          at org.apache.derby.jdbc.AutoloadedDriver40.getParentLogger(AutoloadedDriver40.java:48)
          at Drv41.main(Drv41.java:7)

          It does not fail if I add Class.forName("org.apache.derby.jdbc.EmbeddedDriver") before the call to DriverManager.getDriver(), only when I use the auto-loaded driver.

          Show
          Knut Anders Hatlen added a comment - Hi Rick, The following code causes a ClassCastException in my environment: Driver d1 = DriverManager.getDriver("jdbc:derby:db"); System.out.println(d1); try { d1.getParentLogger(); System.out.println("what?!?"); } catch (SQLFeatureNotSupportedException ex) { // expected } Exception in thread "main" java.lang.ClassCastException: org.apache.derby.impl.jdbc.EmbedSQLException cannot be cast to java.sql.SQLFeatureNotSupportedException at org.apache.derby.jdbc.AutoloadedDriver40.getParentLogger(AutoloadedDriver40.java:48) at Drv41.main(Drv41.java:7) It does not fail if I add Class.forName("org.apache.derby.jdbc.EmbeddedDriver") before the call to DriverManager.getDriver(), only when I use the auto-loaded driver.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-25-aa-removeClosureCheck.diff. This patch makes Connection.get/setNetworkTimeout() always raise a SQLFeatureNotSupportedException, even when the Connection is closed. Committed at subversion revision 1067991.

          I have confirmed with Lance Andersen that this behavior is acceptable. It makes the behavior consistent with other Connection methods which raise SQLFeatureNotSupportedException.

          Touches the following files:

          --------------

          M java/engine/org/apache/derby/impl/jdbc/EmbedConnection40.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionMethodsTest.java
          M java/client/org/apache/derby/client/net/NetConnection40.java

          Show
          Rick Hillegas added a comment - Attaching derby-4869-25-aa-removeClosureCheck.diff. This patch makes Connection.get/setNetworkTimeout() always raise a SQLFeatureNotSupportedException, even when the Connection is closed. Committed at subversion revision 1067991. I have confirmed with Lance Andersen that this behavior is acceptable. It makes the behavior consistent with other Connection methods which raise SQLFeatureNotSupportedException. Touches the following files: -------------- M java/engine/org/apache/derby/impl/jdbc/EmbedConnection40.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionMethodsTest.java M java/client/org/apache/derby/client/net/NetConnection40.java
          Hide
          Knut Anders Hatlen added a comment -

          Another related issue. If I unload the embedded driver and reload it using Class.forName(...).newInstance(), the new driver is not the more capable one, and an attempt to call getParentLogger() results in an AbstractMethodError.

          See the attached program Drv41.java, which fails with this stack trace:

          Exception in thread "main" java.lang.AbstractMethodError
          at Drv41.main(Drv41.java:26)

          Show
          Knut Anders Hatlen added a comment - Another related issue. If I unload the embedded driver and reload it using Class.forName(...).newInstance(), the new driver is not the more capable one, and an attempt to call getParentLogger() results in an AbstractMethodError. See the attached program Drv41.java, which fails with this stack trace: Exception in thread "main" java.lang.AbstractMethodError at Drv41.main(Drv41.java:26)
          Hide
          Rick Hillegas added a comment -

          Thanks, Knut. I will take a look at those issues.

          Show
          Rick Hillegas added a comment - Thanks, Knut. I will take a look at those issues.
          Hide
          Knut Anders Hatlen added a comment -

          The fix for the instability in StatementTest.test_jdbc4_1_queryTimeoutException() doesn't seem to be quite enough. It's still failing sometimes in the nightly tests: http://dbtg.foundry.sun.com/derby/test/Daily/jvm1.6/testing/testlog/sol32/1067702-suitesAll_diff.txt

          Show
          Knut Anders Hatlen added a comment - The fix for the instability in StatementTest.test_jdbc4_1_queryTimeoutException() doesn't seem to be quite enough. It's still failing sometimes in the nightly tests: http://dbtg.foundry.sun.com/derby/test/Daily/jvm1.6/testing/testlog/sol32/1067702-suitesAll_diff.txt
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-27-aa-driver40.diff. This patch adds getParentLogger() to Driver40. On my machine this fixes the problem which surfaced in the tinderbox tests after committing derby-4869-24-ab-getParentLogger.diff. Committed at subversion revision 1068073.

          Touches the following files:

          --------------

          M java/engine/org/apache/derby/jdbc/Driver40.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Driver.java

          Show
          Rick Hillegas added a comment - Attaching derby-4869-27-aa-driver40.diff. This patch adds getParentLogger() to Driver40. On my machine this fixes the problem which surfaced in the tinderbox tests after committing derby-4869-24-ab-getParentLogger.diff. Committed at subversion revision 1068073. Touches the following files: -------------- M java/engine/org/apache/derby/jdbc/Driver40.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Wrapper41Driver.java
          Hide
          Rick Hillegas added a comment -

          The second issue raised by Knut (AbstractMethodError thrown after reloading the engine) is fixed by my previous submission (the addition of getParentLogger() to Driver40). It seems that the work on DERBY-2905 has resulted in a Driver40 being registered when you reload the engine. Before the work on DERBY-2905, the engine reload would re-register AutoloadedDriver.

          Show
          Rick Hillegas added a comment - The second issue raised by Knut (AbstractMethodError thrown after reloading the engine) is fixed by my previous submission (the addition of getParentLogger() to Driver40). It seems that the work on DERBY-2905 has resulted in a Driver40 being registered when you reload the engine. Before the work on DERBY-2905 , the engine reload would re-register AutoloadedDriver.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-28-ab-autoloadExceptionFactory.diff. This fixes the other problem found by Knut: If the embedded driver has not booted, then the JDBC 4.0 exception factory is not installed and getParentLogger() trips over a ClassCastException because the refined JDBC 4.0 exception classes aren't being used. Committed at subversion revision 1068451.

          Touches the following files:

          --------------

          M java/engine/org/apache/derby/jdbc/AutoloadedDriver40.java

          Loads the JDBC 4.0 exception factory on initialization.

          --------------

          A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Driver40UnbootedTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/_Suite.java

          Adds a test for this case.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-28-ab-autoloadExceptionFactory.diff. This fixes the other problem found by Knut: If the embedded driver has not booted, then the JDBC 4.0 exception factory is not installed and getParentLogger() trips over a ClassCastException because the refined JDBC 4.0 exception classes aren't being used. Committed at subversion revision 1068451. Touches the following files: -------------- M java/engine/org/apache/derby/jdbc/AutoloadedDriver40.java Loads the JDBC 4.0 exception factory on initialization. -------------- A java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/Driver40UnbootedTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/_Suite.java Adds a test for this case.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-26-aa-signatureTests.diff. This patch uncomments the signature-matching tests on Java 7. Now that all of the JDBC 4.1 methods have been added, those tests should pass. Committed at subversion revision 1068489.

          Touches the following files:

          --------------

          M java/client/org/apache/derby/client/am/LogicalDatabaseMetaData40.java

          The tests showed that this wrapper still needed to forward new methods to the embedded DatabaseMetaData.

          --------------

          M java/client/org/apache/derby/client/net/NetResultSet40.java

          The tests objected that the text of the error message for ResultSet.getObject( int, Class ) was not what was expected even though the SQLState was correct.

          --------------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/VerifySignatures.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/UnsupportedVetter.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/ClosedObjectTest.java

          Uncommented these tests on Java 7.

          Show
          Rick Hillegas added a comment - Attaching derby-4869-26-aa-signatureTests.diff. This patch uncomments the signature-matching tests on Java 7. Now that all of the JDBC 4.1 methods have been added, those tests should pass. Committed at subversion revision 1068489. Touches the following files: -------------- M java/client/org/apache/derby/client/am/LogicalDatabaseMetaData40.java The tests showed that this wrapper still needed to forward new methods to the embedded DatabaseMetaData. -------------- M java/client/org/apache/derby/client/net/NetResultSet40.java The tests objected that the text of the error message for ResultSet.getObject( int, Class ) was not what was expected even though the SQLState was correct. -------------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/VerifySignatures.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/UnsupportedVetter.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/ClosedObjectTest.java Uncommented these tests on Java 7.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-29-aa-fixAutoloadTest.diff. This patch makes the AutoloadTest aware of the new driver classes introduced by the work on getParentLogger(). This fixes the test failures seen in AutoloadTest here: http://dbtg.foundry.sun.com/derby/test/Daily/jvm1.6/testing/Limited/testSummary-1068013.html

          Committed at subversion revision 1068524.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/AutoloadTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-4869-29-aa-fixAutoloadTest.diff. This patch makes the AutoloadTest aware of the new driver classes introduced by the work on getParentLogger(). This fixes the test failures seen in AutoloadTest here: http://dbtg.foundry.sun.com/derby/test/Daily/jvm1.6/testing/Limited/testSummary-1068013.html Committed at subversion revision 1068524. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/AutoloadTest.java
          Hide
          Rick Hillegas added a comment -

          Hi Knut: Concerning the query timeout failures on Solaris, has the frequency of the failures gone down? Thanks.

          Show
          Rick Hillegas added a comment - Hi Knut: Concerning the query timeout failures on Solaris, has the frequency of the failures gone down? Thanks.
          Hide
          Knut Anders Hatlen added a comment -

          The query timeout test has failed in the last two test cycles on both Solaris 10 x86 and Windows 2003. In the last test cycle it also failed on SUSE and Solaris 10 SPARC. So I don't think the frequency of the failures has gone down much.

          Show
          Knut Anders Hatlen added a comment - The query timeout test has failed in the last two test cycles on both Solaris 10 x86 and Windows 2003. In the last test cycle it also failed on SUSE and Solaris 10 SPARC. So I don't think the frequency of the failures has gone down much.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4869-30-aa-unstableStatementTest.diff. This is a second attempt to remove the instability in StatementTest. Committed at subversion revision 1069070.

          This patch changes the test in 2 ways:

          1) Doubles the number of attempts to force a timeout.

          2) Just soldiers on if a timeout could not be forced.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-4869-30-aa-unstableStatementTest.diff. This is a second attempt to remove the instability in StatementTest. Committed at subversion revision 1069070. This patch changes the test in 2 ways: 1) Doubles the number of attempts to force a timeout. 2) Just soldiers on if a timeout could not be forced. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementTest.java
          Hide
          Knut Anders Hatlen added a comment -

          Maybe this comment in SetQueryTimeoutTest gives a hint as to how we can stabilize the test?

          /**

          • The reason for using the mod function here is to force
          • at least one invocation of ResultSet.next() to read
          • more than one row from the table before returning.
          • This is necessary since timeout is checked only when
          • reading rows from base tables, and when the first row
          • is read, the query still has not exceeded the timeout.
            */
            return "select a from " + tablePrefix + "_orig where mod(DELAY(1,a),3)=0";
          Show
          Knut Anders Hatlen added a comment - Maybe this comment in SetQueryTimeoutTest gives a hint as to how we can stabilize the test? /** The reason for using the mod function here is to force at least one invocation of ResultSet.next() to read more than one row from the table before returning. This is necessary since timeout is checked only when reading rows from base tables, and when the first row is read, the query still has not exceeded the timeout. */ return "select a from " + tablePrefix + "_orig where mod(DELAY(1,a),3)=0";
          Hide
          Rick Hillegas added a comment -

          Hi Knut,

          Could you test-drive derby-4869-31-aa-unstableStatementTest.diff on the platform where StatementTest has been failing? This patch follows the clue from your last comment and rewrites the query to return more than one row. Thanks.

          Show
          Rick Hillegas added a comment - Hi Knut, Could you test-drive derby-4869-31-aa-unstableStatementTest.diff on the platform where StatementTest has been failing? This patch follows the clue from your last comment and rewrites the query to return more than one row. Thanks.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Rick,

          On a machine that has shown this failure in the nightly tests (the SUSE box), I ran the test 100 times successfully without your fix, so I'm not sure how to reproduce it easily.

          I'm not sure, though, if the essence of the comment is that the query should return more than one row. My understanding is that the MOD function is used to force one next() call to consider multiple rows before it returns. The MOD function in that query makes next() have to consider three rows because only every third row qualifies.

          It may also be significant that the second argument to the delay function is a reference to a column, so that the function has to be invoked for every row.

          Show
          Knut Anders Hatlen added a comment - Hi Rick, On a machine that has shown this failure in the nightly tests (the SUSE box), I ran the test 100 times successfully without your fix, so I'm not sure how to reproduce it easily. I'm not sure, though, if the essence of the comment is that the query should return more than one row. My understanding is that the MOD function is used to force one next() call to consider multiple rows before it returns. The MOD function in that query makes next() have to consider three rows because only every third row qualifies. It may also be significant that the second argument to the delay function is a reference to a column, so that the function has to be invoked for every row.
          Hide
          Rick Hillegas added a comment -

          Thanks, Knut. That sounds like a better interpretation of that comment. Could you test-drive this new patch: derby-4869-31-ab-unstableStatementTest.diff?

          I'm not suprised that StatementTest failed to fail for you: the current version of that test silently succeeds if a timeout can't be forced.

          Thanks.

          Show
          Rick Hillegas added a comment - Thanks, Knut. That sounds like a better interpretation of that comment. Could you test-drive this new patch: derby-4869-31-ab-unstableStatementTest.diff? I'm not suprised that StatementTest failed to fail for you: the current version of that test silently succeeds if a timeout can't be forced. Thanks.
          Hide
          Knut Anders Hatlen added a comment -

          I'm still having difficulties reproducing the test failure after modifying the test to fail if it doesn't get a timeout on the first attempt. On the bright side, I'm having the same difficulties reproducing it with the patch, so at least there's no indication that the patch makes it any worse. It's probably worth checking it in to see if it improves the stability in the nightly runs.

          Show
          Knut Anders Hatlen added a comment - I'm still having difficulties reproducing the test failure after modifying the test to fail if it doesn't get a timeout on the first attempt. On the bright side, I'm having the same difficulties reproducing it with the patch, so at least there's no indication that the patch makes it any worse. It's probably worth checking it in to see if it improves the stability in the nightly runs.
          Hide
          Rick Hillegas added a comment -

          Thanks for test-driving the patch, Knut. I have checked it in at subversion revision 1070504. Crossing my fingers and whistling a happy tune.

          Show
          Rick Hillegas added a comment - Thanks for test-driving the patch, Knut. I have checked it in at subversion revision 1070504. Crossing my fingers and whistling a happy tune.
          Hide
          Kathey Marsden added a comment -

          I have a question about AutoLoadedDriver vs AutoLoadedDriver40.

          I think the autoloaded driver concept was new with JDBC 4.0, so I was curious about why a new AutoLoadedDriver40 was created last week in revision 1067954 [1] in addition to the existing AutoloadedDriver for the getParentLogger() functionality (JDBC 4.1). Isn't any AutoloadedDriver JDBC 4.x? Why do we need two or if we do need two, why isn't the new one AutoLoadedDriver41?

          Thanks

          Kathey

          [1] http://svn.apache.org/viewvc?view=revision&revision=1067954

          Show
          Kathey Marsden added a comment - I have a question about AutoLoadedDriver vs AutoLoadedDriver40. I think the autoloaded driver concept was new with JDBC 4.0, so I was curious about why a new AutoLoadedDriver40 was created last week in revision 1067954 [1] in addition to the existing AutoloadedDriver for the getParentLogger() functionality (JDBC 4.1). Isn't any AutoloadedDriver JDBC 4.x? Why do we need two or if we do need two, why isn't the new one AutoLoadedDriver41? Thanks Kathey [1] http://svn.apache.org/viewvc?view=revision&revision=1067954
          Hide
          Rick Hillegas added a comment -

          Hi Kathey,

          Those are good questions. Before answering them, let me first list a couple facts:

          1) The AutoloadedDriver is the embedded driver which is registered with the DriverManager. When it was introduced, it is what was registered with DriverManager regardless of what VM you were using. It was used for JDK 1.4 on up.

          2) The new getParentLogger() method raises a SQLFeatureNotSupportedException. This exception was introduced in Java 6. A method which raises this exception can't be compiled on Java 5 or lower.

          3) As part of satisfying the JDBC 4.1 contract, when running on Java 7 or higher, the new getParentLogger() method must be found in the driver returned by DriverManager.getDriver( "jdbc:derby" ).

          Two options occurred:

          i) Put getParentLogger() in AutoloadedDriver but don't raise SQLFeatureNotSupportedException. This would mean actually implementing the Logger idiom for writing error messages and diagnostics. I have created DERBY-5007 to track this enhancement. It is an attractive feature but it is a mini-project and not part of JDBC 4.1. If we do get around to implementing DERBY-5007, then getParentLogger() will no longer have to raise SQLFeatureNotSupportedException. We can then move it down to AutoloadedDriver and remove the AutoloadedDriver40 class.

          ii) The other option was to add a new AutoloadedDriver subclass to hold the new getParentLogger() method. This is the option I chose. The new subclass is called AutoloadedDriver40 because it is used if you are running on Java 6 on up (that is at JDBC level 4.0 or greater). This follows the general pattern of our JDBC implementation: When a new rev of the JDBC spec adds a method which COULD run on older VMs, we try to put the new method in the lowest level driver for which it can be compiled. You will notice that a great deal of our JDBC 3 and JDBC 4 functionality was actually pushed down into our JDBC 2 drivers.

          I hope this makes sense. Please ask more questions if it doesn't.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Hi Kathey, Those are good questions. Before answering them, let me first list a couple facts: 1) The AutoloadedDriver is the embedded driver which is registered with the DriverManager. When it was introduced, it is what was registered with DriverManager regardless of what VM you were using. It was used for JDK 1.4 on up. 2) The new getParentLogger() method raises a SQLFeatureNotSupportedException. This exception was introduced in Java 6. A method which raises this exception can't be compiled on Java 5 or lower. 3) As part of satisfying the JDBC 4.1 contract, when running on Java 7 or higher, the new getParentLogger() method must be found in the driver returned by DriverManager.getDriver( "jdbc:derby" ). Two options occurred: i) Put getParentLogger() in AutoloadedDriver but don't raise SQLFeatureNotSupportedException. This would mean actually implementing the Logger idiom for writing error messages and diagnostics. I have created DERBY-5007 to track this enhancement. It is an attractive feature but it is a mini-project and not part of JDBC 4.1. If we do get around to implementing DERBY-5007 , then getParentLogger() will no longer have to raise SQLFeatureNotSupportedException. We can then move it down to AutoloadedDriver and remove the AutoloadedDriver40 class. ii) The other option was to add a new AutoloadedDriver subclass to hold the new getParentLogger() method. This is the option I chose. The new subclass is called AutoloadedDriver40 because it is used if you are running on Java 6 on up (that is at JDBC level 4.0 or greater). This follows the general pattern of our JDBC implementation: When a new rev of the JDBC spec adds a method which COULD run on older VMs, we try to put the new method in the lowest level driver for which it can be compiled. You will notice that a great deal of our JDBC 3 and JDBC 4 functionality was actually pushed down into our JDBC 2 drivers. I hope this makes sense. Please ask more questions if it doesn't. Thanks, -Rick
          Hide
          Knut Anders Hatlen added a comment -

          I guess the confusion here is that AutoloadedDriver isn't necessarily auto-loaded and so the name was a bit misleading from the beginning. With the introduction of the AutoloadedDriver40 class, the name becomes even more misleading, since AutoloadedDriver won't ever be auto-loaded. On platforms that do support auto-loading, it's AutoloadedDriver40 that will be picked, if I've understood correctly.

          So perhaps we should rename AutoloadedDriver to ManuallyLoadedDriver?

          Show
          Knut Anders Hatlen added a comment - I guess the confusion here is that AutoloadedDriver isn't necessarily auto-loaded and so the name was a bit misleading from the beginning. With the introduction of the AutoloadedDriver40 class, the name becomes even more misleading, since AutoloadedDriver won't ever be auto-loaded. On platforms that do support auto-loading, it's AutoloadedDriver40 that will be picked, if I've understood correctly. So perhaps we should rename AutoloadedDriver to ManuallyLoadedDriver?
          Hide
          Rick Hillegas added a comment -

          I don't think the name is so bad: AutoloadedDriver is the driver which is cited in the jar manifest--that gives it a good claim to the name. No argument that there is a lot of confusion in this part of the code though.

          Show
          Rick Hillegas added a comment - I don't think the name is so bad: AutoloadedDriver is the driver which is cited in the jar manifest--that gives it a good claim to the name. No argument that there is a lot of confusion in this part of the code though.
          Hide
          Knut Anders Hatlen added a comment -

          That's a good point. We could probably make the manifest reference AutoloadedDriver40 directly to remove one level of indirection now, but that's doesn't sound very important.

          Show
          Knut Anders Hatlen added a comment - That's a good point. We could probably make the manifest reference AutoloadedDriver40 directly to remove one level of indirection now, but that's doesn't sound very important.
          Hide
          Kathey Marsden added a comment -

          Why are two needed? AutoloadedDriver and AutoloadedDriver40.

          Show
          Kathey Marsden added a comment - Why are two needed? AutoloadedDriver and AutoloadedDriver40.
          Hide
          Rick Hillegas added a comment -

          a) AutoloadedDriver is needed for Java 5 and lower. It can't contain the getParentLogger() method because that method raises an exception which is only present in Java 6 and up.

          b) AutoloadedDriver40 is needed for Java 7 and up. On Java 7, the driver returned by DriverManager.getDriver( "jdbc:derby" ) must implement getParentLogger().

          c) The remaining question is: which class do we use on Java 6? We have followed the existing pattern in the code by running the most capable class possible. Since the signature of getParentLogger() can be compiled on Java 6, we use the more capable version on that platform.

          Does one of the above statements not make sense?

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - a) AutoloadedDriver is needed for Java 5 and lower. It can't contain the getParentLogger() method because that method raises an exception which is only present in Java 6 and up. b) AutoloadedDriver40 is needed for Java 7 and up. On Java 7, the driver returned by DriverManager.getDriver( "jdbc:derby" ) must implement getParentLogger(). c) The remaining question is: which class do we use on Java 6? We have followed the existing pattern in the code by running the most capable class possible. Since the signature of getParentLogger() can be compiled on Java 6, we use the more capable version on that platform. Does one of the above statements not make sense? Thanks, -Rick
          Hide
          Kathey Marsden added a comment -

          I think where I am fundamentally confused why is AutoloadedDriver needed for Java 5 when the Autoloaded Driver feature was not introduced until Java 6?

          Show
          Kathey Marsden added a comment - I think where I am fundamentally confused why is AutoloadedDriver needed for Java 5 when the Autoloaded Driver feature was not introduced until Java 6?
          Hide
          Rick Hillegas added a comment - - edited

          For two reasons:

          I) We simply followed the pattern described in (c) above: JDBC 4.0 did not introduce any methods into java.sql.Driver that prevented implementations from being compiled on JDK 1.4. There was therefore no reason for DriverManager.getDriver() to return 2 different classes, one for Java 5 and lower and another for Java 6 and higher. Now, of course, there is a reason to do that.

          II) Driver autoloading actually goes back to JDK 1.4. It's just that Java 6 introduced a new kind of driver autoloading. And Derby was out of compliance by not supporting the JDK 1.4 version of autoloading. The JDK 1.4 version of autoloading involves setting a system property which is a list of Drivers which should be autoloaded. The property is set when you boot the VM. It is described here: http://download.oracle.com/javase/1.4.2/docs/guide/jdbc/getstart/drivermanager.html

          Hope this helps,
          -Rick

          Show
          Rick Hillegas added a comment - - edited For two reasons: I) We simply followed the pattern described in (c) above: JDBC 4.0 did not introduce any methods into java.sql.Driver that prevented implementations from being compiled on JDK 1.4. There was therefore no reason for DriverManager.getDriver() to return 2 different classes, one for Java 5 and lower and another for Java 6 and higher. Now, of course, there is a reason to do that. II) Driver autoloading actually goes back to JDK 1.4. It's just that Java 6 introduced a new kind of driver autoloading. And Derby was out of compliance by not supporting the JDK 1.4 version of autoloading. The JDK 1.4 version of autoloading involves setting a system property which is a list of Drivers which should be autoloaded. The property is set when you boot the VM. It is described here: http://download.oracle.com/javase/1.4.2/docs/guide/jdbc/getstart/drivermanager.html Hope this helps, -Rick
          Hide
          Lily Wei added a comment -

          Thanks Rick. These are very important information. May I suggest to add them in the comments on AutoloadedDriver and AutoloadedDriver40. If the autoloading part is confusing for us, maybe we can add comment for this to ease the future confusion.

          Show
          Lily Wei added a comment - Thanks Rick. These are very important information. May I suggest to add them in the comments on AutoloadedDriver and AutoloadedDriver40. If the autoloading part is confusing for us, maybe we can add comment for this to ease the future confusion.
          Hide
          Kathey Marsden added a comment -

          Thank you Rick. That does clarify things alot. I did not know there was any autoloader capability in JDK 1.4. Do we support that property now?

          I think where things have gotten hairy with the Autoloader is that, if I am not mistaken, the AutoloadedDriver loads even with the DataSources and that is why DERBY-2905 became such an issue for Tomcat users and other app servers that don't use DriverManager at all and then since DriverManager only loads it the first time, there is some sort of odd incongruity that I can't quite put into words. I think that the behavior and the interfaces we have now are good, but hope with some thought we can find a way to make the code simpler and more intuitive per Dan's comment here: https://issues.apache.org/jira/browse/DERBY-2905?focusedCommentId=12537404&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-12537404

          I really appreciate the clarification of the current model.

          Show
          Kathey Marsden added a comment - Thank you Rick. That does clarify things alot. I did not know there was any autoloader capability in JDK 1.4. Do we support that property now? I think where things have gotten hairy with the Autoloader is that, if I am not mistaken, the AutoloadedDriver loads even with the DataSources and that is why DERBY-2905 became such an issue for Tomcat users and other app servers that don't use DriverManager at all and then since DriverManager only loads it the first time, there is some sort of odd incongruity that I can't quite put into words. I think that the behavior and the interfaces we have now are good, but hope with some thought we can find a way to make the code simpler and more intuitive per Dan's comment here: https://issues.apache.org/jira/browse/DERBY-2905?focusedCommentId=12537404&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-12537404 I really appreciate the clarification of the current model.
          Hide
          Rick Hillegas added a comment -

          I believe that we support the property-based autoloading now.

          There is a lot of trickiness in here. The JRE's autoloading contract is tricky and Derby's boot/shutdown logic is tricky.

          We are probably not out of the woods yet. I have misgivings about the original Derby design which makes ComponentUnloading an artifact of OrderlyShutdown. The reverse order makes more sense to me, particularly for smartphones. In that environment, a resource-intensive service like a database may be swapped out frequently in order to make the device responsive. I think that it is tricky to make both orders work.

          Show
          Rick Hillegas added a comment - I believe that we support the property-based autoloading now. There is a lot of trickiness in here. The JRE's autoloading contract is tricky and Derby's boot/shutdown logic is tricky. We are probably not out of the woods yet. I have misgivings about the original Derby design which makes ComponentUnloading an artifact of OrderlyShutdown. The reverse order makes more sense to me, particularly for smartphones. In that environment, a resource-intensive service like a database may be swapped out frequently in order to make the device responsive. I think that it is tricky to make both orders work.
          Hide
          Kathey Marsden added a comment -

          Thank you Rick. I will look into that property. Maybe it can help me turn off my least favorite feature, the Autoloader at user sites if it overrides the manifest.

          Perhaps the deregister attribute will be helpful in the phone environment you describe or somethng else new, but I do not think that we can forget that there are lots of production systems using derby in app servers with isolated class loaders where the unloading is critical and we can't regress the unloading again. Tracking down class loader leaks is just too painful.

          I wonder if now that DERBY-2905 is fixed if the permgen errors we have been seeing on some platforms are fixed now. I should try on my mac again and file a test issue to have formal test to make Derby is fully garbage collected after shutdown.

          Show
          Kathey Marsden added a comment - Thank you Rick. I will look into that property. Maybe it can help me turn off my least favorite feature, the Autoloader at user sites if it overrides the manifest. Perhaps the deregister attribute will be helpful in the phone environment you describe or somethng else new, but I do not think that we can forget that there are lots of production systems using derby in app servers with isolated class loaders where the unloading is critical and we can't regress the unloading again. Tracking down class loader leaks is just too painful. I wonder if now that DERBY-2905 is fixed if the permgen errors we have been seeing on some platforms are fixed now. I should try on my mac again and file a test issue to have formal test to make Derby is fully garbage collected after shutdown.
          Hide
          Lily Wei added a comment -

          While doing buddy testing, I found that there are no mention in turn of "Throw SQLFeatureNotSupportedExceptionn" for CallableStatement.getObject(String, Class) and Connection.setTypeMap(Map) on http://wiki.apache.org/db-derby/JdbcFourOne wiki page. Should we add the information to the wiki page so information is more on one place?

          I also attach derby-4869-32_add_CallableStatement_Connection_getTypeMap_test.diff patch. Please review it. My intension is only to add more test coverage. Hope this is helpful. I test against sun jvm 1.6 and sun jvm 1.7 build 1.7.0-ea-b132 both ConnectionMethodsTest and ConnectionTest tests passed.

          Show
          Lily Wei added a comment - While doing buddy testing, I found that there are no mention in turn of "Throw SQLFeatureNotSupportedExceptionn" for CallableStatement.getObject(String, Class) and Connection.setTypeMap(Map) on http://wiki.apache.org/db-derby/JdbcFourOne wiki page. Should we add the information to the wiki page so information is more on one place? I also attach derby-4869-32_add_CallableStatement_Connection_getTypeMap_test.diff patch. Please review it. My intension is only to add more test coverage. Hope this is helpful. I test against sun jvm 1.6 and sun jvm 1.7 build 1.7.0-ea-b132 both ConnectionMethodsTest and ConnectionTest tests passed.
          Hide
          Rick Hillegas added a comment -

          Thanks for supplying the extra test, Lily. I think it would be appropriate to add the following information to the wiki page (you are welcome to add it):

          1) It would be helpful if the entry for CallableStatement.getObject(String,Class) noted that that overload raises an unsupported feature exception just like the String overloads of the other getXXX() methods.

          You could also note on the Connection.setTypeMap() entry that you have added a test which was missed during the JDBC 4.0 effort. You're welcome to add yourself as the owner of that entry. Thanks.

          Show
          Rick Hillegas added a comment - Thanks for supplying the extra test, Lily. I think it would be appropriate to add the following information to the wiki page (you are welcome to add it): 1) It would be helpful if the entry for CallableStatement.getObject(String,Class) noted that that overload raises an unsupported feature exception just like the String overloads of the other getXXX() methods. You could also note on the Connection.setTypeMap() entry that you have added a test which was missed during the JDBC 4.0 effort. You're welcome to add yourself as the owner of that entry. Thanks.
          Hide
          Lily Wei added a comment -

          Thanks Rick!!! Sure, I will add the extra information and note that CallableStatement.getObject(String,Class) raises an unsupported feature exception just like the String overloads of the other getXXX() methods. And, I will also add to Connection.setTypeMap(map) for extra testing added and be the owner for that entry. If there is no objection, I will checkin the best before today. Thanks!

          Show
          Lily Wei added a comment - Thanks Rick!!! Sure, I will add the extra information and note that CallableStatement.getObject(String,Class) raises an unsupported feature exception just like the String overloads of the other getXXX() methods. And, I will also add to Connection.setTypeMap(map) for extra testing added and be the owner for that entry. If there is no objection, I will checkin the best before today. Thanks!
          Hide
          Knut Anders Hatlen added a comment -

          I didn't quite understand this part of the test:

          + Map<String, Class<?>> map = getConnection().getTypeMap();
          + try

          { + map.put("JAVA_UTIL_LIST", Class.forName("java.util.List")); + }

          catch (ClassNotFoundException se)

          { + se.printStackTrace(); + println("map.put has exception"); + }

          Is this code supposed to raise a ClassNotFoundException? If yes, I think there should be a call to fail() in the try block, and the stack trace shouldn't be printed. If no, I think the CNFE should be thrown so that the test fails.

          Show
          Knut Anders Hatlen added a comment - I didn't quite understand this part of the test: + Map<String, Class<?>> map = getConnection().getTypeMap(); + try { + map.put("JAVA_UTIL_LIST", Class.forName("java.util.List")); + } catch (ClassNotFoundException se) { + se.printStackTrace(); + println("map.put has exception"); + } Is this code supposed to raise a ClassNotFoundException? If yes, I think there should be a call to fail() in the try block, and the stack trace shouldn't be printed. If no, I think the CNFE should be thrown so that the test fails.
          Hide
          Knut Anders Hatlen added a comment -

          The new test case caused the following compiler warnings:

          [javac] /code/derby/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionTest.java:213: warning: [unchecked] unchecked call to add(E) as a member of the raw type java.util.ArrayList
          [javac] lst.add("First element");
          [javac] ^
          [javac] /code/derby/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionTest.java:214: warning: [unchecked] unchecked call to add(E) as a member of the raw type java.util.ArrayList
          [javac] lst.add("Second element");
          [javac] ^
          [javac] 2 warnings

          I'm attaching a patch that fixes the compiler warnings and also removes the unnecessary try/catch block mentioned in the previous comment.

          Show
          Knut Anders Hatlen added a comment - The new test case caused the following compiler warnings: [javac] /code/derby/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionTest.java:213: warning: [unchecked] unchecked call to add(E) as a member of the raw type java.util.ArrayList [javac] lst.add("First element"); [javac] ^ [javac] /code/derby/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ConnectionTest.java:214: warning: [unchecked] unchecked call to add(E) as a member of the raw type java.util.ArrayList [javac] lst.add("Second element"); [javac] ^ [javac] 2 warnings I'm attaching a patch that fixes the compiler warnings and also removes the unnecessary try/catch block mentioned in the previous comment.
          Hide
          Knut Anders Hatlen added a comment -

          Committed "fix-compiler-warning.diff" with revision 1083628.

          I'm wondering, though, if the call to put() on the map returned by getTypeMap() in the new test case is valid. When using the JDBC 3.0 or earlier variants of the Connection class, the returned map is immutable, and put() will throw an exception. With the JDBC 4.0 driver, the map is mutable, but I think that is just an unintended side effect of a hack used to silence a compiler warning. I've logged DERBY-5143 to track this difference between the JDBC 3.0 driver and the JDBC 4.0 driver.

          Show
          Knut Anders Hatlen added a comment - Committed "fix-compiler-warning.diff" with revision 1083628. I'm wondering, though, if the call to put() on the map returned by getTypeMap() in the new test case is valid. When using the JDBC 3.0 or earlier variants of the Connection class, the returned map is immutable, and put() will throw an exception. With the JDBC 4.0 driver, the map is mutable, but I think that is just an unintended side effect of a hack used to silence a compiler warning. I've logged DERBY-5143 to track this difference between the JDBC 3.0 driver and the JDBC 4.0 driver.
          Hide
          Lily Wei added a comment -

          Thanks Knut for removing the compiler warnings. Those lines are meant to throw CNFE so the test will fail.

          Show
          Lily Wei added a comment - Thanks Knut for removing the compiler warnings. Those lines are meant to throw CNFE so the test will fail.
          Hide
          Lily Wei added a comment -

          After apply getTypeMap-warning.diff from DERBY-5143, I now see ConnectionTest.testGetTypeMapReturnsAsExpected() has to change. My bad! I try to change with derby-4869-33_add_CallableStatement_Connection_getTypeMap_test.diff patch. However, the work is not done. First, Dave's comment for getTypeMap-warning.diff patch may cause revision of the getTypeMap-warning patch. Second, I don't understand why fail(…) call after put.map(...) failed. I shall work on this tomorrow. Feel free to change the test as fit. Thanks!

          Show
          Lily Wei added a comment - After apply getTypeMap-warning.diff from DERBY-5143 , I now see ConnectionTest.testGetTypeMapReturnsAsExpected() has to change. My bad! I try to change with derby-4869-33_add_CallableStatement_Connection_getTypeMap_test.diff patch. However, the work is not done. First, Dave's comment for getTypeMap-warning.diff patch may cause revision of the getTypeMap-warning patch. Second, I don't understand why fail(…) call after put.map(...) failed. I shall work on this tomorrow. Feel free to change the test as fit. Thanks!
          Hide
          Lily Wei added a comment -

          With PrepareStatement.execute, we throw SQLTimeoutException if the query runs beyond the limit set by setQueryTimeout(). In PSTimeout.java, I would suspect the second run(5, 0, 1, "5 time out, 0 wait") call will cause SQLTimeoutException in 5 seconds instead of 40 seconds. Since it is throwing SQLTimeoutException, this might be a minor issue. This can very well be DERBY-4863. I was just hoping the behavior will be seeing SQLTimeoutException in 5 seconds for PrepaeStatement.execute(). Is this an unreasonable expectation? .

          Show
          Lily Wei added a comment - With PrepareStatement.execute, we throw SQLTimeoutException if the query runs beyond the limit set by setQueryTimeout(). In PSTimeout.java, I would suspect the second run(5, 0, 1, "5 time out, 0 wait") call will cause SQLTimeoutException in 5 seconds instead of 40 seconds. Since it is throwing SQLTimeoutException, this might be a minor issue. This can very well be DERBY-4863 . I was just hoping the behavior will be seeing SQLTimeoutException in 5 seconds for PrepaeStatement.execute(). Is this an unreasonable expectation? .
          Hide
          Knut Anders Hatlen added a comment -

          Hi Lily, I think you're right that the problem you're seeing is the same as DERBY-4863. The PSTimeout class is more or less identical to the repro attached to that issue.

          Show
          Knut Anders Hatlen added a comment - Hi Lily, I think you're right that the problem you're seeing is the same as DERBY-4863 . The PSTimeout class is more or less identical to the repro attached to that issue.
          Hide
          Lily Wei added a comment -

          Thanks, Knut. I can agree that the PSTimeout class is just experience the same issue as DERBY-4863. How about the case for PSTimeout_execute.java? The ResultSet takes more than 5 seconds to get and the setQuerytimeout is set to 5 seconds. The SQLTimeoutException is throwing about 19 to 21 seconds later. Does Derby throw SQLTimeoutException twice long as the value set to setQueryTimeout? How does user know when SQLTimeoutException get throw depending on the value they set to setQueryTimeout?

          Show
          Lily Wei added a comment - Thanks, Knut. I can agree that the PSTimeout class is just experience the same issue as DERBY-4863 . How about the case for PSTimeout_execute.java? The ResultSet takes more than 5 seconds to get and the setQuerytimeout is set to 5 seconds. The SQLTimeoutException is throwing about 19 to 21 seconds later. Does Derby throw SQLTimeoutException twice long as the value set to setQueryTimeout? How does user know when SQLTimeoutException get throw depending on the value they set to setQueryTimeout?
          Hide
          Knut Anders Hatlen added a comment -

          Hi Lily,

          The 19 to 21 seconds include a call to execute() and many calls to ResultSet.next(). The SQLTimeoutException isn't thrown until one of these needs more than 5 seconds to complete. When I ran PSTimeout_execute in my environment, the next() call that threw the exception was stopped after 5008 ms. I think this behaviour is within specification.

          Show
          Knut Anders Hatlen added a comment - Hi Lily, The 19 to 21 seconds include a call to execute() and many calls to ResultSet.next(). The SQLTimeoutException isn't thrown until one of these needs more than 5 seconds to complete. When I ran PSTimeout_execute in my environment, the next() call that threw the exception was stopped after 5008 ms. I think this behaviour is within specification.
          Hide
          Lily Wei added a comment -

          Thank you so much for verify Derby behavior is within specification. Nothing gets away from you.

          Show
          Lily Wei added a comment - Thank you so much for verify Derby behavior is within specification. Nothing gets away from you.
          Hide
          Lily Wei added a comment -

          The derby-4869_Statement_addBatch_execute_etc.diff hopes to achieve the following agenda:
          1. Add Statement.addBatch test
          2. Add Statement.execute(String, int) and Statement.executeUpdate(String,int) tests to UnsupportedVetter. I am not totally sure this is the right way to add test coverage for these methods. Any recommendation will be appreciated.

          Show
          Lily Wei added a comment - The derby-4869_Statement_addBatch_execute_etc.diff hopes to achieve the following agenda: 1. Add Statement.addBatch test 2. Add Statement.execute(String, int) and Statement.executeUpdate(String,int) tests to UnsupportedVetter. I am not totally sure this is the right way to add test coverage for these methods. Any recommendation will be appreciated.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Lily,

          I think this is already covered by the test case testAddBatchClarification_jdbc4_1 that Rick added to StatementJdbc20Test. testaddBatchNotImplemented() in the patch seems to be a subset of that test case.

          I'm not so familiar with the UnsupportedVetter test, but it looks to me like the patch adds those two methods to the list of methods to exclude from testing, so I don't think that change will increase the test coverage.

          Show
          Knut Anders Hatlen added a comment - Hi Lily, I think this is already covered by the test case testAddBatchClarification_jdbc4_1 that Rick added to StatementJdbc20Test. testaddBatchNotImplemented() in the patch seems to be a subset of that test case. I'm not so familiar with the UnsupportedVetter test, but it looks to me like the patch adds those two methods to the list of methods to exclude from testing, so I don't think that change will increase the test coverage.
          Hide
          Lily Wei added a comment -

          Thank you so much Knut. I see testAddBatchClarification_jdbc4_1 in StatementJdbc20Test has addBatch test for Jdbc 4.1
          I also see tests for execute(String, int), execute(String, int[]), execute(String, String[]), executeUpdate(String, int), executeUpdate(String, int[]), and executeUpdate(String, String[]) are in StatementJdbc20Test. I update wiki page JdbcFourOneTesting http://wiki.apache.org/db-derby/JdbcFourOneTesting to reflect the reality.

          Show
          Lily Wei added a comment - Thank you so much Knut. I see testAddBatchClarification_jdbc4_1 in StatementJdbc20Test has addBatch test for Jdbc 4.1 I also see tests for execute(String, int), execute(String, int[]), execute(String, String[]), executeUpdate(String, int), executeUpdate(String, int[]), and executeUpdate(String, String[]) are in StatementJdbc20Test. I update wiki page JdbcFourOneTesting http://wiki.apache.org/db-derby/JdbcFourOneTesting to reflect the reality.
          Hide
          Kristian Waagan added a comment -

          What's the state of this issue?
          It has the patch available flag set, but hasn't seen any action for several months.

          Show
          Kristian Waagan added a comment - What's the state of this issue? It has the patch available flag set, but hasn't seen any action for several months.
          Hide
          Rick Hillegas added a comment -

          Thanks for noticing that the "patch available" flag was set, Kristian. I committed my last patch on February 11, 2011. I see some later patches from Lily, but I believe that she abandoned them. I have turned off the "patch available" flag. Lily may want to turn the flag back on if she wants to continue working on them.

          There is a small amount of work remaining to be done on JDBC 4.1 support now that JDK 7 has gone GA and the spec has been published. I need to describe those changes and finish that work. Thanks.

          Show
          Rick Hillegas added a comment - Thanks for noticing that the "patch available" flag was set, Kristian. I committed my last patch on February 11, 2011. I see some later patches from Lily, but I believe that she abandoned them. I have turned off the "patch available" flag. Lily may want to turn the flag back on if she wants to continue working on them. There is a small amount of work remaining to be done on JDBC 4.1 support now that JDK 7 has gone GA and the spec has been published. I need to describe those changes and finish that work. Thanks.
          Hide
          Rick Hillegas added a comment -

          Linking this issue to DERBY-5410. This may help us remember to update sysinfo the next time we implement a JDBC increment.

          Show
          Rick Hillegas added a comment - Linking this issue to DERBY-5410 . This may help us remember to update sysinfo the next time we implement a JDBC increment.
          Hide
          Myrna van Lunteren added a comment -

          Can this issue now be closed? Or is there still more work to be done? Rick, looks like you planned to describe the remaining work as indicated on 2/Sep/11 - was that DERBY-5488 or something else?

          Show
          Myrna van Lunteren added a comment - Can this issue now be closed? Or is there still more work to be done? Rick, looks like you planned to describe the remaining work as indicated on 2/Sep/11 - was that DERBY-5488 or something else?
          Hide
          Rick Hillegas added a comment -

          Thanks, Myrna. I believe we've wrapped up the work for this issue. Resolving it.

          Show
          Rick Hillegas added a comment - Thanks, Myrna. I believe we've wrapped up the work for this issue. Resolving it.

            People

            • Assignee:
              Rick Hillegas
              Reporter:
              Rick Hillegas
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development