Derby
  1. Derby
  2. DERBY-4122

ClassCastException in SQLClob when running in soft upgrade mode (10.4.2.0 -> 10.5.1.0)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.5.1.1, 10.6.1.0
    • Component/s: Test
    • Labels:
      None
    • Environment:
      Windows Vista 64, Sun JDK 1.6.0_10, Junit 3.8.2
    • Urgency:
      Blocker
    • Bug behavior facts:
      Regression, Regression Test Failure

      Description

      This bug was found when doing soft upgrade testing from Derby version 10.4.2.0 to 10.5.1.0 (RC1)

      Steps followed are as follows.

      1. Run setEmbeddedCP.bat from version 10.4.2.0's bin folder
      2. In a test folder run ij
      3. create system/wombat database.
      ij> connect 'jdbc:derby:system/wombat;create=true';
      4. exit ij
      5. Copy the 10.5.1.0 derby jars (from lib folder) and the derbyTesting.jar from 10.4.2.0 to the test folder and set classpath with them (including junit and ORO)
      6. Run suites.All
      java -Xmx512M -Xms512M -Dderby.tests.trace=true junit.textui.TestRunner org.apache.derbyTesting.functionTests.suites.All

      Result:
      Tests run: 10479, Failures: 56, Errors: 34

      The exception stack trace from a failed test follows.

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

      3) testClobInTriggerTable(org.apache.derbyTesting.functionTests.tests.lang.TriggerTest)java.sql.SQLException: Java exception: 'org.apache.derby.iapi.types.ReaderToUTF8Stream cannot be cast to org.apache.derby.iapi.types.Resetable: java.lang.ClassCastException'.
      at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(Unknown Source)
      at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Unknown Source)
      at org.apache.derby.impl.jdbc.Util.javaException(Unknown Source)
      at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(Unknown Source)
      at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(Unknown Source)
      at org.apache.derby.impl.jdbc.EmbedConnection.handleException(Unknown Source)
      at org.apache.derby.impl.jdbc.ConnectionChild.handleException(Unknown Source)
      at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(Unknown Source)
      at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeStatement(Unknown Source)
      at org.apache.derby.impl.jdbc.EmbedPreparedStatement.execute(Unknown Source)
      at org.apache.derbyTesting.functionTests.tests.lang.TriggerTest.testClobInTriggerTable(TriggerTest.java:529)
      at org.apache.derbyTesting.functionTests.tests.lang.TriggerTest.testClobInTriggerTable(TriggerTest.java:451)
      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:102)
      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)
      Caused by: java.sql.SQLException: Java exception: 'org.apache.derby.iapi.types.ReaderToUTF8Stream cannot be cast to org.apache.derby.iapi.types.Resetable: java.lang.ClassCastException'.
      at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(Unknown Source)
      at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(Unknown Source)
      ... 39 more
      Caused by: java.lang.ClassCastException: org.apache.derby.iapi.types.ReaderToUTF8Stream cannot be cast to org.apache.derby.iapi.types.Resetable
      at org.apache.derby.iapi.types.SQLClob.rewindStream(Unknown Source)
      at org.apache.derby.iapi.types.SQLClob.readExternal(Unknown Source)
      at org.apache.derby.iapi.types.SQLChar.getString(Unknown Source)
      at org.apache.derby.iapi.types.SQLChar.loadStream(Unknown Source)
      at org.apache.derby.impl.sql.execute.UpdateResultSet.objectifyStream(Unknown Source)
      at org.apache.derby.impl.sql.execute.UpdateResultSet.collectAffectedRows(Unknown Source)
      at org.apache.derby.impl.sql.execute.UpdateResultSet.open(Unknown Source)
      at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(Unknown Source)
      at org.apache.derby.impl.sql.GenericPreparedStatement.execute(Unknown Source)
      ... 32 more
      ------------------------------------------------------------------------------------------------------------------

      When looking at the SVN revisions for SQLClob with Kathey Marsden, we found the following statement in revision # 738408, related to DERBY-3907, which might be related to this issue.

      "NOTE: Databases created with this revision (or later) containing Clobs, cannot be accessed by earlier trunk revisions."
      Patch file: derby-3907-7a3-use_new_header_format.diff

      1. derby-4122-1a-incorrect_stream_positioning.diff
        0.8 kB
        Kristian Waagan
      2. derby-4122-2a-bc4btest.diff
        0.8 kB
        Kristian Waagan
      3. derby-4122-3a-classcast_fix.diff
        9 kB
        Kristian Waagan
      4. derby-4122-3b-classcast_fix.diff
        10 kB
        Kristian Waagan
      5. derby-4122-4a-classcast_fix_mark_reset.diff
        17 kB
        Kristian Waagan
      6. derby-4122-4b-classcast_fix_mark_reset.diff
        39 kB
        Kristian Waagan
      7. derby-4122-4c-classcast_fix_mark_reset.diff
        40 kB
        Kristian Waagan

        Issue Links

          Activity

          Suran Jayathilaka created issue -
          Hide
          Suran Jayathilaka added a comment -

          This is the stack trace from another failed test in the same test run, which may be related to the same issue as above. Should be investigated and filed separately if this is caused by a different problem.

          13) testTriggersWithClobColumn(org.apache.derbyTesting.functionTests.tests.jdbcapi.BlobClob4BlobTest)java.sql.SQLException: An IOException was thrown when reading a 'java.sql.String' from an InputStream.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(Unknown Source)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Unknown Source)
          at org.apache.derby.impl.jdbc.Util.seeNextException(Unknown Source)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(Unknown Source)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(Unknown Source)
          at org.apache.derby.impl.jdbc.EmbedConnection.handleException(Unknown Source)
          at org.apache.derby.impl.jdbc.ConnectionChild.handleException(Unknown Source)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(Unknown Source)
          at org.apache.derby.impl.jdbc.EmbedStatement.execute(Unknown Source)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeUpdate(Unknown Source)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.BlobClob4BlobTest.testTriggersWithClobColumn(BlobClob4BlobTest.java:397)
          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:102)
          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 org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          Caused by: java.sql.SQLException: An IOException was thrown when reading a 'java.sql.String' from an InputStream.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(Unknown Source)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(Unknown Source)
          ... 45 more
          Caused by: java.sql.SQLException: Java exception: ': java.io.EOFException'.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(Unknown Source)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(Unknown Source)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(Unknown Source)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Unknown Source)
          at org.apache.derby.impl.jdbc.Util.javaException(Unknown Source)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(Unknown Source)
          ... 42 more
          Caused by: java.io.EOFException
          at org.apache.derby.iapi.types.SQLChar.readExternal(Unknown Source)
          at org.apache.derby.iapi.types.SQLClob.readExternal(Unknown Source)
          at org.apache.derby.iapi.types.SQLChar.getString(Unknown Source)
          at org.apache.derby.iapi.types.SQLChar.loadStream(Unknown Source)
          at org.apache.derby.impl.sql.execute.DMLWriteResultSet.objectifyStreams(Unknown Source)
          at org.apache.derby.impl.sql.execute.DMLWriteResultSet.getNextRowCore(Unknown Source)
          at org.apache.derby.impl.sql.execute.UpdateResultSet.collectAffectedRows(Unknown Source)
          at org.apache.derby.impl.sql.execute.UpdateResultSet.open(Unknown Source)
          at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(Unknown Source)
          at org.apache.derby.impl.sql.GenericPreparedStatement.execute(Unknown Source)
          ... 38 more

          Show
          Suran Jayathilaka added a comment - This is the stack trace from another failed test in the same test run, which may be related to the same issue as above. Should be investigated and filed separately if this is caused by a different problem. 13) testTriggersWithClobColumn(org.apache.derbyTesting.functionTests.tests.jdbcapi.BlobClob4BlobTest)java.sql.SQLException: An IOException was thrown when reading a 'java.sql.String' from an InputStream. at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(Unknown Source) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Unknown Source) at org.apache.derby.impl.jdbc.Util.seeNextException(Unknown Source) at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(Unknown Source) at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(Unknown Source) at org.apache.derby.impl.jdbc.EmbedConnection.handleException(Unknown Source) at org.apache.derby.impl.jdbc.ConnectionChild.handleException(Unknown Source) at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(Unknown Source) at org.apache.derby.impl.jdbc.EmbedStatement.execute(Unknown Source) at org.apache.derby.impl.jdbc.EmbedStatement.executeUpdate(Unknown Source) at org.apache.derbyTesting.functionTests.tests.jdbcapi.BlobClob4BlobTest.testTriggersWithClobColumn(BlobClob4BlobTest.java:397) 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:102) 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 org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) Caused by: java.sql.SQLException: An IOException was thrown when reading a 'java.sql.String' from an InputStream. at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(Unknown Source) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(Unknown Source) ... 45 more Caused by: java.sql.SQLException: Java exception: ': java.io.EOFException'. at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(Unknown Source) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(Unknown Source) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(Unknown Source) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Unknown Source) at org.apache.derby.impl.jdbc.Util.javaException(Unknown Source) at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(Unknown Source) ... 42 more Caused by: java.io.EOFException at org.apache.derby.iapi.types.SQLChar.readExternal(Unknown Source) at org.apache.derby.iapi.types.SQLClob.readExternal(Unknown Source) at org.apache.derby.iapi.types.SQLChar.getString(Unknown Source) at org.apache.derby.iapi.types.SQLChar.loadStream(Unknown Source) at org.apache.derby.impl.sql.execute.DMLWriteResultSet.objectifyStreams(Unknown Source) at org.apache.derby.impl.sql.execute.DMLWriteResultSet.getNextRowCore(Unknown Source) at org.apache.derby.impl.sql.execute.UpdateResultSet.collectAffectedRows(Unknown Source) at org.apache.derby.impl.sql.execute.UpdateResultSet.open(Unknown Source) at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(Unknown Source) at org.apache.derby.impl.sql.GenericPreparedStatement.execute(Unknown Source) ... 38 more
          Hide
          Kristian Waagan added a comment -

          I'm pretty sure DERBY-3907 introduced this bug. I'll have a look tomorrow.

          Show
          Kristian Waagan added a comment - I'm pretty sure DERBY-3907 introduced this bug. I'll have a look tomorrow.
          Hide
          Kristian Waagan added a comment -

          Posting two small patches;
          1a: makes a stream with a pre-10.5 header correctly positioned after the header has been investigated.
          2a: makes BlobClob4BlobTest.testTriggersWithClobColumn() select from the correct tables (probably a typo in the original test).

          Patch 1a should fix some of the EOFExceptions reported during soft upgrade testing (reported by Suran), but it doesn't fix the class cast errors.

          Show
          Kristian Waagan added a comment - Posting two small patches; 1a: makes a stream with a pre-10.5 header correctly positioned after the header has been investigated. 2a: makes BlobClob4BlobTest.testTriggersWithClobColumn() select from the correct tables (probably a typo in the original test). Patch 1a should fix some of the EOFExceptions reported during soft upgrade testing (reported by Suran), but it doesn't fix the class cast errors.
          Kristian Waagan made changes -
          Field Original Value New Value
          Attachment derby-4122-1a-incorrect_stream_positioning.diff [ 12403803 ]
          Attachment derby-4122-2a-bc4btest.diff [ 12403804 ]
          Kristian Waagan made changes -
          Assignee Kristian Waagan [ kristwaa ]
          Kristian Waagan made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Hide
          Kathey Marsden added a comment -

          Regarsding patch 1a, It would be good to have a new upgrade test to test the fix and also to have a brief code comment to explain the fix and mention the JIRA number for reference. Perhaps, since it is a different problem than the ClassCastException (or at least has a different fix), another Jira might be appropriate.

          Since the test patch is not really related. It would be good to have it as a separate commit.

          Show
          Kathey Marsden added a comment - Regarsding patch 1a, It would be good to have a new upgrade test to test the fix and also to have a brief code comment to explain the fix and mention the JIRA number for reference. Perhaps, since it is a different problem than the ClassCastException (or at least has a different fix), another Jira might be appropriate. Since the test patch is not really related. It would be good to have it as a separate commit.
          Hide
          Kristian Waagan added a comment -

          Sorry Kathey, I just committed the patches before I saw your comment.
          Committed patch 1a to trunk with revision 759153, and patch 2a to trunk with revision 759155.

          Regarding tests, I believe our existing tests cover the issue if they are run against a 10.4 database. I must admit, it is not clear to me what will be run and tested as part of our default upgrade tests. I need to look into this.

          Show
          Kristian Waagan added a comment - Sorry Kathey, I just committed the patches before I saw your comment. Committed patch 1a to trunk with revision 759153, and patch 2a to trunk with revision 759155. Regarding tests, I believe our existing tests cover the issue if they are run against a 10.4 database. I must admit, it is not clear to me what will be run and tested as part of our default upgrade tests. I need to look into this.
          Hide
          Kristian Waagan added a comment -

          BTW, derbyall and suites.All passed.

          Show
          Kristian Waagan added a comment - BTW, derbyall and suites.All passed.
          Hide
          Kristian Waagan added a comment -

          Patch 3a is the first revision for a fix. I haven't had time to run extensive tests yet.
          I found that the problem is that Derby tries to reset / rewind a stream that can't be reset.
          These streams are coming directly from the user, wrapped in a ReaderToUTF8Stream. It would be best to avoid having to move backwards in the stream, but then we have to know we're dealing with a database in soft upgrade mode. This information hasn't been kept in SQLClob earlier.
          DERBY-4102 is a related issue that has been logged.

          There is also a bug in the stream code to deal with the empty string ("").
          In the patch I have chosen to materialize the value if it is very small instead of keeping the stream (see SQLClob.setObject()). The Clob is also materialized when a clone is created in the existing code.
          This problem should be investigated further to understand the root cause.

          Note that the patch doesn't solve all the issues. When running BlobClob4BlobTest in soft upgrade mode, you still get two failures:
          junit.framework.ComparisonFailure: Unexpected SQL state. expected:<[XJ073]> but was:<[40XD0]>
          My theory here is that the exception is being thrown in a different place in 10.5 than in 10.4, and in 10.5 there is no code wrapping the inner-most exception.

          I'm starting derbyall and suites.All now.
          Patch ready for review and comments.

          Show
          Kristian Waagan added a comment - Patch 3a is the first revision for a fix. I haven't had time to run extensive tests yet. I found that the problem is that Derby tries to reset / rewind a stream that can't be reset. These streams are coming directly from the user, wrapped in a ReaderToUTF8Stream. It would be best to avoid having to move backwards in the stream, but then we have to know we're dealing with a database in soft upgrade mode. This information hasn't been kept in SQLClob earlier. DERBY-4102 is a related issue that has been logged. There is also a bug in the stream code to deal with the empty string (""). In the patch I have chosen to materialize the value if it is very small instead of keeping the stream (see SQLClob.setObject()). The Clob is also materialized when a clone is created in the existing code. This problem should be investigated further to understand the root cause. Note that the patch doesn't solve all the issues. When running BlobClob4BlobTest in soft upgrade mode, you still get two failures: junit.framework.ComparisonFailure: Unexpected SQL state. expected:< [XJ073] > but was:< [40XD0] > My theory here is that the exception is being thrown in a different place in 10.5 than in 10.4, and in 10.5 there is no code wrapping the inner-most exception. I'm starting derbyall and suites.All now. Patch ready for review and comments.
          Kristian Waagan made changes -
          Attachment derby-4122-3a-classcast_fix.diff [ 12403823 ]
          Hide
          Kristian Waagan added a comment -

          derbyall and suites.All ran without failures (with freshly created test databases, not in soft upgrade mode).

          Show
          Kristian Waagan added a comment - derbyall and suites.All ran without failures (with freshly created test databases, not in soft upgrade mode).
          Hide
          Kristian Waagan added a comment -

          Patch 3b also takes care of the incorrect SQLState errors seen during soft upgrade testing.

          Show
          Kristian Waagan added a comment - Patch 3b also takes care of the incorrect SQLState errors seen during soft upgrade testing.
          Kristian Waagan made changes -
          Attachment derby-4122-3b-classcast_fix.diff [ 12404147 ]
          Hide
          Kristian Waagan added a comment -

          In patch 3b, there are some issues that need to be resolved:
          1) How to best determine if a stream is resetable or not.
          2) Is it okay to materialize small Clobs in SQLClob.setObject?
          And what should the threshold for a small Clob be?

          The problem in 1 is that no matter if the source stream is resetable or not, we wrap it in a FormatIdInputStream. This class implements the Resetable interface, and a test for instanceof Resetable will fail with the reported ClassCastException.
          Some options from the top of my head (I'll spend more time investigating, but would appreciate comments);
          a) Introduce yet another class (a "non-resetable-FormatIdInputStream")
          b) Add the method isResetable to the Resetable interface (kind of contradicting...)
          c) Try to reset / rewind, catch ClassCastException and fallback to passing along the "bytes peeked at".
          d) Add instance variable in SQLChar, which allows us to do the instanceof check before we wrap the stream in a FormatIdInputStream.
          e) Your suggestion?

          Note that I haven't had time to throughly investigate this yet, so I may have missed something that's simpler and easier to implement.

          A few comments on some of the options I mentioned so far:
          a) This adds to the overhead, but the fact that this problem arises suggests there is a design problem in the current implementation. It is not clear to me what kind of changes this approach requires.
          c) Probably doable, but this makes exception catching part of the "flow control" for Clobs written with the old header format.
          d) Adds more state to a central data type, which has to be maintained. I think the maintenance costs should be modest, but I haven't investigated that. It may be possible to add this information to the CharacterStreamDescriptor-object, but again I don't know how this will play out.

          That said, the current patch already works...
          Feedback appreciated!

          Show
          Kristian Waagan added a comment - In patch 3b, there are some issues that need to be resolved: 1) How to best determine if a stream is resetable or not. 2) Is it okay to materialize small Clobs in SQLClob.setObject? And what should the threshold for a small Clob be? The problem in 1 is that no matter if the source stream is resetable or not, we wrap it in a FormatIdInputStream. This class implements the Resetable interface, and a test for instanceof Resetable will fail with the reported ClassCastException. Some options from the top of my head (I'll spend more time investigating, but would appreciate comments); a) Introduce yet another class (a "non-resetable-FormatIdInputStream") b) Add the method isResetable to the Resetable interface (kind of contradicting...) c) Try to reset / rewind, catch ClassCastException and fallback to passing along the "bytes peeked at". d) Add instance variable in SQLChar, which allows us to do the instanceof check before we wrap the stream in a FormatIdInputStream. e) Your suggestion? Note that I haven't had time to throughly investigate this yet, so I may have missed something that's simpler and easier to implement. A few comments on some of the options I mentioned so far: a) This adds to the overhead, but the fact that this problem arises suggests there is a design problem in the current implementation. It is not clear to me what kind of changes this approach requires. c) Probably doable, but this makes exception catching part of the "flow control" for Clobs written with the old header format. d) Adds more state to a central data type, which has to be maintained. I think the maintenance costs should be modest, but I haven't investigated that. It may be possible to add this information to the CharacterStreamDescriptor-object, but again I don't know how this will play out. That said, the current patch already works... Feedback appreciated!
          Hide
          Kristian Waagan added a comment -

          Backported patches 1a and 2a to 10.5 with revision 760820.

          Show
          Kristian Waagan added a comment - Backported patches 1a and 2a to 10.5 with revision 760820.
          Kristian Waagan made changes -
          Link This issue incorporates DERBY-4135 [ DERBY-4135 ]
          Hide
          Knut Anders Hatlen added a comment -

          > 1) How to best determine if a stream is resetable or not.
          >
          > The problem in 1 is that no matter if the source stream is resetable
          > or not, we wrap it in a FormatIdInputStream. This class implements
          > the Resetable interface, and a test for instanceof Resetable will
          > fail with the reported ClassCastException.

          I'm not sure I understand. The patch solved the ClassCastException by
          checking stream instanceof ReaderToUTF8Stream. Where does the
          FormatIdInputStream come into the picture?

          > 2) Is it okay to materialize small Clobs in SQLClob.setObject?
          > And what should the threshold for a small Clob be?

          That sounds OK to me. When we get the Clobs from store, isn't the
          limit about 32K? Perhaps that could be used as a starting point.

          Nit: SQLChar.EMPTY could be replaced by ReuseFactory.getZeroLenByteArray().

          Show
          Knut Anders Hatlen added a comment - > 1) How to best determine if a stream is resetable or not. > > The problem in 1 is that no matter if the source stream is resetable > or not, we wrap it in a FormatIdInputStream. This class implements > the Resetable interface, and a test for instanceof Resetable will > fail with the reported ClassCastException. I'm not sure I understand. The patch solved the ClassCastException by checking stream instanceof ReaderToUTF8Stream. Where does the FormatIdInputStream come into the picture? > 2) Is it okay to materialize small Clobs in SQLClob.setObject? > And what should the threshold for a small Clob be? That sounds OK to me. When we get the Clobs from store, isn't the limit about 32K? Perhaps that could be used as a starting point. Nit: SQLChar.EMPTY could be replaced by ReuseFactory.getZeroLenByteArray().
          Hide
          Kristian Waagan added a comment -

          > I'm not sure I understand. The patch solved the ClassCastException by
          > checking stream instanceof ReaderToUTF8Stream. Where does the
          > FormatIdInputStream come into the picture?

          The instanceof ReaderToUTF8Stream happens in SQLClob.readExternal.
          From SQLChar.getString():
          """
          } else if (stream != null) {

          // data stored as a stream
          try {

          if (stream instanceof FormatIdInputStream)

          { readExternal((FormatIdInputStream) stream); }

          else

          { readExternal(new FormatIdInputStream(stream)); }

          stream = null;

          // at this point the value is only in the char[]
          // so call again to convert to a String
          return getString();

          } catch (IOException ioe) {
          """

          In the code above, the variable 'stream' will be a FormatIdInputStream if it
          comes from store, and ReaderToUTF8Stream if the value hasn't been
          inserted into the store (i.e. we're working on this value from within a trigger,
          or a VALUES clause).
          At this point, doing the more general check 'stream instanceof Resetable' is valid.

          And again, the reason why this was discovered during (soft) upgrade testing, is that
          in a fresh 10.5 database you won't have to reset / rewind the stream.

          Show
          Kristian Waagan added a comment - > I'm not sure I understand. The patch solved the ClassCastException by > checking stream instanceof ReaderToUTF8Stream. Where does the > FormatIdInputStream come into the picture? The instanceof ReaderToUTF8Stream happens in SQLClob.readExternal. From SQLChar.getString(): """ } else if (stream != null) { // data stored as a stream try { if (stream instanceof FormatIdInputStream) { readExternal((FormatIdInputStream) stream); } else { readExternal(new FormatIdInputStream(stream)); } stream = null; // at this point the value is only in the char[] // so call again to convert to a String return getString(); } catch (IOException ioe) { """ In the code above, the variable 'stream' will be a FormatIdInputStream if it comes from store, and ReaderToUTF8Stream if the value hasn't been inserted into the store (i.e. we're working on this value from within a trigger, or a VALUES clause). At this point, doing the more general check 'stream instanceof Resetable' is valid. And again, the reason why this was discovered during (soft) upgrade testing, is that in a fresh 10.5 database you won't have to reset / rewind the stream.
          Hide
          Knut Anders Hatlen added a comment -

          Is it possible to implement mark()/reset()/markSupported() in ReaderToUTF8Stream? Then we could use stream.mark(MAX_STREAM_HEADER_LENGTH) before the read operation and stream.reset() + InputStreamUtil.skipFully(stream, hdrInfo.headerLenght()) after on. We would still need separate code for the Resetable case, though, but we wouldn't need to check for exact class match, checks for InputStream.markSupported() and instanceof Resetable would do.

          Show
          Knut Anders Hatlen added a comment - Is it possible to implement mark()/reset()/markSupported() in ReaderToUTF8Stream? Then we could use stream.mark(MAX_STREAM_HEADER_LENGTH) before the read operation and stream.reset() + InputStreamUtil.skipFully(stream, hdrInfo.headerLenght()) after on. We would still need separate code for the Resetable case, though, but we wouldn't need to check for exact class match, checks for InputStream.markSupported() and instanceof Resetable would do.
          Hide
          Kristian Waagan added a comment -

          Thanks for the comments, Knut Anders!

          I have implemented a first draft of the mark/reset approach. Please have a look.
          I'm also wondering what to do if we are still unable to reset the stream (i.e., that it doesn't support mark/reset nor Resetable).
          Keep the "bytesPeekedAt"-mechanism or just fail with an IOException?

          I think the mark/reset approach you suggested looks better than the bytesPeekedAt-approach, and it may also be useful in other settings. Keeping the latter clutters the code to some degree, and I don't think it's possible for a user to inject a stream that doesn't support mark/reset nor Resetable.

          Show
          Kristian Waagan added a comment - Thanks for the comments, Knut Anders! I have implemented a first draft of the mark/reset approach. Please have a look. I'm also wondering what to do if we are still unable to reset the stream (i.e., that it doesn't support mark/reset nor Resetable). Keep the "bytesPeekedAt"-mechanism or just fail with an IOException? I think the mark/reset approach you suggested looks better than the bytesPeekedAt-approach, and it may also be useful in other settings. Keeping the latter clutters the code to some degree, and I don't think it's possible for a user to inject a stream that doesn't support mark/reset nor Resetable.
          Kristian Waagan made changes -
          Kristian Waagan made changes -
          Derby Info [Regression] [Patch Available, Regression]
          Hide
          Knut Anders Hatlen added a comment -

          I agree that it sounds better to remove the bytesPeekedAt code since we are able to control which streams are passed in and can make sure that they implement one of the reset interfaces.

          I haven't reviewed the full patch yet, only the ReaderToUTF8Stream part. Here are my comments:

          • Since the buffer is not of constant size, I think we should remove the BUFSIZE constant so that no one incorrectly uses it instead of buffer.length. In fact, I think the available() method will return the wrong result now because it uses BUFSIZE.
          • Do we need to separate between MARK_UNSET and EXCEEDED_MARK_LIMIT? Seems like we can get away with using MARK_UNSET in both cases and get slightly simpler code, and one less error message to internationalize (the message for the new IOException should be internationalized, shouldn't it?)
          • I think I would have removed the shrink logic in fillBuffer(). It's probably going to end up as code that's never called, so I don't think the benefit (potentially release memory earlier if mark() is called with a really large argument) justifies the extra code.
          • Should there be a comment and/or constant for the magic number 6 in fillBuffer()? I see that there is a comment in the existing code, but it is further down, so I scratched my head for a while trying to understand it before I got to that comment.
          • One potential optimization (not needed in the first increment, but it's so simple that it's probably worth adding it later): When we allocate a new buffer, we could check if buffer.length <= (readAheadLimit + 6), and if it is, we can just reuse the old buffer. If oldBuf and buffer point to the same array, the call to arraycopy() will just shift the bytes to the left and free space at the right side of the array.
          Show
          Knut Anders Hatlen added a comment - I agree that it sounds better to remove the bytesPeekedAt code since we are able to control which streams are passed in and can make sure that they implement one of the reset interfaces. I haven't reviewed the full patch yet, only the ReaderToUTF8Stream part. Here are my comments: Since the buffer is not of constant size, I think we should remove the BUFSIZE constant so that no one incorrectly uses it instead of buffer.length. In fact, I think the available() method will return the wrong result now because it uses BUFSIZE. Do we need to separate between MARK_UNSET and EXCEEDED_MARK_LIMIT? Seems like we can get away with using MARK_UNSET in both cases and get slightly simpler code, and one less error message to internationalize (the message for the new IOException should be internationalized, shouldn't it?) I think I would have removed the shrink logic in fillBuffer(). It's probably going to end up as code that's never called, so I don't think the benefit (potentially release memory earlier if mark() is called with a really large argument) justifies the extra code. Should there be a comment and/or constant for the magic number 6 in fillBuffer()? I see that there is a comment in the existing code, but it is further down, so I scratched my head for a while trying to understand it before I got to that comment. One potential optimization (not needed in the first increment, but it's so simple that it's probably worth adding it later): When we allocate a new buffer, we could check if buffer.length <= (readAheadLimit + 6), and if it is, we can just reuse the old buffer. If oldBuf and buffer point to the same array, the call to arraycopy() will just shift the bytes to the left and free space at the right side of the array.
          Hide
          Knut Anders Hatlen added a comment -

          SQLClob.java:

          The approach seems fine to me. Some more detailed comments:

          • Won't the code below throw NPE if ioe.getCause() returns null?
            Initializing rootCause to ioe would be safer, I think, and still do the right thing.

          + Throwable rootCause = ioe.getCause();
          + while (rootCause.getCause() != null)

          { + rootCause = rootCause.getCause(); + }
          • Perhaps we should remove the instanceof check in the code below and remove the else branch? I think the ClassCastException we would get then is more informative than "Unable to reset stream" (and it saves us one translation

          + } else if (stream instanceof Resetable)

          { + // We have a store stream. + rewindStream(hdrInfo.headerLength()); + }

          else

          { + throw new IOException("Unable to reset stream"); + }
          • In readExternal() we now subtract the header length from the byte length before we pass it to super.readExternal(). As far as I can see, this is a code path taken in normal operation too, not only in soft upgrade. Was this a bug that was possible to see when not running with a soft upgraded db?
          • Both SQLClob and ReaderToUTF8Stream have code where they use Math.max(0, x-y) to prevent that an argument goes negative. I'm not sure I see why the arguments will go negative, and isn't there a chance that we're hiding real bugs this way? It would be good if there at least was a comment stating which conditions that may make the result negative, so that it is easier to see that it is OK to use 0 in those cases.
          Show
          Knut Anders Hatlen added a comment - SQLClob.java: The approach seems fine to me. Some more detailed comments: Won't the code below throw NPE if ioe.getCause() returns null? Initializing rootCause to ioe would be safer, I think, and still do the right thing. + Throwable rootCause = ioe.getCause(); + while (rootCause.getCause() != null) { + rootCause = rootCause.getCause(); + } Perhaps we should remove the instanceof check in the code below and remove the else branch? I think the ClassCastException we would get then is more informative than "Unable to reset stream" (and it saves us one translation + } else if (stream instanceof Resetable) { + // We have a store stream. + rewindStream(hdrInfo.headerLength()); + } else { + throw new IOException("Unable to reset stream"); + } In readExternal() we now subtract the header length from the byte length before we pass it to super.readExternal(). As far as I can see, this is a code path taken in normal operation too, not only in soft upgrade. Was this a bug that was possible to see when not running with a soft upgraded db? Both SQLClob and ReaderToUTF8Stream have code where they use Math.max(0, x-y) to prevent that an argument goes negative. I'm not sure I see why the arguments will go negative, and isn't there a chance that we're hiding real bugs this way? It would be good if there at least was a comment stating which conditions that may make the result negative, so that it is easier to see that it is OK to use 0 in those cases.
          Hide
          Kristian Waagan added a comment -

          Knut, I'm working on a new patch. The following items (taken from your comments) will be addressed:
          o remove BUFSIZE (and adjust code where required)
          o remove shrink logic
          o move and/or add more comments about the magic number 6
          o copy optimization
          o NPE possibility in root cause determination code
          o rewrite if in SQLClob

          Then some comments on some of the other items:
          > Do we need to separate between MARK_UNSET and EXCEEDED_MARK_LIMIT?
          No, only for more specific error reporting, I think.

          > In readExternal() we now subtract the header length from the byte length before we pass it to super.readExternal(). As far as I can see, this is a code path taken in normal operation too, not only in soft upgrade. Was this a bug that was possible to see when not running with a soft upgraded db?
          No, it should not happen in a fresh 10.5 database. It can happen in soft and hard upgrade.
          The reason why it doesn't happen in a fresh database, is that all lengths are stored as character counts.

          > Both SQLClob and ReaderToUTF8Stream have code where they use Math.max(0, x-y) to prevent that an argument goes negative. I'm not sure I see why the arguments will go negative, and isn't there a chance that we're hiding real bugs this way?
          The argument in SQLClob will go negative when the stored length in the byte header is 0. Since the header length is still two in this case, we get 0 - 2 = -2. There is a slight chance we may hide a bug with this, but the bug will be revealed when we actually try to read the value. In principle, what we're saying here is that if the length we obtained from the header is negative, assume it is unknown.
          I can try to rewrite the check so that it is easier to understand, and add some comments.

          In ReaderToUTF8Stream, the variable blen will be -1 the first time we fill the buffer. Again, I'll rewrite or/and add a comment to make it easier to read the code.

          Show
          Kristian Waagan added a comment - Knut, I'm working on a new patch. The following items (taken from your comments) will be addressed: o remove BUFSIZE (and adjust code where required) o remove shrink logic o move and/or add more comments about the magic number 6 o copy optimization o NPE possibility in root cause determination code o rewrite if in SQLClob Then some comments on some of the other items: > Do we need to separate between MARK_UNSET and EXCEEDED_MARK_LIMIT? No, only for more specific error reporting, I think. > In readExternal() we now subtract the header length from the byte length before we pass it to super.readExternal(). As far as I can see, this is a code path taken in normal operation too, not only in soft upgrade. Was this a bug that was possible to see when not running with a soft upgraded db? No, it should not happen in a fresh 10.5 database. It can happen in soft and hard upgrade. The reason why it doesn't happen in a fresh database, is that all lengths are stored as character counts. > Both SQLClob and ReaderToUTF8Stream have code where they use Math.max(0, x-y) to prevent that an argument goes negative. I'm not sure I see why the arguments will go negative, and isn't there a chance that we're hiding real bugs this way? The argument in SQLClob will go negative when the stored length in the byte header is 0. Since the header length is still two in this case, we get 0 - 2 = -2. There is a slight chance we may hide a bug with this, but the bug will be revealed when we actually try to read the value. In principle, what we're saying here is that if the length we obtained from the header is negative, assume it is unknown. I can try to rewrite the check so that it is easier to understand, and add some comments. In ReaderToUTF8Stream, the variable blen will be -1 the first time we fill the buffer. Again, I'll rewrite or/and add a comment to make it easier to read the code.
          Hide
          Kristian Waagan added a comment -

          New revision (4b) of the mark/reset fix.

          I'm rerunning tests, plan to commit tomorrow if they pass.

          Show
          Kristian Waagan added a comment - New revision (4b) of the mark/reset fix. I'm rerunning tests, plan to commit tomorrow if they pass.
          Kristian Waagan made changes -
          Hide
          Knut Anders Hatlen added a comment -

          4b looks good to me. Two minor comments:

          • SQLState.LANG_STREAM_MARK_UNSET_OR_EXCEEDED should be moved to MessageId, since it's not used in an SQLException
          • I don't think EOFException should be thrown by reset() when the stream is closed, as stream closed does not mean that EOF has been reached. What about a plain IOException with the message from SQLState.LANG_STREAM_CLOSED?
          Show
          Knut Anders Hatlen added a comment - 4b looks good to me. Two minor comments: SQLState.LANG_STREAM_MARK_UNSET_OR_EXCEEDED should be moved to MessageId, since it's not used in an SQLException I don't think EOFException should be thrown by reset() when the stream is closed, as stream closed does not mean that EOF has been reached. What about a plain IOException with the message from SQLState.LANG_STREAM_CLOSED?
          Hide
          Kristian Waagan added a comment -

          Thanks for looking at the patch again, Knut Anders.

          I have moved the new message to MessageId. It wasn't immediately clear to me what to name the message and into which category it should be placed. We can move/rename it later if required.

          Regarding the EOFException, I have chosen to keep it as it is for now because other methods in the class use it too. It may be that it should be changed, but it must be tested. Since ReaderToUTF8Stream has been considered an "internal stream class" it is not clear to me whether Derby code expects the EOFException or not.

          I also want to improve the test a bit, by making the randomized test verify the content returned by the stream. Depending on when I get around to do it, I may do this under this issue or create a sub-issue.

          Show
          Kristian Waagan added a comment - Thanks for looking at the patch again, Knut Anders. I have moved the new message to MessageId. It wasn't immediately clear to me what to name the message and into which category it should be placed. We can move/rename it later if required. Regarding the EOFException, I have chosen to keep it as it is for now because other methods in the class use it too. It may be that it should be changed, but it must be tested. Since ReaderToUTF8Stream has been considered an "internal stream class" it is not clear to me whether Derby code expects the EOFException or not. I also want to improve the test a bit, by making the randomized test verify the content returned by the stream. Depending on when I get around to do it, I may do this under this issue or create a sub-issue.
          Kristian Waagan made changes -
          Attachment derby-4122-4b-classcast_fix_mark_reset.diff [ 12404736 ]
          Kristian Waagan made changes -
          Attachment derby-4122-4b-classcast_fix_mark_reset.diff [ 12404736 ]
          Hide
          Kristian Waagan added a comment -

          Attached the correct file this time (4c)...

          Show
          Kristian Waagan added a comment - Attached the correct file this time (4c)...
          Kristian Waagan made changes -
          Hide
          Kristian Waagan added a comment -

          Committed patch 4c to trunk with revision 762384, and to 10.5 with revision 762389.

          I'm marking this issue as resolved, but I may still add the last test improvement I have mentioned.
          I don't believe any more changes are required to fix the reported bug. I don't see the reported problem when running in soft upgrade with the fix, but please verify.

          (BTW, I'll be unavailable for a week from today)

          Show
          Kristian Waagan added a comment - Committed patch 4c to trunk with revision 762384, and to 10.5 with revision 762389. I'm marking this issue as resolved, but I may still add the last test improvement I have mentioned. I don't believe any more changes are required to fix the reported bug. I don't see the reported problem when running in soft upgrade with the fix, but please verify. (BTW, I'll be unavailable for a week from today)
          Kristian Waagan made changes -
          Status In Progress [ 3 ] Resolved [ 5 ]
          Derby Info [Regression, Patch Available] [Regression]
          Fix Version/s 10.5.1.1 [ 12313771 ]
          Fix Version/s 10.6.0.0 [ 12313727 ]
          Resolution Fixed [ 1 ]
          Hide
          Knut Anders Hatlen added a comment -

          Thanks. Keeping the EOFException for consistency with the other methods in the class sounds OK to me.

          Show
          Knut Anders Hatlen added a comment - Thanks. Keeping the EOFException for consistency with the other methods in the class sounds OK to me.
          Hide
          Suran Jayathilaka added a comment -

          Verified the fix with Snapshot build 10.5.1.1_2009-04-07T20-30-19_SVN762838 by running suites.All on a soft upgrade.
          The ClassCastException failures did not occur.

          Show
          Suran Jayathilaka added a comment - Verified the fix with Snapshot build 10.5.1.1_2009-04-07T20-30-19_SVN762838 by running suites.All on a soft upgrade. The ClassCastException failures did not occur.
          Suran Jayathilaka made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Myrna van Lunteren made changes -
          Affects Version/s 10.5.1.0 [ 12313770 ]
          Dag H. Wanvik made changes -
          Component/s Regression Test Failure [ 12310664 ]
          Dag H. Wanvik made changes -
          Component/s Test [ 11413 ]
          Dag H. Wanvik made changes -
          Bug behavior facts [Regression Test Failure] [Regression]
          Dag H. Wanvik made changes -
          Bug behavior facts [Regression] [Regression Test Failure]
          Dag H. Wanvik made changes -
          Bug behavior facts [Regression Test Failure] [Regression, Regression Test Failure]
          Gavin made changes -
          Workflow jira [ 12457455 ] Default workflow, editable Closed status [ 12801661 ]

            People

            • Assignee:
              Kristian Waagan
              Reporter:
              Suran Jayathilaka
            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development