Derby
  1. Derby
  2. DERBY-3650

internal multiple references from different rows to a single BLOB/CLOB stream leads to various errors when second reference used.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.3.3.0, 10.4.1.3, 10.5.3.0, 10.6.1.0
    • Fix Version/s: 10.6.1.0
    • Component/s: Network Client, SQL, Store
    • Environment:
      Mac OSX 10.4
      JDK 1.5.0_13
      Hibernate EntityManager 3.2.1
    • Urgency:
      Normal
    • Issue & fix info:
      Known fix, Repro attached
    • Bug behavior facts:
      Regression

      Description

      Derby + Hibernate JPA 3.2.1 problem on entity with Blob/Clob

      Hi,

      I'm using Derby in Client - Server mode with Hibernate JPA EJB 3.0.
      When a query on an entity containing a Clob and some joins on other entites is executed, an exception with the following message is thrown:
      XJ073: The data in this BLOB or CLOB is no longer available. The BLOB/CLOB's transaction may be committed, or its connection is closed.

      This problem occurs when the property "hibernate.max_fetch_depth" is greater than 0.
      When hibernate.max_fetch_depth=0, the query works.

      If Derby is configured in embedded mode, the query works independently of the value of hibernate.max_fetch_depth.

      On the Hibernate's documentation, the advised value of hibernate.max_fetch_depth is 3.
      Could you explain me if I made something wrong ?

      Thank you.
      Stephane

      1. UnionAll.java
        2 kB
        Kathey Marsden
      2. traces_on_FormatIdStream_alloc.txt
        4 kB
        Kathey Marsden
      3. testdb.zip
        124 kB
        Kristian Waagan
      4. DerbyHibernateTest.zip
        89 kB
        Golgoth 14
      5. Derby3650Repro.java
        1 kB
        Kristian Waagan
      6. derby-3650-preliminary_diff.txt
        35 kB
        Mike Matrigali
      7. derby-3650-preliminary_2_reworked.diff
        5 kB
        Kristian Waagan
      8. derby-3650-preliminary_2_diff.txt
        44 kB
        Mike Matrigali
      9. Derby3650FullRepro.java
        2 kB
        Kathey Marsden
      10. Derby3650FullClientRepro.java
        2 kB
        Kathey Marsden
      11. Derby3650EmbeddedRepro.java
        1 kB
        Kathey Marsden
      12. derby-3650_tests_diff.txt
        8 kB
        Kathey Marsden
      13. cloning-methods.html
        5 kB
        Kristian Waagan
      14. ASF.LICENSE.NOT.GRANTED--derby-3650-preliminary_2_reworked-1b.diff
        5 kB
        Kristian Waagan

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          [bulk update] Close all resolved issues that haven't been updated for more than one year.

          Show
          Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.
          Hide
          Kathey Marsden added a comment -

          reclosing. This will come off the backport list. Comments say:
          Note that this fix won't be back-ported, because the underlying clone functionality is hard to back-port (major changes).

          I added the derby_backport_reject_10_5 label.

          Show
          Kathey Marsden added a comment - reclosing. This will come off the backport list. Comments say: Note that this fix won't be back-ported, because the underlying clone functionality is hard to back-port (major changes). I added the derby_backport_reject_10_5 label.
          Hide
          Kathey Marsden added a comment -

          Reopen for backport.

          Show
          Kathey Marsden added a comment - Reopen for backport.
          Hide
          Kristian Waagan added a comment -

          Unless problems arise, I don't expect to do more work on this issue. Resolving as fixed.

          Show
          Kristian Waagan added a comment - Unless problems arise, I don't expect to do more work on this issue. Resolving as fixed.
          Hide
          Kristian Waagan added a comment -

          Thanks for the review, Dag.
          Attached derby-3650-preliminary_2_reworked-1b.diff, in which I removed the unnecessary formatting change.

          Committed to trunk with revision 935336.
          Note that this fix won't be back-ported, because the underlying clone functionality is hard to back-port (major changes).

          Show
          Kristian Waagan added a comment - Thanks for the review, Dag. Attached derby-3650-preliminary_2_reworked-1b.diff, in which I removed the unnecessary formatting change. Committed to trunk with revision 935336. Note that this fix won't be back-ported, because the underlying clone functionality is hard to back-port (major changes).
          Hide
          Dag H. Wanvik added a comment -

          The patch looks good to me. Nit: the patch for NestedLoopResultSet has a formatting diff of old code which doesn't seem to change anything except layout (line 43).

          Show
          Dag H. Wanvik added a comment - The patch looks good to me. Nit: the patch for NestedLoopResultSet has a formatting diff of old code which doesn't seem to change anything except layout (line 43).
          Hide
          Kristian Waagan added a comment -

          The regression tests (suites.All, derbyall) with 'derby-3650-preliminary_2_reworked.diff' applied ran without failures.

          Show
          Kristian Waagan added a comment - The regression tests (suites.All, derbyall) with 'derby-3650-preliminary_2_reworked.diff' applied ran without failures.
          Hide
          Kristian Waagan added a comment -

          Mike wrote:


          I think this would build fine on top of the clone changes that Kristian is working on in DERBY-4520.


          You're probably right, but I think it's time to start ripping something out soon

          At least for my own work I feel very much that I have been building my way around various obstacles. This has caused the complexity to increase, and I hope that by taking a step back much of it can be reduced.
          The good thing is that most of the added complexity has been added for BLOB and CLOB. That said, I'm not happy about the state of SQLChar. Maybe it is just because I associate it too much with the SQL data type CHAR, but in theory the code dealing with CHAR (and VARCHAR) should be a lot simpler than the code dealing with CLOB.

          Show
          Kristian Waagan added a comment - Mike wrote: I think this would build fine on top of the clone changes that Kristian is working on in DERBY-4520 . You're probably right, but I think it's time to start ripping something out soon At least for my own work I feel very much that I have been building my way around various obstacles. This has caused the complexity to increase, and I hope that by taking a step back much of it can be reduced. The good thing is that most of the added complexity has been added for BLOB and CLOB. That said, I'm not happy about the state of SQLChar. Maybe it is just because I associate it too much with the SQL data type CHAR, but in theory the code dealing with CHAR (and VARCHAR) should be a lot simpler than the code dealing with CLOB.
          Hide
          Kristian Waagan added a comment -

          Attached patch 'derby-3650-preliminary_2_reworked.diff'.
          I updated it with regard to the latest changes done on trunk. The rest of the original patch has now been committed under other issues (the test was committed a long time ago).
          I'm running the regression tests to see what the current state of the patch is, and will post the results tomorrow.

          Would be nice if someone had another look at the patch.
          Note that even with the patch committed there are use cases where Derby will fail. This is most likely due to DERBY-1511 (DERBY-3749 is marked as a duplicate).

          Show
          Kristian Waagan added a comment - Attached patch 'derby-3650-preliminary_2_reworked.diff'. I updated it with regard to the latest changes done on trunk. The rest of the original patch has now been committed under other issues (the test was committed a long time ago). I'm running the regression tests to see what the current state of the patch is, and will post the results tomorrow. Would be nice if someone had another look at the patch. Note that even with the patch committed there are use cases where Derby will fail. This is most likely due to DERBY-1511 ( DERBY-3749 is marked as a duplicate).
          Hide
          Mike Matrigali added a comment -

          I don't remember what came up when I was first working on this, and have not done anything on it other than post the patch a long time ago. I wasn't really happy with the complexity, and was hoping to come up with something better but never did.

          I don't plan on doing anything else with this patch but am happy if someone else wants to run with it.

          What I would really like to do is change the underlying implementation of streams coming out of the store so that they can easily be cloned completely and not have their state shared. Basically store should return an object that
          just has an id that can be used to open a new stream that is independent of
          other using that id to open the stream. I think this could solve a number of
          problems with current undefined behavior of sessions accessing the same
          clob multiple times.

          I think this would build fine on top of the clone changes that Kristian is working on in DERBY-4520.

          Show
          Mike Matrigali added a comment - I don't remember what came up when I was first working on this, and have not done anything on it other than post the patch a long time ago. I wasn't really happy with the complexity, and was hoping to come up with something better but never did. I don't plan on doing anything else with this patch but am happy if someone else wants to run with it. What I would really like to do is change the underlying implementation of streams coming out of the store so that they can easily be cloned completely and not have their state shared. Basically store should return an object that just has an id that can be used to open a new stream that is independent of other using that id to open the stream. I think this could solve a number of problems with current undefined behavior of sessions accessing the same clob multiple times. I think this would build fine on top of the clone changes that Kristian is working on in DERBY-4520 .
          Hide
          Kristian Waagan added a comment -

          I have created DERBY-4520 to track the clone-work.

          Show
          Kristian Waagan added a comment - I have created DERBY-4520 to track the clone-work.
          Hide
          Kristian Waagan added a comment - - edited

          I have been investigating this a bit further, and I'll try to share some of my findings.
          My experiments consisted of the following high-level changes:
          a) add mechanism to clone store streams
          b) remove CloneableObject interface (and the method cloneObject)
          c) make DataValueDescriptor.getClone smarter (or more complex...)
          d) enable streaming support in the sorter
          e) add a special-case clone method for obtaining a materialized clone

          a) add mechanism to clone store streams
          Here I reused Mike's patch for DERBY-3650. I haven't found any problems with this yet, but haven't investigated very well.
          I'm wondering if we can optimize the cloning a little bit by deferring the initial buffer fill? This is to avoid reading bytes we might never pass on to the user. I tried once and got a few test failures (probably because the exception was thrown in a different place, for instance that it is now thrown during the first InputStream.read instead of some other method).
          See (d) for an additional problem associated with this change.

          b) remove CloneableObject interface (and the method cloneObject)
          This change in itself didn't cause any trouble when combined with (a) and (c). When combined with (d) an ASSERT was thrown, but I haven't yet investigated if it is a real problem or not.

          c) make DataValueDescriptor.getClone smarter (or more complex...)
          Here I made getClone return the most appropriate clone based on the state of the DVD:

          • simple types: normal clone (i.e. new SQLInteger(this.value) or new SQLClob(this.getString())
          • source is a cloneable stream: clone the stream
          • source is a non-cloneable stream: materialize and return normal clone

          Again, I have to investigate more, but there seems to be a need for a "transfer the state of that DVD to a new DVD"-method. This is different from a clone in the sense that the original DVD will be reused for another row and have its value overwritten. In this case there is no need to actually clone the source stream, we can just reuse the stream reference. This is what cloneObject does.

          d) enable streaming support in the sorter
          When I did this, I found a bug where Derby enters an infinite loop while reading a stream: DERBY-4520
          Another problem that surfaced is that the sorter closes the source result set immediately, before the values are actually processed / sorted. This caused the cloned streams to fail when processing them, because the associated container handle got closed. I tried naively to not close the source rs, but this caused some problems when running suites.All (asserts, lock timeouts). Maybe the sorter can be changed to make sure the source rs is closed at another point, but this seems like a potentially dangerous approach.
          Instead I added a new method, described in (e).

          For clarity, Derby isn't currently able to efficiently execute something like "select ... order by length(clob_column)". There are user workarounds for this problem, so I'm not sure fixing it should have a high priority at this point. Also, the LOB values cannot be used in an order by. I don't know which types of operations you can do in an order by, and whether it is possible to perform these immediately instead of first reading the source rs into a temporary holder and then applying the function later.

          e) add a special-case clone method for obtaining a materialized clone
          Added to make (d) work in an easy way. suites.All passed when using this in a single place (the sorter), but there might be other usages for it as well.
          By default the method will simply forward to getClone, but for SQLChar and SQLBinary it will materialize the stream if required.

          With all the changes combined (prototype quality, I must recheck to make sure I didn't cheat too much), only lang.StreamsTest failed (on line 243) failed. The difference was that with the changes the value was materialized, whereas with clean trunk the stream was passed directly into store. The root cause is that I removed the "transfer value" functionality of cloneObject, and produced a real clone instead. The reason my smarter getClone method failed to produce a clone with a stream as source was that the source stream wasn't a store stream and thus the only way to safely clone it is to materialize it.

          For Derby to function and keep its current performance, I see the need for the following functionality:
          1) value clones (capable of cloning source streams when possible)
          2) forcing materialization
          3) copying state from one DVD to a new DVD

          I don't think all three can be combined into one method, because it is impossible for this method to know the context in which the "clone" will be used.
          It is important to keep in mind that for many of the data types there is no difference between items 1,2, and 3.

          Now, how does my changes differ from the original copyForRead method added by Mike?
          DVD.copyForRead can simply return a reference to itself ('this'). Doing this is the cheapest way to copy a DVD, but it is also the way which puts the most restrictions on how it can be used. Since there will be multiple references to the same DVD, a single update or state change will affect all the code referring to that DVD.
          This can be exploited for better performance in some cases, but I'm not sure if we should leave the decision to the calling code (using the public interface of DVD), or if we should either create a new method (like copyForRead) or add arguments to the getClone method.
          Forcing materialization can also be done explicitly by the calling code, but it wouldn't look too nice:
          if (dvd instanceof StreamStorable)

          { // Assuming calling this when there is no stream is working, otherwise one have to do another check is stream != null dvd.loadStream(); }

          clone = dvd.getClone();

          Opinions?

          Show
          Kristian Waagan added a comment - - edited I have been investigating this a bit further, and I'll try to share some of my findings. My experiments consisted of the following high-level changes: a) add mechanism to clone store streams b) remove CloneableObject interface (and the method cloneObject) c) make DataValueDescriptor.getClone smarter (or more complex...) d) enable streaming support in the sorter e) add a special-case clone method for obtaining a materialized clone a) add mechanism to clone store streams Here I reused Mike's patch for DERBY-3650 . I haven't found any problems with this yet, but haven't investigated very well. I'm wondering if we can optimize the cloning a little bit by deferring the initial buffer fill? This is to avoid reading bytes we might never pass on to the user. I tried once and got a few test failures (probably because the exception was thrown in a different place, for instance that it is now thrown during the first InputStream.read instead of some other method). See (d) for an additional problem associated with this change. b) remove CloneableObject interface (and the method cloneObject) This change in itself didn't cause any trouble when combined with (a) and (c). When combined with (d) an ASSERT was thrown, but I haven't yet investigated if it is a real problem or not. c) make DataValueDescriptor.getClone smarter (or more complex...) Here I made getClone return the most appropriate clone based on the state of the DVD: simple types: normal clone (i.e. new SQLInteger(this.value) or new SQLClob(this.getString()) source is a cloneable stream: clone the stream source is a non-cloneable stream: materialize and return normal clone Again, I have to investigate more, but there seems to be a need for a "transfer the state of that DVD to a new DVD"-method. This is different from a clone in the sense that the original DVD will be reused for another row and have its value overwritten. In this case there is no need to actually clone the source stream, we can just reuse the stream reference. This is what cloneObject does. d) enable streaming support in the sorter When I did this, I found a bug where Derby enters an infinite loop while reading a stream: DERBY-4520 Another problem that surfaced is that the sorter closes the source result set immediately, before the values are actually processed / sorted. This caused the cloned streams to fail when processing them, because the associated container handle got closed. I tried naively to not close the source rs, but this caused some problems when running suites.All (asserts, lock timeouts). Maybe the sorter can be changed to make sure the source rs is closed at another point, but this seems like a potentially dangerous approach. Instead I added a new method, described in (e). For clarity, Derby isn't currently able to efficiently execute something like "select ... order by length(clob_column)". There are user workarounds for this problem, so I'm not sure fixing it should have a high priority at this point. Also, the LOB values cannot be used in an order by. I don't know which types of operations you can do in an order by, and whether it is possible to perform these immediately instead of first reading the source rs into a temporary holder and then applying the function later. e) add a special-case clone method for obtaining a materialized clone Added to make (d) work in an easy way. suites.All passed when using this in a single place (the sorter), but there might be other usages for it as well. By default the method will simply forward to getClone, but for SQLChar and SQLBinary it will materialize the stream if required. With all the changes combined (prototype quality, I must recheck to make sure I didn't cheat too much), only lang.StreamsTest failed (on line 243) failed. The difference was that with the changes the value was materialized, whereas with clean trunk the stream was passed directly into store. The root cause is that I removed the "transfer value" functionality of cloneObject, and produced a real clone instead. The reason my smarter getClone method failed to produce a clone with a stream as source was that the source stream wasn't a store stream and thus the only way to safely clone it is to materialize it. For Derby to function and keep its current performance, I see the need for the following functionality: 1) value clones (capable of cloning source streams when possible) 2) forcing materialization 3) copying state from one DVD to a new DVD I don't think all three can be combined into one method, because it is impossible for this method to know the context in which the "clone" will be used. It is important to keep in mind that for many of the data types there is no difference between items 1,2, and 3. Now, how does my changes differ from the original copyForRead method added by Mike? DVD.copyForRead can simply return a reference to itself ('this'). Doing this is the cheapest way to copy a DVD, but it is also the way which puts the most restrictions on how it can be used. Since there will be multiple references to the same DVD, a single update or state change will affect all the code referring to that DVD. This can be exploited for better performance in some cases, but I'm not sure if we should leave the decision to the calling code (using the public interface of DVD), or if we should either create a new method (like copyForRead) or add arguments to the getClone method. Forcing materialization can also be done explicitly by the calling code, but it wouldn't look too nice: if (dvd instanceof StreamStorable) { // Assuming calling this when there is no stream is working, otherwise one have to do another check is stream != null dvd.loadStream(); } clone = dvd.getClone(); Opinions?
          Hide
          Dag H. Wanvik added a comment - - edited

          Ok, bear with me if I misunderstand issues here, still trying to grok
          this, but I'll weigh in just to get some discussion going.

          I looked at the clone methods, and it seems to me that originally
          there was getClone, and that cloneObject was introduced later to avoid
          always materializing large objects into many copies. The naming is not
          good, the names imply the same behavior, I think. cloneObject is
          shallow in the sense that it does not clone the value, nor does it
          clone the stream state, if any. (Btw, the implementation of
          SQLChar#cloneObject could be simplified to look the same as
          SQLBinary#cloneObject).

          Now, if I understand correctly, the new method, CopyForRead is slightly
          less shallow, in that you now also copy the stream state.

          For non-stream data types, cloneObject defaults to getClone (deep
          copy).

          I would suggest we change the names here to clarify code and APIs, the
          better to reflect the shallowness of the cloning.

          cloneDeep (old getClone; clones even value, share nothing)
          cloneHalfDeep (new CopyForRead, clones even stream state,
          but not value, which is still shared)
          cloneShallow (old cloneObject, clones just "holder", shares
          stream/stream state)

          If the code really needs cloneShallow also, after cloneHalfDeep is
          added, I don't know, if not, I'd call cloneHalfDeep cloneShallow
          instead

          Now, for modification, what to use? I guess that depends on what
          semantics are desired/at what level in the code you are..? Maybe we
          could just do COW semantics? I.e. use cloneHalfDeep until update is
          attempted and only then do a full deep clone? (by overloading stream
          class perhaps) Then the updating of the deep copy could proceed until
          the column is actually updated without affecting the other aliases?

          Show
          Dag H. Wanvik added a comment - - edited Ok, bear with me if I misunderstand issues here, still trying to grok this, but I'll weigh in just to get some discussion going. I looked at the clone methods, and it seems to me that originally there was getClone, and that cloneObject was introduced later to avoid always materializing large objects into many copies. The naming is not good, the names imply the same behavior, I think. cloneObject is shallow in the sense that it does not clone the value , nor does it clone the stream state , if any. (Btw, the implementation of SQLChar#cloneObject could be simplified to look the same as SQLBinary#cloneObject). Now, if I understand correctly, the new method, CopyForRead is slightly less shallow , in that you now also copy the stream state. For non-stream data types, cloneObject defaults to getClone (deep copy). I would suggest we change the names here to clarify code and APIs, the better to reflect the shallowness of the cloning. cloneDeep (old getClone; clones even value , share nothing) cloneHalfDeep (new CopyForRead, clones even stream state, but not value, which is still shared) cloneShallow (old cloneObject, clones just "holder", shares stream/stream state) If the code really needs cloneShallow also, after cloneHalfDeep is added, I don't know, if not, I'd call cloneHalfDeep cloneShallow instead Now, for modification, what to use? I guess that depends on what semantics are desired/at what level in the code you are..? Maybe we could just do COW semantics? I.e. use cloneHalfDeep until update is attempted and only then do a full deep clone? (by overloading stream class perhaps) Then the updating of the deep copy could proceed until the column is actually updated without affecting the other aliases?
          Hide
          Kristian Waagan added a comment -

          Working on other issues involving reuse of streaming DVDs, it has occurred to me that I can reuse the stream cloning code found in 'derby-3650-preliminary_2_diff.txt'.
          I plan to split the original patch into two parts, but first I'd like some feedback on a few questions. General information about how things are working / supposed to work is also appreciated

          I ended up writing down my questions and observations as HTML, as I needed a table. See the attached file "cloning-methods.html".
          Feel free to upload a new version of the HTML file if you find errors in it or want to add more information, or just add a comment in Jira.

          Thanks,

          Show
          Kristian Waagan added a comment - Working on other issues involving reuse of streaming DVDs, it has occurred to me that I can reuse the stream cloning code found in 'derby-3650-preliminary_2_diff.txt'. I plan to split the original patch into two parts, but first I'd like some feedback on a few questions. General information about how things are working / supposed to work is also appreciated I ended up writing down my questions and observations as HTML, as I needed a table. See the attached file "cloning-methods.html". Feel free to upload a new version of the HTML file if you find errors in it or want to add more information, or just add a comment in Jira. Thanks,
          Hide
          Knut Anders Hatlen added a comment -

          Triaged for 10.5.2.

          • Checked "Repro attached"
          • Checked "Known fix" since a (not commit-ready) patch proposal is attached
          • Added components SQL and Store since they are touched by the suggested fix
          • Marked as unassigned since there has been no activity in over a year
          • Left the Regression flag on since the network client problems were not seen in earlier versions. The underlying embedded issue can be seen all the way back to 10.0.2.1
          Show
          Knut Anders Hatlen added a comment - Triaged for 10.5.2. Checked "Repro attached" Checked "Known fix" since a (not commit-ready) patch proposal is attached Added components SQL and Store since they are touched by the suggested fix Marked as unassigned since there has been no activity in over a year Left the Regression flag on since the network client problems were not seen in earlier versions. The underlying embedded issue can be seen all the way back to 10.0.2.1
          Hide
          Rick Hillegas added a comment -

          Hey Mike,

          Are you planning to commit a fix (or partial fix) for 10.5?

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Hey Mike, Are you planning to commit a fix (or partial fix) for 10.5? Thanks, -Rick
          Hide
          Golgoth 14 added a comment -

          Hi all,

          Do you have some news about this bug ?

          Thank you.

          Show
          Golgoth 14 added a comment - Hi all, Do you have some news about this bug ? Thank you.
          Hide
          Mike Matrigali added a comment -

          still not ready for commit, incorporates comments, improved testing, and fixes left outer join issue also. Also
          fixes null pointer bug found by aggregate tests. Will be running set of tests on this tonight.

          Show
          Mike Matrigali added a comment - still not ready for commit, incorporates comments, improved testing, and fixes left outer join issue also. Also fixes null pointer bug found by aggregate tests. Will be running set of tests on this tonight.
          Hide
          Mike Matrigali added a comment -

          > I experimented a little with the patch, and was able to solve the
          > problem we have when selecting the same column twice, i.e.
          > "select c as c1, c as c2 from testMultipleClob"
          > I did this by always creating a clone.
          > I didn't quite understand what would happen to the original stream, but
          > using 'finalize()' I was able to observe that it was garbage collected.
          > No call to 'closeStream' was made though.
          At least at store level all the streams get cleaned up at commit or abort
          if at no other time. They are all based off of open BaseContainerHandles
          and all those that are open at commit or abort get cleaned up.
          >
          > Is it an acceptable performance hit to clone streams unconditionally for
          > LOBs, or should we try to come up with some kind of bookkeeping?
          > (i.e. reference counting)
          > I'm thinking in the context of the JDBC layer, but the same question
          > might apply to lower layers.
          I am already uncomfortable with the unnecessary level of cloning/object
          creation this change creates for lobs that are part of a join. So I think
          we should look to minimize it. So I don't think it appropriate to throw in
          a clone every time we see a stream.

          I would like to figure out a way in a 1-1 join to not have to clone anything
          and just let the original stream proprogate up. Maybe some sort of reference
          counting is the solution.
          >
          >
          > Also, I see that a new container handle is created in 'initStream'. Am I
          > correct saying that you have to do 'aClone.initStream()' after calling
          > 'theStream.cloneStream()' and before closing 'theStream'?

          I think understanding this is key to fixing this problem correctly. After
          reading the code today I think what you describe is the expected behavior,
          but is not very well documented. I think the rule currently is something
          like:
          before using any Resetable interface on a stream one must call initStream().

          I think there might be cleaner solutions to the various clob/blob issues if
          this rule could be expanded to something like no jdbc interaction with a
          blob/clob in stream form can happen unless initStream() has been called.
          >
          >
          > The test 'tests.lang.LangScript#aggregate' failed with a NPE. The reason
          > was because a null DVD was encountered and '<null>.copyForRead()' was
          > called. Checking for null fixed the problem and the test succeeded.
          > Can we get a null inside the for loop in 'if (! notExistsRightSide)" as
          > well?
          >
          thanks for tracking down the bug before I could run tests. I have fixed
          the null pointer problems in the next patch.

          Show
          Mike Matrigali added a comment - > I experimented a little with the patch, and was able to solve the > problem we have when selecting the same column twice, i.e. > "select c as c1, c as c2 from testMultipleClob" > I did this by always creating a clone. > I didn't quite understand what would happen to the original stream, but > using 'finalize()' I was able to observe that it was garbage collected. > No call to 'closeStream' was made though. At least at store level all the streams get cleaned up at commit or abort if at no other time. They are all based off of open BaseContainerHandles and all those that are open at commit or abort get cleaned up. > > Is it an acceptable performance hit to clone streams unconditionally for > LOBs, or should we try to come up with some kind of bookkeeping? > (i.e. reference counting) > I'm thinking in the context of the JDBC layer, but the same question > might apply to lower layers. I am already uncomfortable with the unnecessary level of cloning/object creation this change creates for lobs that are part of a join. So I think we should look to minimize it. So I don't think it appropriate to throw in a clone every time we see a stream. I would like to figure out a way in a 1-1 join to not have to clone anything and just let the original stream proprogate up. Maybe some sort of reference counting is the solution. > > > Also, I see that a new container handle is created in 'initStream'. Am I > correct saying that you have to do 'aClone.initStream()' after calling > 'theStream.cloneStream()' and before closing 'theStream'? I think understanding this is key to fixing this problem correctly. After reading the code today I think what you describe is the expected behavior, but is not very well documented. I think the rule currently is something like: before using any Resetable interface on a stream one must call initStream(). I think there might be cleaner solutions to the various clob/blob issues if this rule could be expanded to something like no jdbc interaction with a blob/clob in stream form can happen unless initStream() has been called. > > > The test 'tests.lang.LangScript#aggregate' failed with a NPE. The reason > was because a null DVD was encountered and '<null>.copyForRead()' was > called. Checking for null fixed the problem and the test succeeded. > Can we get a null inside the for loop in 'if (! notExistsRightSide)" as > well? > thanks for tracking down the bug before I could run tests. I have fixed the null pointer problems in the next patch.
          Hide
          Kristian Waagan added a comment -

          Good work on the patch Mike, we need to get this bug fixed...
          I applied the patch and ran the updated test.
          I also ran the regression tests and got one failure (see below).
          Looks to me like the general approach is working and is a feasible solution
          to me.

          I have the following comments on the preliminary patch, where the
          whitespace and formatting only changes are mentioned because they cause
          the diff to be larger than necessary:

          [NestedLoopJoinResultSet]
          1) Whitespace only change.
          2) Formatting only change (first 'mergedRow ='). Intentional?
          [MemByteHolder]
          3) Mix of tabs and spaces in new code.
          [OverflowInputStream]
          4) SanityManager is imported, but never used.
          5) Formatting only changes.
          6) Strange indentation causing unnecessary diff (tab-space-tab):
          'columnOverflowPage.restorePortion...'
          7) Formatting only change.
          [ByteHolder]
          8) Mix of tabs and spaces for indentation.
          [StoredPage]
          9) Separate commit for the typo?
          File is untouched by the functional changes.
          [CloneableStream]
          10) Wrong class name in header.
          11) Some tabs in the file.
          [SQLChar]
          12) One tab: 'if (stream != null)'
          [SQLBinary]
          13) One tab: 'if (stream != null)'
          [Derby3650Test]
          14) I would prefer to remove the 'runDerby3749tests' variable after
          that bug has been fixed.
          15) Would it make sense to fill the LOBs with a pattern, instead of
          using a fixed value?
          One could use LoopingAlphabet[Stream|Reader] for this, also in the
          verification procedures (create a new stream to compare with).
          I'm thinking about detecting offset errors.
          16) Incorrect name for the setup method, which configured auto commit:
          'setup' -> 'setUp'

          I experimented a little with the patch, and was able to solve the
          problem we have when selecting the same column twice, i.e.
          "select c as c1, c as c2 from testMultipleClob"
          I did this by always creating a clone.
          I didn't quite understand what would happen to the original stream, but
          using 'finalize()' I was able to observe that it was garbage collected.
          No call to 'closeStream' was made though.

          Is it an acceptable performance hit to clone streams unconditionally for
          LOBs, or should we try to come up with some kind of bookkeeping?
          (i.e. reference counting)
          I'm thinking in the context of the JDBC layer, but the same question
          might apply to lower layers.

          Also, I see that a new container handle is created in 'initStream'. Am I
          correct saying that you have to do 'aClone.initStream()' after calling
          'theStream.cloneStream()' and before closing 'theStream'?

          The test 'tests.lang.LangScript#aggregate' failed with a NPE. The reason
          was because a null DVD was encountered and '<null>.copyForRead()' was
          called. Checking for null fixed the problem and the test succeeded.
          Can we get a null inside the for loop in 'if (! notExistsRightSide)" as
          well?

          I also found a small issue with LOBs related to mappings and reference
          book keeping while reviewing the patch, and I'll create a Jira for that
          tomorrow.

          I have not yet had time to look at your questions/comments from your previous post.

          Show
          Kristian Waagan added a comment - Good work on the patch Mike, we need to get this bug fixed... I applied the patch and ran the updated test. I also ran the regression tests and got one failure (see below). Looks to me like the general approach is working and is a feasible solution to me. I have the following comments on the preliminary patch, where the whitespace and formatting only changes are mentioned because they cause the diff to be larger than necessary: [NestedLoopJoinResultSet] 1) Whitespace only change. 2) Formatting only change (first 'mergedRow ='). Intentional? [MemByteHolder] 3) Mix of tabs and spaces in new code. [OverflowInputStream] 4) SanityManager is imported, but never used. 5) Formatting only changes. 6) Strange indentation causing unnecessary diff (tab-space-tab): 'columnOverflowPage.restorePortion...' 7) Formatting only change. [ByteHolder] 8) Mix of tabs and spaces for indentation. [StoredPage] 9) Separate commit for the typo? File is untouched by the functional changes. [CloneableStream] 10) Wrong class name in header. 11) Some tabs in the file. [SQLChar] 12) One tab: 'if (stream != null)' [SQLBinary] 13) One tab: 'if (stream != null)' [Derby3650Test] 14) I would prefer to remove the 'runDerby3749tests' variable after that bug has been fixed. 15) Would it make sense to fill the LOBs with a pattern, instead of using a fixed value? One could use LoopingAlphabet [Stream|Reader] for this, also in the verification procedures (create a new stream to compare with). I'm thinking about detecting offset errors. 16) Incorrect name for the setup method, which configured auto commit: 'setup' -> 'setUp' I experimented a little with the patch, and was able to solve the problem we have when selecting the same column twice, i.e. "select c as c1, c as c2 from testMultipleClob" I did this by always creating a clone. I didn't quite understand what would happen to the original stream, but using 'finalize()' I was able to observe that it was garbage collected. No call to 'closeStream' was made though. Is it an acceptable performance hit to clone streams unconditionally for LOBs, or should we try to come up with some kind of bookkeeping? (i.e. reference counting) I'm thinking in the context of the JDBC layer, but the same question might apply to lower layers. Also, I see that a new container handle is created in 'initStream'. Am I correct saying that you have to do 'aClone.initStream()' after calling 'theStream.cloneStream()' and before closing 'theStream'? The test 'tests.lang.LangScript#aggregate' failed with a NPE. The reason was because a null DVD was encountered and '<null>.copyForRead()' was called. Checking for null fixed the problem and the test succeeded. Can we get a null inside the for loop in 'if (! notExistsRightSide)" as well? I also found a small issue with LOBs related to mappings and reference book keeping while reviewing the patch, and I'll create a Jira for that tomorrow. I have not yet had time to look at your questions/comments from your previous post.
          Hide
          Mike Matrigali added a comment -

          The derby-3650-preliminary_diff.txt diff shows the direction I am trying to fix this bug. It is not ready for commit yet.
          Still needs tests to run, some more comments, maybe some more tests. I need to verify that this change has not made DERBY-3749 worse. Also need to see if the NestedLoopJoinResultSet.java changes need to be applied to any other nodes, hash joins seem to be taken care of. Looks like NestedLoopLeftOuterJoinResultSet.java
          could use the change, would like a test case that gets into that code first.

          Show
          Mike Matrigali added a comment - The derby-3650-preliminary_diff.txt diff shows the direction I am trying to fix this bug. It is not ready for commit yet. Still needs tests to run, some more comments, maybe some more tests. I need to verify that this change has not made DERBY-3749 worse. Also need to see if the NestedLoopJoinResultSet.java changes need to be applied to any other nodes, hash joins seem to be taken care of. Looks like NestedLoopLeftOuterJoinResultSet.java could use the change, would like a test case that gets into that code first.
          Hide
          Mike Matrigali added a comment -

          i am working on this following the approach I described earlier. I got distracted by some other issues, but should be able to focus on this again. This area of the code is not my expertise, so I will definitely post a patch for comment when I am closer. Any comments/reviews at that time are appreciated. Should have something within a week.

          Show
          Mike Matrigali added a comment - i am working on this following the approach I described earlier. I got distracted by some other issues, but should be able to focus on this again. This area of the code is not my expertise, so I will definitely post a patch for comment when I am closer. Any comments/reviews at that time are appreciated. Should have something within a week.
          Hide
          Kristian Waagan added a comment -

          Any progress on this issue?
          Anything the community can do to help drive it forwards?

          Show
          Kristian Waagan added a comment - Any progress on this issue? Anything the community can do to help drive it forwards?
          Hide
          Mike Matrigali added a comment -

          During a join we may stream N rows out from 1 left row and N right rows. When w
          e do this we use multiple references
          to the same underlining column, so N rows may point to 1 DataValueDescriptor (DV
          D). I believe this mostly works but is a problem if the DVD is represented by a
          stream rather than some sort of fixed
          datatype like an SQLInteger.

          In the case of DERBY-3650 a free is being called after one of the rows
          is finished processing, causing problems when the subsequent row needs
          access to the stream.

          The approach I am going to look at is to force the join case, where streams
          are involved to generate a real copy of the stream rather than 2 references
          to the same stream. I plan on leaving all other copies as reference copies
          so that performance and memory usage of the whole system is not affected
          too much.

          Show
          Mike Matrigali added a comment - During a join we may stream N rows out from 1 left row and N right rows. When w e do this we use multiple references to the same underlining column, so N rows may point to 1 DataValueDescriptor (DV D). I believe this mostly works but is a problem if the DVD is represented by a stream rather than some sort of fixed datatype like an SQLInteger. In the case of DERBY-3650 a free is being called after one of the rows is finished processing, causing problems when the subsequent row needs access to the stream. The approach I am going to look at is to force the join case, where streams are involved to generate a real copy of the stream rather than 2 references to the same stream. I plan on leaving all other copies as reference copies so that performance and memory usage of the whole system is not affected too much.
          Hide
          Mike Matrigali added a comment -

          Changing the problem description to reflect the research so far on the issue.

          Show
          Mike Matrigali added a comment - Changing the problem description to reflect the research so far on the issue.
          Hide
          Kristian Waagan added a comment -

          I see that the same DataValueDescriptor (dvd) object is used for both rows when running the full repro for embedded. The object probably comes from the base result set (the single clob value).
          Now, I see in OverflowInputStream.close that it also closes the associated container handler. This is again called by Clob.free.

          A simple way to make the repro pass is to make sure the stream is materialized when the object is accessed the first time. However, I think this solution is too naive and it will use too much memory (the user might never actually request the value).

          Show
          Kristian Waagan added a comment - I see that the same DataValueDescriptor (dvd) object is used for both rows when running the full repro for embedded. The object probably comes from the base result set (the single clob value). Now, I see in OverflowInputStream.close that it also closes the associated container handler. This is again called by Clob.free. A simple way to make the repro pass is to make sure the stream is materialized when the object is accessed the first time. However, I think this solution is too naive and it will use too much memory (the user might never actually request the value).
          Hide
          Kristian Waagan added a comment -

          This issue is still on my radar, but I find it had to start working on a fix...
          Does anyone have more information that could be used to determine where the fix should be made?
          Does the store support having two streams serving the same data in a single row? (i.e. select myblob as stream1, myblob as stream2)

          My next step would be to work through one of the repros attached and see if there are any obvious poins of incorrect behavior.
          Since I have some other issues in the pipeline, I do not know when I can get around to do this.

          Show
          Kristian Waagan added a comment - This issue is still on my radar, but I find it had to start working on a fix... Does anyone have more information that could be used to determine where the fix should be made? Does the store support having two streams serving the same data in a single row? (i.e. select myblob as stream1, myblob as stream2) My next step would be to work through one of the repros attached and see if there are any obvious poins of incorrect behavior. Since I have some other issues in the pipeline, I do not know when I can get around to do this.
          Hide
          Kathey Marsden added a comment -

          Committed tests with revision 657124.

          Show
          Kathey Marsden added a comment - Committed tests with revision 657124.
          Hide
          Kathey Marsden added a comment -

          Attached is a patch to add tests for this issue. The tests currently fail but I want to check them in (disabled) so they are available for reference.

          One thing I noticed in writing the test is that for embedded, while the one to many join works ok when using getBlob() or getClob(), it does not work if we just use ResultSet.getBinaryStream() or ResultSet.getCharacterStream(),
          reenforcing Mike's statement that the fact that it works is probably by accident and not by design.

          Show
          Kathey Marsden added a comment - Attached is a patch to add tests for this issue. The tests currently fail but I want to check them in (disabled) so they are available for reference. One thing I noticed in writing the test is that for embedded, while the one to many join works ok when using getBlob() or getClob(), it does not work if we just use ResultSet.getBinaryStream() or ResultSet.getCharacterStream(), reenforcing Mike's statement that the fact that it works is probably by accident and not by design.
          Hide
          Golgoth 14 added a comment -

          Hi all,

          I do not understand all the concepts behind the notion of stream.
          I understand that the problem is sharing a stream reading and should not be that the flow is closed if other clients read.

          This could help you ?
          http://java.sun.com/javaee/5/docs/api/javax/mail/internet/SharedInputStream.html
          or
          http://commons.apache.org/vfs/apidocs/index.html?org/apache/commons/vfs/util/SharedRandomContentInputStream.html

          Show
          Golgoth 14 added a comment - Hi all, I do not understand all the concepts behind the notion of stream. I understand that the problem is sharing a stream reading and should not be that the flow is closed if other clients read. This could help you ? http://java.sun.com/javaee/5/docs/api/javax/mail/internet/SharedInputStream.html or http://commons.apache.org/vfs/apidocs/index.html?org/apache/commons/vfs/util/SharedRandomContentInputStream.html
          Hide
          Mike Matrigali added a comment -

          I am not sure of the solution, but thought I would share my understanding
          of the stream and clob datatype support from store up through language
          on the embedded side of the system - hoping to spark a discussion.
          The more of these bugs that come up
          makes me think that a number of things are working by accident and that
          we just keep patching up the latest thing that crops up which then pushes
          something else up later. The main area of concern are cases where we
          copy and or share references to a single column that may contain a stream.

          For this discussion I am going to concentrate on an embedded client not
          using locators.

          The base use case is that language requests a column from store, which
          reads the data into a datatype. In this case the datatype of interest
          is SQLClob, but the discussion is similar internally to all the types
          that could support a stream representation (SQLChar, SQLVarchar, SQLClob,
          SQLBlob, ...). All these datatypes, internal to the datatype, can either
          hold the data completely in memory at the point of exit from the store
          routines or in the form of a stream which can be held unread until finally
          getting to user.

          The initial decision of whether the state should be a stream or in memory
          is first decided by the store layer. Basically if the entire column data
          resides on one page then the data is read into an array. If the column
          spans multiple pages then a stream is returned which knows how to traverse
          the pages of the store to return the stream. Subsequently the datatype
          itself can decide to materialize the stream into memory, but code avoids this
          as it may mean instantiating a 2GB blob in memory.

          The functionality of the stream is somewhat limited. It is assumed single
          user and can only be read from front to back. It does support a reset
          functionality which can rewind the stream back to the beginning. The
          stream keeps a buffer of the current page worth of data that has been
          read.

          A number of issues stem from the system copying
          references to streams rather than somehow making independent copies of
          the streams. For instance:
          DERBY-3650 - during a 1 to many join, multiple rows end up with references to
          a single stream.
          DERBY-3646 - select of same clob twice in row leads to 2 references to same
          column.

          o If we want to continue the current usage then how are we going to manage
          multiple references to a single stream across rows? The new free code
          adds problems here. We have defined a lot of the other problems away
          with documenting we don't support stream references across rows.
          o If we want to continue the current usage of sharing reference across columns
          what kind of concurrent stream access should we support (see the list of
          options in 3646)

          I don't know how hard it is but should we just stop language from sharing
          references to streams? The existing usage works very well for all the other
          datatypes including the stream types when they are not streams.

          Should we somehow make the datatype/stream smarter to somehow handle the
          multiple reference?

          Should we recognize the multiple reference and just give up and materialize
          the clob?

          Show
          Mike Matrigali added a comment - I am not sure of the solution, but thought I would share my understanding of the stream and clob datatype support from store up through language on the embedded side of the system - hoping to spark a discussion. The more of these bugs that come up makes me think that a number of things are working by accident and that we just keep patching up the latest thing that crops up which then pushes something else up later. The main area of concern are cases where we copy and or share references to a single column that may contain a stream. For this discussion I am going to concentrate on an embedded client not using locators. The base use case is that language requests a column from store, which reads the data into a datatype. In this case the datatype of interest is SQLClob, but the discussion is similar internally to all the types that could support a stream representation (SQLChar, SQLVarchar, SQLClob, SQLBlob, ...). All these datatypes, internal to the datatype, can either hold the data completely in memory at the point of exit from the store routines or in the form of a stream which can be held unread until finally getting to user. The initial decision of whether the state should be a stream or in memory is first decided by the store layer. Basically if the entire column data resides on one page then the data is read into an array. If the column spans multiple pages then a stream is returned which knows how to traverse the pages of the store to return the stream. Subsequently the datatype itself can decide to materialize the stream into memory, but code avoids this as it may mean instantiating a 2GB blob in memory. The functionality of the stream is somewhat limited. It is assumed single user and can only be read from front to back. It does support a reset functionality which can rewind the stream back to the beginning. The stream keeps a buffer of the current page worth of data that has been read. A number of issues stem from the system copying references to streams rather than somehow making independent copies of the streams. For instance: DERBY-3650 - during a 1 to many join, multiple rows end up with references to a single stream. DERBY-3646 - select of same clob twice in row leads to 2 references to same column. o If we want to continue the current usage then how are we going to manage multiple references to a single stream across rows? The new free code adds problems here. We have defined a lot of the other problems away with documenting we don't support stream references across rows. o If we want to continue the current usage of sharing reference across columns what kind of concurrent stream access should we support (see the list of options in 3646) I don't know how hard it is but should we just stop language from sharing references to streams? The existing usage works very well for all the other datatypes including the stream types when they are not streams. Should we somehow make the datatype/stream smarter to somehow handle the multiple reference? Should we recognize the multiple reference and just give up and materialize the clob?
          Hide
          Kathey Marsden added a comment -

          The client regression is related to the DERBY-2892 fix, although the underlying embedded problem has been there since free was introduced.

          Show
          Kathey Marsden added a comment - The client regression is related to the DERBY-2892 fix, although the underlying embedded problem has been there since free was introduced.
          Hide
          Kathey Marsden added a comment -

          I spoke briefly on IRC with Kristian regarding this bug and he suggested perhaps the right solution is for each lob to have its own "store stream." I don't know even if this would be a store change or a language change. Anyway, input from either area is welcome. Even though the underlying embedded problem has been around, I think the client regression is a likely hit by users upgrading to 10.3.30 or 10.4, so it would be good to get it addressed as soon as possible, especially since we are encouraging aggressive upgrade.

          Show
          Kathey Marsden added a comment - I spoke briefly on IRC with Kristian regarding this bug and he suggested perhaps the right solution is for each lob to have its own "store stream." I don't know even if this would be a store change or a language change. Anyway, input from either area is welcome. Even though the underlying embedded problem has been around, I think the client regression is a likely hit by users upgrading to 10.3.30 or 10.4, so it would be good to get it addressed as soon as possible, especially since we are encouraging aggressive upgrade.
          Hide
          Kathey Marsden added a comment -

          Attaching one more case, Derby3650FullClientRepro which shows the regression from 10.2 to 10.3 caused by the fix for DERBY-2892. For client even without the free we fail,, because there is a free under the covers. This case passes with 10.2 or with soft upgrade to higher versions but fails with a new database for 10.3.3.0
          java DerbyFullClientRepro
          — rs.next iteration 1
          Clob id : 1
          — rs.next iteration 2
          Clob id : 1
          Exception in thread "main" java.sql.SQLException: The exception 'java.sql.SQLException: Java exception: 'Reached end-of-
          stream prematurely, with 2 byte(s) to go: java.io.EOFException'.' was thrown while evaluating an expression.
          at org.apache.derby.client.am.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:96)
          at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:362)
          at org.apache.derby.client.am.Clob.getCharacterStream(Clob.java:349)
          at Derby3650FullClientRepro.main(Derby3650FullClientRepro.java:25)
          Caused by: org.apache.derby.client.am.SqlException: The exception 'java.sql.SQLException: Java exception: 'Reached end-o
          f-stream prematurely, with 2 byte(s) to go: java.io.EOFException'.' was thrown while evaluating an expression.
          at org.apache.derby.client.am.Statement.completeExecute(Statement.java:1601)
          at org.apache.derby.client.net.NetStatementReply.parseEXCSQLSTTreply(NetStatementReply.java:322)
          at org.apache.derby.client.net.NetStatementReply.readExecuteCall(NetStatementReply.java:106)
          at org.apache.derby.client.net.StatementReply.readExecuteCall(StatementReply.java:75)
          at org.apache.derby.client.net.NetStatement.readExecuteCall_(NetStatement.java:179)
          at org.apache.derby.client.am.Statement.readExecuteCall(Statement.java:1567)
          at org.apache.derby.client.am.PreparedStatement.flowExecute(PreparedStatement.java:2139)
          at org.apache.derby.client.am.PreparedStatement.executeX(PreparedStatement.java:1582)
          at org.apache.derby.client.am.CallableLocatorProcedures.clobGetLength(CallableLocatorProcedures.java:859)
          at org.apache.derby.client.am.Clob.getLocatorLength(Clob.java:1050)
          at org.apache.derby.client.am.Lob.sqlLength(Lob.java:116)
          at org.apache.derby.client.am.Lob.checkForLocatorValidity(Lob.java:386)
          at org.apache.derby.client.am.UpdateSensitiveClobLocatorReader.<init>(UpdateSensitiveClobLocatorReader.java:74)
          at org.apache.derby.client.am.Clob.getCharacterStreamX(Clob.java:362)
          at org.apache.derby.client.am.Clob.getCharacterStream(Clob.java:340)
          ... 1 more
          Caused by: org.apache.derby.client.am.SqlException: Java exception: 'Reached end-of-stream prematurely, with 2 byte(s) t
          o go: java.io.EOFException'.
          ... 16 more

          Show
          Kathey Marsden added a comment - Attaching one more case, Derby3650FullClientRepro which shows the regression from 10.2 to 10.3 caused by the fix for DERBY-2892 . For client even without the free we fail,, because there is a free under the covers. This case passes with 10.2 or with soft upgrade to higher versions but fails with a new database for 10.3.3.0 java DerbyFullClientRepro — rs.next iteration 1 Clob id : 1 — rs.next iteration 2 Clob id : 1 Exception in thread "main" java.sql.SQLException: The exception 'java.sql.SQLException: Java exception: 'Reached end-of- stream prematurely, with 2 byte(s) to go: java.io.EOFException'.' was thrown while evaluating an expression. at org.apache.derby.client.am.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:96) at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:362) at org.apache.derby.client.am.Clob.getCharacterStream(Clob.java:349) at Derby3650FullClientRepro.main(Derby3650FullClientRepro.java:25) Caused by: org.apache.derby.client.am.SqlException: The exception 'java.sql.SQLException: Java exception: 'Reached end-o f-stream prematurely, with 2 byte(s) to go: java.io.EOFException'.' was thrown while evaluating an expression. at org.apache.derby.client.am.Statement.completeExecute(Statement.java:1601) at org.apache.derby.client.net.NetStatementReply.parseEXCSQLSTTreply(NetStatementReply.java:322) at org.apache.derby.client.net.NetStatementReply.readExecuteCall(NetStatementReply.java:106) at org.apache.derby.client.net.StatementReply.readExecuteCall(StatementReply.java:75) at org.apache.derby.client.net.NetStatement.readExecuteCall_(NetStatement.java:179) at org.apache.derby.client.am.Statement.readExecuteCall(Statement.java:1567) at org.apache.derby.client.am.PreparedStatement.flowExecute(PreparedStatement.java:2139) at org.apache.derby.client.am.PreparedStatement.executeX(PreparedStatement.java:1582) at org.apache.derby.client.am.CallableLocatorProcedures.clobGetLength(CallableLocatorProcedures.java:859) at org.apache.derby.client.am.Clob.getLocatorLength(Clob.java:1050) at org.apache.derby.client.am.Lob.sqlLength(Lob.java:116) at org.apache.derby.client.am.Lob.checkForLocatorValidity(Lob.java:386) at org.apache.derby.client.am.UpdateSensitiveClobLocatorReader.<init>(UpdateSensitiveClobLocatorReader.java:74) at org.apache.derby.client.am.Clob.getCharacterStreamX(Clob.java:362) at org.apache.derby.client.am.Clob.getCharacterStream(Clob.java:340) ... 1 more Caused by: org.apache.derby.client.am.SqlException: Java exception: 'Reached end-of-stream prematurely, with 2 byte(s) t o go: java.io.EOFException'. ... 16 more
          Hide
          Kathey Marsden added a comment -

          I am really quite stuck on this issue. Somehow we will need to defer the freeing of the lob until the result set is closed when it is used in a join if free or commit are called. I tried simply nulling out the InternalClob in EmbedClob and adding a finalize method to StoreStreamClob to close the stream, thinking that in this way we could perhaps rely on garbage collection to close the stream after it is no longer referenced, but even with a forced gc() after closing the result set, I saw failures in ClobTest.testLockingAfterFree, which indicates that there is still a reference to the stream until the end of the transaction.

          Perhaps this method would still work if I could figure out where the reference to the stream is and I could null that out too on free(). Then we could force a gc() on result set close. Perhaps I am off track all together. Does anyone know where else the stream is referenced or have any other ideas on this issue?

          Thanks

          Kathey

          Show
          Kathey Marsden added a comment - I am really quite stuck on this issue. Somehow we will need to defer the freeing of the lob until the result set is closed when it is used in a join if free or commit are called. I tried simply nulling out the InternalClob in EmbedClob and adding a finalize method to StoreStreamClob to close the stream, thinking that in this way we could perhaps rely on garbage collection to close the stream after it is no longer referenced, but even with a forced gc() after closing the result set, I saw failures in ClobTest.testLockingAfterFree, which indicates that there is still a reference to the stream until the end of the transaction. Perhaps this method would still work if I could figure out where the reference to the stream is and I could null that out too on free(). Then we could force a gc() on result set close. Perhaps I am off track all together. Does anyone know where else the stream is referenced or have any other ideas on this issue? Thanks Kathey
          Hide
          Kathey Marsden added a comment -

          I think I understand what is going on here. In the join case we are doing a nested loop join, joining the left result set, (1 row with clob) with the right result set (2 rows), so essentially we do
          for each row in left result set
          for each row in right result set
          form tuple.

          So we need to access the 1 row in the left result set twice, so freeing the clob after the first row is returned is no good. Somehow, freeing the clob needs to be deferred in this case.

          Note the same symptom occurs if we commit instead of calling clob.free() because commit also frees the clobs.

          Show
          Kathey Marsden added a comment - I think I understand what is going on here. In the join case we are doing a nested loop join, joining the left result set, (1 row with clob) with the right result set (2 rows), so essentially we do for each row in left result set for each row in right result set form tuple. So we need to access the 1 row in the left result set twice, so freeing the clob after the first row is returned is no good. Somehow, freeing the clob needs to be deferred in this case. Note the same symptom occurs if we commit instead of calling clob.free() because commit also frees the clobs.
          Hide
          Mamta A. Satoor added a comment -

          I spent some time on this jira entry. In the test cases attached by Kathey, Derby3650FullRepro.java and UnionAll.java, there are 2 tables TAB and TAB2. There is one row in table TAB which has the CLOB column and 2 rows in table TAB2. It appears that for loj case, we are looking at TAB only once to get the CLOB and using that same row from TAB twice for the 2 rows returned from TAB2 but in case of union, it looks like we get the row from TAB twice once for each row in TAB2. I am not sure why there is a difference in how and when the row is fetched from TAB but may be explore that difference further more. May be in loj case, we should not free the CLOB so it can be reused. Not exactly sure but I thought I would share what I found.

          Show
          Mamta A. Satoor added a comment - I spent some time on this jira entry. In the test cases attached by Kathey, Derby3650FullRepro.java and UnionAll.java, there are 2 tables TAB and TAB2. There is one row in table TAB which has the CLOB column and 2 rows in table TAB2. It appears that for loj case, we are looking at TAB only once to get the CLOB and using that same row from TAB twice for the 2 rows returned from TAB2 but in case of union, it looks like we get the row from TAB twice once for each row in TAB2. I am not sure why there is a difference in how and when the row is fetched from TAB but may be explore that difference further more. May be in loj case, we should not free the CLOB so it can be reused. Not exactly sure but I thought I would share what I found.
          Hide
          Kathey Marsden added a comment -

          For lack of a better idea where to look, I started looking at the FormatIdStream allocations for the Clob. I saw that there was only one allocation for Derby3650FullRepro.java which happened on openCore() and two for UnionAll.java which happened on each of the next() calls. Not sure if this is relevant but am attaching the traces in case someone is interested.

          Show
          Kathey Marsden added a comment - For lack of a better idea where to look, I started looking at the FormatIdStream allocations for the Clob. I saw that there was only one allocation for Derby3650FullRepro.java which happened on openCore() and two for UnionAll.java which happened on each of the next() calls. Not sure if this is relevant but am attaching the traces in case someone is interested.
          Hide
          Kristian Waagan added a comment -

          I can't tell you where the decision is made, but I observe that in the two repros (UnionAll and Derby3650FullRepro) the behavior is different.
          In the former one, the Clobs are created with different instances of FormatIdInputStream, whereas in the latter the same instance of FormatIdInputStream is passed to both Clobs.

          Maybe the next step would be to figure out why this difference occur.
          I thought I saw a JIRA on this alredady, but note that you also get the same error by simply selecting the same LOB column twice.

          Show
          Kristian Waagan added a comment - I can't tell you where the decision is made, but I observe that in the two repros (UnionAll and Derby3650FullRepro) the behavior is different. In the former one, the Clobs are created with different instances of FormatIdInputStream, whereas in the latter the same instance of FormatIdInputStream is passed to both Clobs. Maybe the next step would be to figure out why this difference occur. I thought I saw a JIRA on this alredady, but note that you also get the same error by simply selecting the same LOB column twice.
          Hide
          Kathey Marsden added a comment -

          Here is the UnionAll case that works fine. I want to understand why in this case we are able to return the same clob twice without problem but with the left outer join we have trouble. Does it materialize the clob in the union all case or is there something else going on? I am not quite sure where in the code to look but am looking ... Any advice is appreciated.

          Show
          Kathey Marsden added a comment - Here is the UnionAll case that works fine. I want to understand why in this case we are able to return the same clob twice without problem but with the left outer join we have trouble. Does it materialize the clob in the union all case or is there something else going on? I am not quite sure where in the code to look but am looking ... Any advice is appreciated.
          Hide
          Kathey Marsden added a comment -

          Attached is a full repro of the issue with embedded (explicit free) without the testdb. We just had to do a left outer join that returned the same clob twice.

          Show
          Kathey Marsden added a comment - Attached is a full repro of the issue with embedded (explicit free) without the testdb. We just had to do a left outer join that returned the same clob twice.
          Hide
          Kathey Marsden added a comment -

          I tried making a standalone repro by having a clob table with one row and returning it as two rows in the result set, with
          SELECT * FROM TAB UNION ALL SELECT * FROM TAB

          but that case seemed to work ok. There must be something different going on with the query in the repro.

          Show
          Kathey Marsden added a comment - I tried making a standalone repro by having a clob table with one row and returning it as two rows in the result set, with SELECT * FROM TAB UNION ALL SELECT * FROM TAB but that case seemed to work ok. There must be something different going on with the query in the repro.
          Hide
          Kathey Marsden added a comment -

          Attached is Derby3650EmbeddedRepro that shows the same problem with embedded if the clob is explicitly freed.

          Show
          Kathey Marsden added a comment - Attached is Derby3650EmbeddedRepro that shows the same problem with embedded if the clob is explicitly freed.
          Hide
          Kristian Waagan added a comment -

          My early suspicions pointed at the new release mechanism to avoid LOB locators piling up in the server, eventually leading to an OOME before the problem was fix.
          Following this trail led me to the cause of the bug.

          When a new Clob is created, a store stream that has already been closed is passed in to the EmbedClob constructor.
          The stream has been closed by EmbedClob.free (the Clob being represented as a StoreStreamClob). Derby fails at EmbedClob:153, more specifically in OverflowInputStream:155. Here it detects that there is no active transaction, which indicates a commit or rollback has taken place. However, I think that is a red herring.

          Further investigation made it clear that in the case of the data generation script in the original repro, you need childrenCout to be 19 (or higher) to provoke the error.
          Making the original join query less verbose, gives:
          SELECT
          Nm_Data
          FROM
          Tbl_T4 inner join Tbl_T2 on Tbl_T4.Id_T2=Tbl_T2.Id
          left outer join Tbl_T1 on Tbl_T2.Id_T1=Tbl_T1.Id
          WHERE Tbl_T4.Id_T3=?

          If you allow the LOB release mechanism to release a Clob, the bug occurs because all rows in the result set points to the same Clob data.
          I made a smaller repro, you have to run the original one first to create the data or download the zipped db. Feel free to improve it, I was a bit short on time to extract the data generation and add proper comments...

          The question is, where and how should this problem be fixed?

          Show
          Kristian Waagan added a comment - My early suspicions pointed at the new release mechanism to avoid LOB locators piling up in the server, eventually leading to an OOME before the problem was fix. Following this trail led me to the cause of the bug. When a new Clob is created, a store stream that has already been closed is passed in to the EmbedClob constructor. The stream has been closed by EmbedClob.free (the Clob being represented as a StoreStreamClob). Derby fails at EmbedClob:153, more specifically in OverflowInputStream:155. Here it detects that there is no active transaction, which indicates a commit or rollback has taken place. However, I think that is a red herring. Further investigation made it clear that in the case of the data generation script in the original repro, you need childrenCout to be 19 (or higher) to provoke the error. Making the original join query less verbose, gives: SELECT Nm_Data FROM Tbl_T4 inner join Tbl_T2 on Tbl_T4.Id_T2=Tbl_T2.Id left outer join Tbl_T1 on Tbl_T2.Id_T1=Tbl_T1.Id WHERE Tbl_T4.Id_T3=? If you allow the LOB release mechanism to release a Clob, the bug occurs because all rows in the result set points to the same Clob data. I made a smaller repro, you have to run the original one first to create the data or download the zipped db. Feel free to improve it, I was a bit short on time to extract the data generation and add proper comments... The question is, where and how should this problem be fixed?
          Hide
          Golgoth 14 added a comment -

          On my previous comment, I've added the Maven command to run the test without Eclipse

          I will perhaps post the same test case on the Hibernate Forum...

          Thanks,
          Stephane

          Show
          Golgoth 14 added a comment - On my previous comment, I've added the Maven command to run the test without Eclipse I will perhaps post the same test case on the Hibernate Forum... Thanks, Stephane
          Hide
          Kristian Waagan added a comment -

          What makes you think I'm using Eclipse?

          I managed to get the repro running in NetBeans by generating a freeform project with Maven and then creating a "Run class/file" target.
          The repro demonstrates the problem, and I guess the first thing to do is to figure out if the repro or Hibernate is doing something wrong or if Derby has a bug.

          BTW; I'll be away for two weeks.

          Show
          Kristian Waagan added a comment - What makes you think I'm using Eclipse? I managed to get the repro running in NetBeans by generating a freeform project with Maven and then creating a "Run class/file" target. The repro demonstrates the problem, and I guess the first thing to do is to figure out if the repro or Hibernate is doing something wrong or if Derby has a bug. BTW; I'll be away for two weeks.
          Hide
          Golgoth 14 added a comment - - edited

          Hi,

          Please, import the project into Eclipse.
          You could generate the eclipse project with the maven command "mvn eclipse:eclipse".

          Next, run the JUnit test called TestBlob.

          OR

          run the command "mvn test -DtestClassesDirectory=.\target\classes"
          under the directory "...\mvn test -DtestClassesDirectory=.\target\classes"

          Show
          Golgoth 14 added a comment - - edited Hi, Please, import the project into Eclipse. You could generate the eclipse project with the maven command "mvn eclipse:eclipse". Next, run the JUnit test called TestBlob. OR run the command "mvn test -DtestClassesDirectory=.\target\classes" under the directory "...\mvn test -DtestClassesDirectory=.\target\classes"
          Hide
          Kristian Waagan added a comment -

          Thank you for providing the repro.

          And what is the magic command to run the repro?
          I'm able to build it, but haven't figured out how to run it in a simple way...

          Show
          Kristian Waagan added a comment - Thank you for providing the repro. And what is the magic command to run the repro? I'm able to build it, but haven't figured out how to run it in a simple way...
          Hide
          Golgoth 14 added a comment - - edited

          I've attached a test case.

          You should modify the value of the variable "childrenCout" into the method "insertData".
          If the value is about 10, the error not appear.
          If the value is about 100 or 1000, the exception describe below is thrown.

          Show
          Golgoth 14 added a comment - - edited I've attached a test case. You should modify the value of the variable "childrenCout" into the method "insertData". If the value is about 10, the error not appear. If the value is about 100 or 1000, the exception describe below is thrown.
          Hide
          Kristian Waagan added a comment -

          Any chance you can provide a repro for this?

          Basically, a LOB is released when the method 'free' is explicitly called on the LOB (requires Java SE 6.0), if a column is not accessed before moving off a row (i.e. you have a row with a Clob, but never access it with getClob) or if the transaction is committed / rolled back.

          To determine if the LOB is released incorrectly, it would help a lot to have a working repro. This would also help determine if the problem is in another area of the code.
          The first thing to verify, is that you are running with auto commit disabled.

          Show
          Kristian Waagan added a comment - Any chance you can provide a repro for this? Basically, a LOB is released when the method 'free' is explicitly called on the LOB (requires Java SE 6.0), if a column is not accessed before moving off a row (i.e. you have a row with a Clob, but never access it with getClob) or if the transaction is committed / rolled back. To determine if the LOB is released incorrectly, it would help a lot to have a working repro. This would also help determine if the problem is in another area of the code. The first thing to verify, is that you are running with auto commit disabled.

            People

            • Assignee:
              Kristian Waagan
              Reporter:
              Golgoth 14
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development