Derby
  1. Derby
  2. DERBY-3574

With client, attempting to get the lob length after commit or connection close if there was a call to length() before commit does not throw an exception

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 10.3.3.0, 10.4.2.0, 10.5.1.1
    • Fix Version/s: 10.4.2.0, 10.5.1.1
    • Component/s: JDBC, Network Client
    • Labels:
      None

      Description

      Attempting to get call Blob/Clob.length() after commit or connection close does not fail if there was a previous call to length(). If no previous call was made an exception is thrown as expected.

      See attached program TestLobLength for repro with commit. If you comment out the two lines to get the length before the commit we get the expected exception.

      1. TestLobLength.java
        1 kB
        Kathey Marsden
      2. derby3574.patch
        8 kB
        Tiago R. Espinha
      3. derby3574-wtest.patch
        9 kB
        Tiago R. Espinha
      4. derby3574-testfix.patch
        1 kB
        Tiago R. Espinha
      5. derby3574-testfix.patch
        2 kB
        Tiago R. Espinha

        Issue Links

          Activity

          Hide
          Tiago R. Espinha added a comment -

          I believe this will fix the issue. The patch is just for Clob.java but the same has to be done for Blob.java, I'll do it as soon as someone with more experience than me can confirm if it's the correct way to do it.

          I ran the test before and after, and it did pass with that change, but it's my first time here so any guidance is appreciated.

          Show
          Tiago R. Espinha added a comment - I believe this will fix the issue. The patch is just for Clob.java but the same has to be done for Blob.java, I'll do it as soon as someone with more experience than me can confirm if it's the correct way to do it. I ran the test before and after, and it did pass with that change, but it's my first time here so any guidance is appreciated.
          Hide
          Kathey Marsden added a comment -

          I spoke with Tiago on IRC about this and we came to the conclusion that.
          1) There would be a problem with the patch if a transaction was committed and then another one began.
          2) The error message should be localized.
          3) It would be nice if checkValidity() could do this check but we don't mark client Lobs as invalid on commit()/rollback().
          4) Length is a problem and not getSubString() etc because we cache the result of length(). I wonder with updatable resultSets if it is even valid to cache this result.

          Show
          Kathey Marsden added a comment - I spoke with Tiago on IRC about this and we came to the conclusion that. 1) There would be a problem with the patch if a transaction was committed and then another one began. 2) The error message should be localized. 3) It would be nice if checkValidity() could do this check but we don't mark client Lobs as invalid on commit()/rollback(). 4) Length is a problem and not getSubString() etc because we cache the result of length(). I wonder with updatable resultSets if it is even valid to cache this result.
          Hide
          V.Narayanan added a comment -

          I have suggested a possible workaround for the related issue Derby-3469. I think the solution
          suggested there if found OK will work for this issue also.

          Show
          V.Narayanan added a comment - I have suggested a possible workaround for the related issue Derby-3469. I think the solution suggested there if found OK will work for this issue also.
          Hide
          Tiago R. Espinha added a comment -

          I have read your solution for this issue although I have thought of something different, and perhaps you can tell me which would be more correct. Keep in mind I still haven't tested this one solution either.

          What if instead of setting lengthObtained_ to false in a free() method on the Lob, we would, instead, extract the isValid property of Clob and Blob to its parent class, the Lob and set it to false on that free() method?

          Then when we call sqlLength() on the Lob we could do something like:
          if(lengthObtained_ && isValid) return sqlLength_;

          I'm just suggesting this solution because it seems more correct to me than just resetting lengthObtained_ to false. By doing this we're pretty much just fooling the mechanism that deals with cache instead of actually checking whether the Lob is valid or not (i.e. whether it has been free()'d or not). What method should I look into doing?

          Show
          Tiago R. Espinha added a comment - I have read your solution for this issue although I have thought of something different, and perhaps you can tell me which would be more correct. Keep in mind I still haven't tested this one solution either. What if instead of setting lengthObtained_ to false in a free() method on the Lob, we would, instead, extract the isValid property of Clob and Blob to its parent class, the Lob and set it to false on that free() method? Then when we call sqlLength() on the Lob we could do something like: if(lengthObtained_ && isValid) return sqlLength_; I'm just suggesting this solution because it seems more correct to me than just resetting lengthObtained_ to false. By doing this we're pretty much just fooling the mechanism that deals with cache instead of actually checking whether the Lob is valid or not (i.e. whether it has been free()'d or not). What method should I look into doing?
          Hide
          V.Narayanan added a comment -

          >What if instead of setting lengthObtained_ to false in a free() method on the Lob,
          >we would, instead, extract the isValid property of Clob and Blob to its parent class,
          >the Lob and set it to false on that free() method?

          I think this is a very good solution because you do not need to make a stored procedure
          call to determine that Lob is no longer valid. (unnecessary round trip to the server).

          >Then when we call sqlLength() on the Lob we could do something like:
          >if(lengthObtained_ && isValid) return sqlLength_;

          I think here if isValid is false you should do this

          throw new SqlException(null,new ClientMessageId(SQLState.LOB_OBJECT_INVALID))
          .getSQLException();

          Show
          V.Narayanan added a comment - >What if instead of setting lengthObtained_ to false in a free() method on the Lob, >we would, instead, extract the isValid property of Clob and Blob to its parent class, >the Lob and set it to false on that free() method? I think this is a very good solution because you do not need to make a stored procedure call to determine that Lob is no longer valid. (unnecessary round trip to the server). >Then when we call sqlLength() on the Lob we could do something like: >if(lengthObtained_ && isValid) return sqlLength_; I think here if isValid is false you should do this throw new SqlException(null,new ClientMessageId(SQLState.LOB_OBJECT_INVALID)) .getSQLException();
          Hide
          Tiago R. Espinha added a comment -

          Ok, perfect! I will look into this today still.

          Show
          Tiago R. Espinha added a comment - Ok, perfect! I will look into this today still.
          Hide
          Tiago R. Espinha added a comment -

          Apparently there's more to it. I've just implemented and tested this solution and the fact is that the TestLobLength still manages to request the length of the Clob, after the transaction has been committed.

          I found one thing: The Clobs effectively aren't being free()'d when a commit is issued on the connection they're bound with. I temporarily encapsulated isValid with a public method and I found out that it is true before we issue the commit, and it remains true after as well.

          The bottom-line is: do we want the Clobs (and Lobs in general) to be free()'d upon the commit? If we don't, then Narayanan's solution will certainly fix this issue (since it doesn't rely on the "validity" of the Lob) and this problem gets dealt with quickly. If we do want them free()'d, then the problem runs deeper than just some work on *lob.java .

          Some thoughts please?

          Show
          Tiago R. Espinha added a comment - Apparently there's more to it. I've just implemented and tested this solution and the fact is that the TestLobLength still manages to request the length of the Clob, after the transaction has been committed. I found one thing: The Clobs effectively aren't being free()'d when a commit is issued on the connection they're bound with. I temporarily encapsulated isValid with a public method and I found out that it is true before we issue the commit, and it remains true after as well. The bottom-line is: do we want the Clobs (and Lobs in general) to be free()'d upon the commit? If we don't, then Narayanan's solution will certainly fix this issue (since it doesn't rely on the "validity" of the Lob) and this problem gets dealt with quickly. If we do want them free()'d, then the problem runs deeper than just some work on *lob.java . Some thoughts please?
          Hide
          Kathey Marsden added a comment -

          Well the lobs are freed on the server with commit/rollback, just not marked invalid on the client. So there is not a need for all of the work of free().

          I hate to keep a list of lobs just for this. Part of me thinks the caching of the length is not such a good idea. If we got rid of that, at least for locators, we wouldn't have this problem.

          Let me think about it some more and perhaps others will have better ideas in the meanwhile.

          Kathey

          Show
          Kathey Marsden added a comment - Well the lobs are freed on the server with commit/rollback, just not marked invalid on the client. So there is not a need for all of the work of free(). I hate to keep a list of lobs just for this. Part of me thinks the caching of the length is not such a good idea. If we got rid of that, at least for locators, we wouldn't have this problem. Let me think about it some more and perhaps others will have better ideas in the meanwhile. Kathey
          Hide
          V.Narayanan added a comment -

          while working on Derby-694 I understood that Lob implemented Event
          callback methods in the UnitOfWorkListener interface. The methods
          are

          public void listenToUnitOfWork();

          public void completeLocalCommit(java.util.Iterator listenerIterator);

          public void completeLocalRollback(java.util.Iterator listenerIterator);

          There are three implementations of the UnitOfWorkListener interface Lob,Statement
          and ResultSet. Each time an object of this type is created they are register themselves
          to the connection by doing the following

          agent_.connection_.CommitAndRollbackListeners_.put(this,null);

          when a End Unit of Work Condition (ENDUOWRM) Reply Mesage is received
          (see line no 738 NetConnectionReply.java) signifying that the unit of work has ended as a
          result of the last command

          a completeLocalCommit() or a completeLocalRollback() is called. These methods are
          also called in other places. Please refer Derby-694 for more background.

          Since the primary problem as pointed out is

          >"Well the lobs are freed on the server with commit/rollback, just not marked invalid on the client."

          Could these method implementations in Lob.java be used to invalidate the Lob
          (isValid=false) when a commit or a rollback happens?

          Not sure, but maybe we need to call free() here.

          Show
          V.Narayanan added a comment - while working on Derby-694 I understood that Lob implemented Event callback methods in the UnitOfWorkListener interface. The methods are public void listenToUnitOfWork(); public void completeLocalCommit(java.util.Iterator listenerIterator); public void completeLocalRollback(java.util.Iterator listenerIterator); There are three implementations of the UnitOfWorkListener interface Lob,Statement and ResultSet. Each time an object of this type is created they are register themselves to the connection by doing the following agent_.connection_.CommitAndRollbackListeners_.put(this,null); when a End Unit of Work Condition (ENDUOWRM) Reply Mesage is received (see line no 738 NetConnectionReply.java) signifying that the unit of work has ended as a result of the last command a completeLocalCommit() or a completeLocalRollback() is called. These methods are also called in other places. Please refer Derby-694 for more background. Since the primary problem as pointed out is >"Well the lobs are freed on the server with commit/rollback, just not marked invalid on the client." Could these method implementations in Lob.java be used to invalidate the Lob (isValid=false) when a commit or a rollback happens? Not sure, but maybe we need to call free() here.
          Hide
          Knut Anders Hatlen added a comment -

          I think that solution would work, but the callback methods in Lob are not currently used (Lob.listenToUnitOfWork() is never called) so they would have to be wired in. I'm not sure we should call free() from the callback methods, since we don't need to call the stored procedures on commit/rollback. Perhaps setting isValid to false is enough.

          Like Kathey, I would prefer not storing the LOBs in a list just to invalidate them on transaction boundaries. What about this alternative:

          • In the connection object, keep a counter which is incremented each time a transaction is committed or aborted.
          • When a Lob object is created, store the current value of the transaction counter in the Lob object.
          • Change checkValidity() so that it checks that the current transaction is the same as the one that created it, something similar to:

          private void checkValidity() throws SQLException

          { if (!isValid || tx != agent_.connection_.txCounter) throw new SqlException(....); }
          Show
          Knut Anders Hatlen added a comment - I think that solution would work, but the callback methods in Lob are not currently used (Lob.listenToUnitOfWork() is never called) so they would have to be wired in. I'm not sure we should call free() from the callback methods, since we don't need to call the stored procedures on commit/rollback. Perhaps setting isValid to false is enough. Like Kathey, I would prefer not storing the LOBs in a list just to invalidate them on transaction boundaries. What about this alternative: In the connection object, keep a counter which is incremented each time a transaction is committed or aborted. When a Lob object is created, store the current value of the transaction counter in the Lob object. Change checkValidity() so that it checks that the current transaction is the same as the one that created it, something similar to: private void checkValidity() throws SQLException { if (!isValid || tx != agent_.connection_.txCounter) throw new SqlException(....); }
          Hide
          Tiago R. Espinha added a comment -

          Okay, so what do we know at this point:

          • Both my solution and Narayanan's don't work since they both rely on a free() being invoked client-side, upon a commit(); . This would require for all the Lobs to be stored in a list so that they could then be freed. Undesirable solution.
          • Knut's solution will most likely work. So unless someone objects to it, I believe it is the way to go. Perhaps instead of a counter I'd create a new UUID upon each commit, that would uniquely identify each transaction, but those are implementation details.
          Show
          Tiago R. Espinha added a comment - Okay, so what do we know at this point: Both my solution and Narayanan's don't work since they both rely on a free() being invoked client-side, upon a commit(); . This would require for all the Lobs to be stored in a list so that they could then be freed. Undesirable solution. Knut's solution will most likely work. So unless someone objects to it, I believe it is the way to go. Perhaps instead of a counter I'd create a new UUID upon each commit, that would uniquely identify each transaction, but those are implementation details.
          Hide
          Tiago R. Espinha added a comment - - edited

          I have attached Knut's implementation with a twist. I did use an integer as a counter, as apparently the UUID class is only for Java 1.5+ and the integer is just as good.

          Anyway, the twist is:
          Instead of doing the check on checkValidity(), I added the transactionID_ to the Lob class. This is better since we want the same check for all kinds of Lobs. Since Lob doesn't have a checkValidity(), I've added the check to the sqlLength() method and I throw an exception there.

          The Connection.java also has a transactionID_ now, which is the "index" of the current transaction. It's private and it should never be altered, except for the increment done on the commit() method.

          Needs to be added that with this solution,TestLobLength does pass.

          Show
          Tiago R. Espinha added a comment - - edited I have attached Knut's implementation with a twist. I did use an integer as a counter, as apparently the UUID class is only for Java 1.5+ and the integer is just as good. Anyway, the twist is: Instead of doing the check on checkValidity(), I added the transactionID_ to the Lob class. This is better since we want the same check for all kinds of Lobs. Since Lob doesn't have a checkValidity(), I've added the check to the sqlLength() method and I throw an exception there. The Connection.java also has a transactionID_ now, which is the "index" of the current transaction. It's private and it should never be altered, except for the increment done on the commit() method. Needs to be added that with this solution,TestLobLength does pass.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for the patch. From a quick look, I have these comments:

          • Connection.java:
            The transactionID variable is only incremented on explicit commit. I think it should also be incremented when the transaction is auto-committed or aborted. Perhaps it's better to do it in Connection.completeLocalCommit() and Connection.completeLocalRollback() instead of Connection.commit()?
          • Lob.java:
            I feel that the check belongs in checkValidity() rather than in sqlLength(). Wouldn't it be possible to move the isValid flag and the checkValidity() method from Blob/Clob into Lob? They do exactly the same work in Blob and Clob, so there's no reason why we can't move them into the Lob class.
          Show
          Knut Anders Hatlen added a comment - Thanks for the patch. From a quick look, I have these comments: Connection.java: The transactionID variable is only incremented on explicit commit. I think it should also be incremented when the transaction is auto-committed or aborted. Perhaps it's better to do it in Connection.completeLocalCommit() and Connection.completeLocalRollback() instead of Connection.commit()? Lob.java: I feel that the check belongs in checkValidity() rather than in sqlLength(). Wouldn't it be possible to move the isValid flag and the checkValidity() method from Blob/Clob into Lob? They do exactly the same work in Blob and Clob, so there's no reason why we can't move them into the Lob class.
          Hide
          Tiago R. Espinha added a comment -

          Added new patches to comply with those changes. The check is done in checkValidity() now, extracted isValid to Lob.java and changed the place where transactionID is incremented.

          Show
          Tiago R. Espinha added a comment - Added new patches to comply with those changes. The check is done in checkValidity() now, extracted isValid to Lob.java and changed the place where transactionID is incremented.
          Hide
          Kristian Waagan added a comment -

          Hi Tiago,

          I haven't looked at the code changes in detail, but I did take a quick look at the uploaded patches.
          You are of course free to upload one patch per file, but I find it easier to apply fixes when there is only one diff per "change iteration" / chunk of work.
          Also, I believe the usual pattern is to generate the diffs from the within the trunk directory so that you can apply the patch using "patch -p0 < patch.diff" from the root directory (i.e. the path / context in the diff would be "java/client/org/apache/...").

          A few suggestions;

          • use JavaDoc instead of normal comments for instance variables
          • avoid whitespace changes and trailing whitespace (see for instance the patch for Connection)
          • avoid mixing tabs and spaces for indentation

          A committer might still commit the patch even if these guidelines aren't followed, but following them tends to speed up the review and commit process.
          Thank you for your time and contribution

          Show
          Kristian Waagan added a comment - Hi Tiago, I haven't looked at the code changes in detail, but I did take a quick look at the uploaded patches. You are of course free to upload one patch per file, but I find it easier to apply fixes when there is only one diff per "change iteration" / chunk of work. Also, I believe the usual pattern is to generate the diffs from the within the trunk directory so that you can apply the patch using "patch -p0 < patch.diff" from the root directory (i.e. the path / context in the diff would be "java/client/org/apache/..."). A few suggestions; use JavaDoc instead of normal comments for instance variables avoid whitespace changes and trailing whitespace (see for instance the patch for Connection) avoid mixing tabs and spaces for indentation A committer might still commit the patch even if these guidelines aren't followed, but following them tends to speed up the review and commit process. Thank you for your time and contribution
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for the new patch! Apart from what Kristian said, I think we should also update the javadoc for the checkValidity() method so that it reflects the new behaviour (it should mention that it also checks that the transaction that created it is still active). The code changes look correct to me.

          Show
          Knut Anders Hatlen added a comment - Thanks for the new patch! Apart from what Kristian said, I think we should also update the javadoc for the checkValidity() method so that it reflects the new behaviour (it should mention that it also checks that the transaction that created it is still active). The code changes look correct to me.
          Hide
          Tiago R. Espinha added a comment -

          My apologies Kristian. This is actually the first time I created diff files so I'm still in the process of learning. I found a website with basic instructions on using the diff tool but if someone can point me in the direction of something more useful for this specific case (or point me to some tool with a GUI that makes things easier for us..), I'd appreciate it. I tried diff'ing the whole directory and it errored out, claiming it had exhausted the memory.

          As for the other suggestions, I'll certainly follow them in future patches.

          Show
          Tiago R. Espinha added a comment - My apologies Kristian. This is actually the first time I created diff files so I'm still in the process of learning. I found a website with basic instructions on using the diff tool but if someone can point me in the direction of something more useful for this specific case (or point me to some tool with a GUI that makes things easier for us..), I'd appreciate it. I tried diff'ing the whole directory and it errored out, claiming it had exhausted the memory. As for the other suggestions, I'll certainly follow them in future patches.
          Hide
          Kathey Marsden added a comment -

          You can use svn diff from the top of the source tree (on directory up from java).

          svn diff > patch_filename

          Show
          Kathey Marsden added a comment - You can use svn diff from the top of the source tree (on directory up from java). svn diff > patch_filename
          Hide
          Tiago R. Espinha added a comment -

          Thank you Kathey, that made my life much easier. Added a new patch with all the changes and some more fixes to comments.

          Show
          Tiago R. Espinha added a comment - Thank you Kathey, that made my life much easier. Added a new patch with all the changes and some more fixes to comments.
          Hide
          Kathey Marsden added a comment -

          Thanks Tiago for the patch. The change looks good for commit/rollback. Does the change work after connection close? We should add a regression test. There are currently fixtures in BlobClob4BLOBTest, testB/ClobAfterCommit, and testB/ClobAfterConnectionClose where the tests could be added.

          With your new patch you can just attach it and leave the old one for reference.
          Kathey

          Show
          Kathey Marsden added a comment - Thanks Tiago for the patch. The change looks good for commit/rollback. Does the change work after connection close? We should add a regression test. There are currently fixtures in BlobClob4BLOBTest, testB/ClobAfterCommit, and testB/ClobAfterConnectionClose where the tests could be added. With your new patch you can just attach it and leave the old one for reference. Kathey
          Hide
          Tiago R. Espinha added a comment - - edited

          I have just talked with Kathey on IRC and here's the main topics:

          1) Initially, with my patch, if you tried to invoke length() on a Lob (over a connection that had been closed), you'd get the INVALID_LOB exception instead of the expected NO_CURRENT_CONNECTION.

          2) The testB/ClobAfterCommit() was always passing because it wasn't replicating the original issue: cached results. Therefore a change was made so that length() is invoked before the commit(), caching the result, and it is then invoked for real after the commit(), where it should throw the expected exception.

          3) To fix the problem in point 1, checkValidity() was changed once again, to include a checkForClosedConnection() .

          All the tests in BlobClob4BlobTest are successful now, and these changes are available on the latest patch (including the regression test changes).

          Show
          Tiago R. Espinha added a comment - - edited I have just talked with Kathey on IRC and here's the main topics: 1) Initially, with my patch, if you tried to invoke length() on a Lob (over a connection that had been closed), you'd get the INVALID_LOB exception instead of the expected NO_CURRENT_CONNECTION. 2) The testB/ClobAfterCommit() was always passing because it wasn't replicating the original issue: cached results. Therefore a change was made so that length() is invoked before the commit(), caching the result, and it is then invoked for real after the commit(), where it should throw the expected exception. 3) To fix the problem in point 1, checkValidity() was changed once again, to include a checkForClosedConnection() . All the tests in BlobClob4BlobTest are successful now, and these changes are available on the latest patch (including the regression test changes).
          Hide
          Kathey Marsden added a comment -

          Changes look good. Could you run regression tests with your patch and let us know how it goes?

          Thanks

          Kathey

          Show
          Kathey Marsden added a comment - Changes look good. Could you run regression tests with your patch and let us know how it goes? Thanks Kathey
          Hide
          Tiago R. Espinha added a comment -

          I have ran derbyall but I can't seem to find suites.all like you told me to run, am I doing something wrong?

          Here's the result for derbyall:

          ------------------------------------------------------
          ------------------------------------------------------
          Summary results:

          Test Run Started: 2008-04-11 20:21:52.0
          Test Run Duration: 01:27:29

          249 Tests Run
          100% Pass (249 tests passed)
          0% Fail (0 tests failed)
          7 Suites skipped
          ------------------------------------------------------
          Passed tests in: derbyall_pass.txt
          ------------------------------------------------------
          Skipped suites in: derbyall_skip.txt
          ------------------------------------------------------
          System properties in: derbyall_prop.txt
          ------------------------------------------------------
          ------------------------------------------------------
          No Failures.
          ------------------------------------------------------

          Show
          Tiago R. Espinha added a comment - I have ran derbyall but I can't seem to find suites.all like you told me to run, am I doing something wrong? Here's the result for derbyall: ------------------------------------------------------ ------------------------------------------------------ Summary results: Test Run Started: 2008-04-11 20:21:52.0 Test Run Duration: 01:27:29 249 Tests Run 100% Pass (249 tests passed) 0% Fail (0 tests failed) 7 Suites skipped ------------------------------------------------------ Passed tests in: derbyall_pass.txt ------------------------------------------------------ Skipped suites in: derbyall_skip.txt ------------------------------------------------------ System properties in: derbyall_prop.txt ------------------------------------------------------ ------------------------------------------------------ No Failures. ------------------------------------------------------
          Hide
          Bryan Pendleton added a comment -

          http://wiki.apache.org/db-derby/DerbyTopLevelJunitTests
          has information on how to run the various Junit tests.

          Note that you can either run them via "ant" commands, or directly by invoking Junit.

          Show
          Bryan Pendleton added a comment - http://wiki.apache.org/db-derby/DerbyTopLevelJunitTests has information on how to run the various Junit tests. Note that you can either run them via "ant" commands, or directly by invoking Junit.
          Hide
          Kathey Marsden added a comment -

          For some reason I had thought it was in README.htm but I guess not.
          Here is the wiki page that talks about running the tests.
          http://wiki.apache.org/db-derby/DerbyJUnitTesting?highlight=%28JunitTesting%29#head-216e836b20ddca6253e67308abdabb545416232a

          When running with the textui runner I usually run with extra memory e.g.
          java -XX:MaxPermSize=128M -Xmx512M -Xms512M -DderbyTesting.oldReleasePath=C:/kmarsden/svnreleases/jars junit.textui.TestRunner org.apache.derbyTesting.functionTests.suites.All

          Show
          Kathey Marsden added a comment - For some reason I had thought it was in README.htm but I guess not. Here is the wiki page that talks about running the tests. http://wiki.apache.org/db-derby/DerbyJUnitTesting?highlight=%28JunitTesting%29#head-216e836b20ddca6253e67308abdabb545416232a When running with the textui runner I usually run with extra memory e.g. java -XX:MaxPermSize=128M -Xmx512M -Xms512M -DderbyTesting.oldReleasePath=C:/kmarsden/svnreleases/jars junit.textui.TestRunner org.apache.derbyTesting.functionTests.suites.All
          Hide
          Tiago R. Espinha added a comment -

          Ah, perfect! I'm running that test right now, I'll post the results as soon as I have them.

          Show
          Tiago R. Espinha added a comment - Ah, perfect! I'm running that test right now, I'll post the results as soon as I have them.
          Hide
          Tiago R. Espinha added a comment -

          Ok I have a problem.

          I ran the command that Kathey posted (without the -D switch and with -Xmx1024M since it gave out of memory with just 512MiB) and I got a stackload of error lines after a few hours of running. Here's what it said in the end:

          FAILURES!!!
          Tests run: 8010, Failures: 1, Errors: 8

          Somehow I get the feeling that this is still a memory issue. I'm running the tests in a Virtual Machine running Ubuntu 7.10 with just 1GiB of RAM, and after a while, java is using all the RAM and has entered the swap file. Should I discard the VM and run it all natively on XP where I have 2GiB of RAM?

          Show
          Tiago R. Espinha added a comment - Ok I have a problem. I ran the command that Kathey posted (without the -D switch and with -Xmx1024M since it gave out of memory with just 512MiB) and I got a stackload of error lines after a few hours of running. Here's what it said in the end: FAILURES!!! Tests run: 8010, Failures: 1, Errors: 8 Somehow I get the feeling that this is still a memory issue. I'm running the tests in a Virtual Machine running Ubuntu 7.10 with just 1GiB of RAM, and after a while, java is using all the RAM and has entered the swap file. Should I discard the VM and run it all natively on XP where I have 2GiB of RAM?
          Hide
          Tiago R. Espinha added a comment -

          I ran the tests again (suites.all) natively on my Windows XP and here's the result:

          Time: 6.169

          OK (9830 tests)

          The derbyall suite also had a 100% success rate.

          Apparently there is a problem while running suites.All under Ubuntu 7.10 (refer to DERBY-3616) and that's the reason why my tests were failing, and not something patch-related. Therefore I think it's safe to say that this patch can be applied to the trunk.

          Show
          Tiago R. Espinha added a comment - I ran the tests again (suites.all) natively on my Windows XP and here's the result: Time: 6.169 OK (9830 tests) The derbyall suite also had a 100% success rate. Apparently there is a problem while running suites.All under Ubuntu 7.10 (refer to DERBY-3616 ) and that's the reason why my tests were failing, and not something patch-related. Therefore I think it's safe to say that this patch can be applied to the trunk.
          Hide
          Kathey Marsden added a comment -

          I committed revision 647931 for this issue. Thank you Tiago for the patch. Could you submit a followup patch with the tests for length() after connection.close?

          Kathey

          Show
          Kathey Marsden added a comment - I committed revision 647931 for this issue. Thank you Tiago for the patch. Could you submit a followup patch with the tests for length() after connection.close? Kathey
          Hide
          Tiago R. Espinha added a comment -

          Followup patch as suggested by Kathey submitted.

          Show
          Tiago R. Espinha added a comment - Followup patch as suggested by Kathey submitted.
          Hide
          Knut Anders Hatlen added a comment -

          The committed patch (rev #647931) contained many whitespace changes (adding trailing blanks, extra blank lines and replacing spaces with tabs) as mentioned in Kristian's review. I cleaned it up and committed revision 648180.

          Show
          Knut Anders Hatlen added a comment - The committed patch (rev #647931) contained many whitespace changes (adding trailing blanks, extra blank lines and replacing spaces with tabs) as mentioned in Kristian's review. I cleaned it up and committed revision 648180.
          Hide
          Knut Anders Hatlen added a comment -

          Do we really need this extra check in checkValidity()?

          + /**
          + * If there isn't an open connection, the Lob is invalid.
          + */
          + try

          { + agent_.connection_.checkForClosedConnection(); + }

          catch(SqlException se)

          { + throw se.getSQLException(); + }

          If I am not mistaken, closing the connection is only allowed if there is no active transaction on the connection, which means that Connection.transactionID != Lob.transactionID so that checkValidity() will throw an exception anyway.

          Show
          Knut Anders Hatlen added a comment - Do we really need this extra check in checkValidity()? + /** + * If there isn't an open connection, the Lob is invalid. + */ + try { + agent_.connection_.checkForClosedConnection(); + } catch(SqlException se) { + throw se.getSQLException(); + } If I am not mistaken, closing the connection is only allowed if there is no active transaction on the connection, which means that Connection.transactionID != Lob.transactionID so that checkValidity() will throw an exception anyway.
          Hide
          Tiago R. Espinha added a comment -

          If I recall correctly, that check is to comply with expected behaviour, namely the testC/BlobAfterClosingConnection(). This test expects to get a NO_CURRENT_CONNECTION after a close(); is invoked on the connection and we attempt to access the contents of a C/Lob. If that check isn't in place, the length(); call will throw a INVALID_LOB instead.

          Basically that check is pretty much to comply with the regression tests.

          Show
          Tiago R. Espinha added a comment - If I recall correctly, that check is to comply with expected behaviour, namely the testC/BlobAfterClosingConnection(). This test expects to get a NO_CURRENT_CONNECTION after a close(); is invoked on the connection and we attempt to access the contents of a C/Lob. If that check isn't in place, the length(); call will throw a INVALID_LOB instead. Basically that check is pretty much to comply with the regression tests.
          Hide
          Knut Anders Hatlen added a comment -

          OK, thanks for the explanation. I don't think it matters whether we throw NO_CURRENT_CONNECTION or INVALID_LOB, since both are true. So I think I would have preferred simplifying the code and changing the test to accept the other exception.

          Show
          Knut Anders Hatlen added a comment - OK, thanks for the explanation. I don't think it matters whether we throw NO_CURRENT_CONNECTION or INVALID_LOB, since both are true. So I think I would have preferred simplifying the code and changing the test to accept the other exception.
          Hide
          Tiago R. Espinha added a comment -

          This is true, I just thought that since the connection was closed, on top of the Lob being invalid, we don't have a connection anymore. That was the logic I followed to choose to do that check instead (and I believe it was also Kathey who suggested this approach). I reckon though that in the end, changing the test would simplify the code and the purpose would be met anyways.

          If needed I can revert this to your way.

          Show
          Tiago R. Espinha added a comment - This is true, I just thought that since the connection was closed, on top of the Lob being invalid, we don't have a connection anymore. That was the logic I followed to choose to do that check instead (and I believe it was also Kathey who suggested this approach). I reckon though that in the end, changing the test would simplify the code and the purpose would be met anyways. If needed I can revert this to your way.
          Hide
          Knut Anders Hatlen added a comment -

          It's not a big deal. The extra check is probably not that expensive, so I'm fine with either of the approaches. It seems like the current approach matches what the embedded driver does.

          Show
          Knut Anders Hatlen added a comment - It's not a big deal. The extra check is probably not that expensive, so I'm fine with either of the approaches. It seems like the current approach matches what the embedded driver does.
          Hide
          Tiago R. Espinha added a comment -

          Issue resolved. Final revision regarding this issue: 648409 .

          Show
          Tiago R. Espinha added a comment - Issue resolved. Final revision regarding this issue: 648409 .
          Hide
          Kristian Waagan added a comment -

          Updating affects and fix versions.
          I plan to port this to the 10.4 branch, as it also fixes the bug reported as DERBY-3469.

          Show
          Kristian Waagan added a comment - Updating affects and fix versions. I plan to port this to the 10.4 branch, as it also fixes the bug reported as DERBY-3469 .
          Hide
          Kristian Waagan added a comment -

          Backported fix to the 10.4 branch with revision 660187.

          Show
          Kristian Waagan added a comment - Backported fix to the 10.4 branch with revision 660187.
          Hide
          Rick Hillegas added a comment -

          Does this issue need a release note? With this fix, something which used to succeed now fails.

          Show
          Rick Hillegas added a comment - Does this issue need a release note? With this fix, something which used to succeed now fails.

            People

            • Assignee:
              Tiago R. Espinha
              Reporter:
              Kathey Marsden
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development