Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.3.1.4
    • Component/s: JDBC
    • Labels:
      None
    • Environment:
      all
    1. derby-2247-v4-usingStoreFactory.diff
      72 kB
      Kristian Waagan
    2. derby-2247-v3-usingStoreFactory.diff
      72 kB
      Anurag Shekhar
    3. derby-2247v2-using_StoreFactory.diff
      71 kB
      Anurag Shekhar
    4. derby-2247-followupv3.diff
      26 kB
      Anurag Shekhar
    5. derby-2247-followup2.diff
      15 kB
      Anurag Shekhar
    6. derby-2247-followup2.diff
      15 kB
      Anurag Shekhar
    7. derby-2247-followup.diff
      14 kB
      Anurag Shekhar
    8. derby-2247.diff
      66 kB
      Anurag Shekhar

      Issue Links

        Activity

        Hide
        Myrna van Lunteren added a comment -

        Is this issue complete?

        Show
        Myrna van Lunteren added a comment - Is this issue complete?
        Hide
        Knut Anders Hatlen added a comment -

        Thanks Anurag! Committed derby-2247-followupv3.diff with revision 542473.

        Show
        Knut Anders Hatlen added a comment - Thanks Anurag! Committed derby-2247-followupv3.diff with revision 542473.
        Hide
        Anurag Shekhar added a comment -

        >No, I didn't notice the difference in signatures. It should still
        >return StorageFile, so the method would look something like this:

        > public StorageFile createTemporaryFile(String prefix, String suffix)
        throws IOException
        >

        { > return newStorageFile(File.createTempFile( > prefix, suffix, new File(getTempDir().getPath())).getPath()); > }

        updated with a minor change passing directory in newStorageFile (without this its tries to make absulute path starting from db direcotry)

        >>5) LOBInputStream.read(byte[]) and read(byte[],int,int) contain this
        >> piece of code:
        >>+ int ret = control.read(b, off, len, pos);
        >>+ if (ret > 0)

        { >>+ pos += ret; >>+ return ret; >>+ }

        >>+ return -1;
        >>Since a call to InputStream.read(byte[]...) theoretically can return
        >>0, I think (ret > 0) should be changed to (ret >= 0) or perhaps (ret
        >>!= -1).
        >
        >this was taken care by one of the privious patch

        >The previous patch removed read(byte[]), but read(byte[],int,int) is
        >still there with the same code.

        fixed

        >>11) LOBStreamControl.init() might ignore some exceptions.
        >
        >Added all skipped exception in unexpected exception.

        >I'm not sure unexpectedUserException() is the correct method to use to
        >wrap the exception in this case. I thought it was supposed to be used
        >to wrap exceptions thrown by user code (like stored procedures and
        >VTIs). Perhaps Util.javaException() would be better?

        It was wrong. I am now throw IOException (assuming most of the time it will be a i/o error apart form RuntimeException and StandardException) with cause set to original exception.

        >>1) Can control be final?
        >
        >ClobStreamControl extends from LOBStreamControl. ClobStreamControl is
        >already final.

        >I think Kristian was referring to the variable control in
        >LOBOutputStream. It is not final.

        changed control in LOBInputStrea and LOBOutputStream to final

        >>10) I think it would be good to split the testSetBytes into one test
        >>method for in-memory operation, and one for on disk operation.
        >
        >fixed

        >It would also be good if these test cases called close() on
        >rs. Another thing I noticed is that the test cases contain clean-up
        >code in a finally block. If an exception is thrown in the finally
        >block, it will hide the original error, so I think it's better to
        >remove the finally block.

        added close and removed fianlly block

        >>15) Comment why the test is currently only run in embedded in suite().
        >
        >these tests, the way they are written, are specific to embedded
        >(testing with memory and then file system) and the client already has
        >similar tests in jdbcapi/LobStreamsTest.

        >Could you add this comment in the source file as well? Would it run in
        >client/server mode? If so, I think it would be valuable to run it
        >under the client too.

        updated comment in suite method

        Some other comments to LobStreamTest:

        • tearDown() doesn't call super.tearDown() (if it did, calling
          rollback() and close() on conn wouldn't be necessary)
        • the instance variables dbName, useLOBStreamControl and f are not
          used and should be removed
        • the instance variable conn is not needed since the connection is
          also stored in the super class
        • blob should be set to null in tearDown()

        called super.tearDown and set blob to null and removed rollback and close call.

        >Follow up comments from knuth
        >
        >>- long finalLen = (dataBytes != null) ? dataBytes.length + b.length
        >>- : b.length;
        >>- if (finalLen < MAX_BUF_SIZE)
        >>+ if (pos + b.length < MAX_BUF_SIZE)
        >> return updateData(b, off, len, pos);
        >> else

        { >> init(dataBytes, pos); >> }

        >
        >>Shouldn't b.length have been len, in case we don't write the entire byte array?
        >>And shouldn't the comparison use <= instead of <?
        >
        >changed

        I noticed that you didn't change the comparison operator. Do you think
        that comment was wrong?

        i had missed it changed it in the current patch.

        Show
        Anurag Shekhar added a comment - >No, I didn't notice the difference in signatures. It should still >return StorageFile, so the method would look something like this: > public StorageFile createTemporaryFile(String prefix, String suffix) throws IOException > { > return newStorageFile(File.createTempFile( > prefix, suffix, new File(getTempDir().getPath())).getPath()); > } updated with a minor change passing directory in newStorageFile (without this its tries to make absulute path starting from db direcotry) >>5) LOBInputStream.read(byte[]) and read(byte[],int,int) contain this >> piece of code: >>+ int ret = control.read(b, off, len, pos); >>+ if (ret > 0) { >>+ pos += ret; >>+ return ret; >>+ } >>+ return -1; >>Since a call to InputStream.read(byte[]...) theoretically can return >>0, I think (ret > 0) should be changed to (ret >= 0) or perhaps (ret >>!= -1). > >this was taken care by one of the privious patch >The previous patch removed read(byte[]), but read(byte[],int,int) is >still there with the same code. fixed >>11) LOBStreamControl.init() might ignore some exceptions. > >Added all skipped exception in unexpected exception. >I'm not sure unexpectedUserException() is the correct method to use to >wrap the exception in this case. I thought it was supposed to be used >to wrap exceptions thrown by user code (like stored procedures and >VTIs). Perhaps Util.javaException() would be better? It was wrong. I am now throw IOException (assuming most of the time it will be a i/o error apart form RuntimeException and StandardException) with cause set to original exception. >>1) Can control be final? > >ClobStreamControl extends from LOBStreamControl. ClobStreamControl is >already final. >I think Kristian was referring to the variable control in >LOBOutputStream. It is not final. changed control in LOBInputStrea and LOBOutputStream to final >>10) I think it would be good to split the testSetBytes into one test >>method for in-memory operation, and one for on disk operation. > >fixed >It would also be good if these test cases called close() on >rs. Another thing I noticed is that the test cases contain clean-up >code in a finally block. If an exception is thrown in the finally >block, it will hide the original error, so I think it's better to >remove the finally block. added close and removed fianlly block >>15) Comment why the test is currently only run in embedded in suite(). > >these tests, the way they are written, are specific to embedded >(testing with memory and then file system) and the client already has >similar tests in jdbcapi/LobStreamsTest. >Could you add this comment in the source file as well? Would it run in >client/server mode? If so, I think it would be valuable to run it >under the client too. updated comment in suite method Some other comments to LobStreamTest: tearDown() doesn't call super.tearDown() (if it did, calling rollback() and close() on conn wouldn't be necessary) the instance variables dbName, useLOBStreamControl and f are not used and should be removed the instance variable conn is not needed since the connection is also stored in the super class blob should be set to null in tearDown() called super.tearDown and set blob to null and removed rollback and close call. >Follow up comments from knuth > >>- long finalLen = (dataBytes != null) ? dataBytes.length + b.length >>- : b.length; >>- if (finalLen < MAX_BUF_SIZE) >>+ if (pos + b.length < MAX_BUF_SIZE) >> return updateData(b, off, len, pos); >> else { >> init(dataBytes, pos); >> } > >>Shouldn't b.length have been len, in case we don't write the entire byte array? >>And shouldn't the comparison use <= instead of <? > >changed I noticed that you didn't change the comparison operator. Do you think that comment was wrong? i had missed it changed it in the current patch.
        Hide
        Knut Anders Hatlen added a comment -

        > Are you suggesting to signature change from public StorageFile
        > createTemporaryFile (String, String) to public java.io.File
        > createTemporaryFile (String, String) ?

        No, I didn't notice the difference in signatures. It should still
        return StorageFile, so the method would look something like this:

        public StorageFile createTemporaryFile(String prefix, String suffix)
        throws IOException

        { return newStorageFile(File.createTempFile( prefix, suffix, new File(getTempDir().getPath())).getPath()); }
        Show
        Knut Anders Hatlen added a comment - > Are you suggesting to signature change from public StorageFile > createTemporaryFile (String, String) to public java.io.File > createTemporaryFile (String, String) ? No, I didn't notice the difference in signatures. It should still return StorageFile, so the method would look something like this: public StorageFile createTemporaryFile(String prefix, String suffix) throws IOException { return newStorageFile(File.createTempFile( prefix, suffix, new File(getTempDir().getPath())).getPath()); }
        Hide
        Anurag Shekhar added a comment -

        >BaseStorageFactory.createTemporaryFile() could still be used by other
        >modules if it was written in terms of File.createTempFile(), but the
        >implementation of the method would be simpler, like this:

        > public StorageFile createTemporaryFile(String prefix, String suffix)
        > throws IOException
        >

        { > return File.createTempFile(prefix, suffix, > new File(getTempDir().getPath())); > }

        I got confused and thought you are suggesting to take away the prams (prefix and suffix). Are you suggesting to signature change from public StorageFile createTemporaryFile (String, String) to public java.io.File createTemporaryFile (String, String) ?

        Show
        Anurag Shekhar added a comment - >BaseStorageFactory.createTemporaryFile() could still be used by other >modules if it was written in terms of File.createTempFile(), but the >implementation of the method would be simpler, like this: > public StorageFile createTemporaryFile(String prefix, String suffix) > throws IOException > { > return File.createTempFile(prefix, suffix, > new File(getTempDir().getPath())); > } I got confused and thought you are suggesting to take away the prams (prefix and suffix). Are you suggesting to signature change from public StorageFile createTemporaryFile (String, String) to public java.io.File createTemporaryFile (String, String) ?
        Hide
        Knut Anders Hatlen added a comment -

        >>1) Wouldn't it be simpler to write
        >> BaseStorageFactory.createTemporaryFile() in terms of
        >> File.createTempFile(String prefix, String suffix, File directory)?
        >
        >I thought it may be useful in case some other module decides to use
        >this same service and want to generate file name based of some
        >pattern.

        BaseStorageFactory.createTemporaryFile() could still be used by other
        modules if it was written in terms of File.createTempFile(), but the
        implementation of the method would be simpler, like this:

        public StorageFile createTemporaryFile(String prefix, String suffix)
        throws IOException

        { return File.createTempFile(prefix, suffix, new File(getTempDir().getPath())); }

        This would also remove the need for the counter variable in
        BaseStorageFactory. The only difference would be that the prefix no
        longer could be null, but that should be easy to fix.

        >>5) LOBInputStream.read(byte[]) and read(byte[],int,int) contain this
        >> piece of code:
        >>+ int ret = control.read(b, off, len, pos);
        >>+ if (ret > 0)

        { >>+ pos += ret; >>+ return ret; >>+ }

        >>+ return -1;
        >>Since a call to InputStream.read(byte[]...) theoretically can return
        >>0, I think (ret > 0) should be changed to (ret >= 0) or perhaps (ret
        >>!= -1).
        >
        >this was taken care by one of the privious patch

        The previous patch removed read(byte[]), but read(byte[],int,int) is
        still there with the same code.

        >>11) LOBStreamControl.init() might ignore some exceptions.
        >
        >Added all skipped exception in unexpected exception.

        I'm not sure unexpectedUserException() is the correct method to use to
        wrap the exception in this case. I thought it was supposed to be used
        to wrap exceptions thrown by user code (like stored procedures and
        VTIs). Perhaps Util.javaException() would be better?

        >>1) Can control be final?
        >
        >ClobStreamControl extends from LOBStreamControl. ClobStreamControl is
        >already final.

        I think Kristian was referring to the variable control in
        LOBOutputStream. It is not final.

        >>10) I think it would be good to split the testSetBytes into one test
        >>method for in-memory operation, and one for on disk operation.
        >
        >fixed

        It would also be good if these test cases called close() on
        rs. Another thing I noticed is that the test cases contain clean-up
        code in a finally block. If an exception is thrown in the finally
        block, it will hide the original error, so I think it's better to
        remove the finally block.

        >>15) Comment why the test is currently only run in embedded in suite().
        >
        >these tests, the way they are written, are specific to embedded
        >(testing with memory and then file system) and the client already has
        >similar tests in jdbcapi/LobStreamsTest.

        Could you add this comment in the source file as well? Would it run in
        client/server mode? If so, I think it would be valuable to run it
        under the client too.

        Some other comments to LobStreamTest:

        • tearDown() doesn't call super.tearDown() (if it did, calling
          rollback() and close() on conn wouldn't be necessary)
        • the instance variables dbName, useLOBStreamControl and f are not
          used and should be removed
        • the instance variable conn is not needed since the connection is
          also stored in the super class
        • blob should be set to null in tearDown()

        >Follow up comments from knuth
        >
        >>- long finalLen = (dataBytes != null) ? dataBytes.length + b.length
        >>- : b.length;
        >>- if (finalLen < MAX_BUF_SIZE)
        >>+ if (pos + b.length < MAX_BUF_SIZE)
        >> return updateData(b, off, len, pos);
        >> else

        { >> init(dataBytes, pos); >> }

        >
        >>Shouldn't b.length have been len, in case we don't write the entire byte array?
        >>And shouldn't the comparison use <= instead of <?
        >
        >changed

        I noticed that you didn't change the comparison operator. Do you think
        that comment was wrong?

        Show
        Knut Anders Hatlen added a comment - >>1) Wouldn't it be simpler to write >> BaseStorageFactory.createTemporaryFile() in terms of >> File.createTempFile(String prefix, String suffix, File directory)? > >I thought it may be useful in case some other module decides to use >this same service and want to generate file name based of some >pattern. BaseStorageFactory.createTemporaryFile() could still be used by other modules if it was written in terms of File.createTempFile(), but the implementation of the method would be simpler, like this: public StorageFile createTemporaryFile(String prefix, String suffix) throws IOException { return File.createTempFile(prefix, suffix, new File(getTempDir().getPath())); } This would also remove the need for the counter variable in BaseStorageFactory. The only difference would be that the prefix no longer could be null, but that should be easy to fix. >>5) LOBInputStream.read(byte[]) and read(byte[],int,int) contain this >> piece of code: >>+ int ret = control.read(b, off, len, pos); >>+ if (ret > 0) { >>+ pos += ret; >>+ return ret; >>+ } >>+ return -1; >>Since a call to InputStream.read(byte[]...) theoretically can return >>0, I think (ret > 0) should be changed to (ret >= 0) or perhaps (ret >>!= -1). > >this was taken care by one of the privious patch The previous patch removed read(byte[]), but read(byte[],int,int) is still there with the same code. >>11) LOBStreamControl.init() might ignore some exceptions. > >Added all skipped exception in unexpected exception. I'm not sure unexpectedUserException() is the correct method to use to wrap the exception in this case. I thought it was supposed to be used to wrap exceptions thrown by user code (like stored procedures and VTIs). Perhaps Util.javaException() would be better? >>1) Can control be final? > >ClobStreamControl extends from LOBStreamControl. ClobStreamControl is >already final. I think Kristian was referring to the variable control in LOBOutputStream. It is not final. >>10) I think it would be good to split the testSetBytes into one test >>method for in-memory operation, and one for on disk operation. > >fixed It would also be good if these test cases called close() on rs. Another thing I noticed is that the test cases contain clean-up code in a finally block. If an exception is thrown in the finally block, it will hide the original error, so I think it's better to remove the finally block. >>15) Comment why the test is currently only run in embedded in suite(). > >these tests, the way they are written, are specific to embedded >(testing with memory and then file system) and the client already has >similar tests in jdbcapi/LobStreamsTest. Could you add this comment in the source file as well? Would it run in client/server mode? If so, I think it would be valuable to run it under the client too. Some other comments to LobStreamTest: tearDown() doesn't call super.tearDown() (if it did, calling rollback() and close() on conn wouldn't be necessary) the instance variables dbName, useLOBStreamControl and f are not used and should be removed the instance variable conn is not needed since the connection is also stored in the super class blob should be set to null in tearDown() >Follow up comments from knuth > >>- long finalLen = (dataBytes != null) ? dataBytes.length + b.length >>- : b.length; >>- if (finalLen < MAX_BUF_SIZE) >>+ if (pos + b.length < MAX_BUF_SIZE) >> return updateData(b, off, len, pos); >> else { >> init(dataBytes, pos); >> } > >>Shouldn't b.length have been len, in case we don't write the entire byte array? >>And shouldn't the comparison use <= instead of <? > >changed I noticed that you didn't change the comparison operator. Do you think that comment was wrong?
        Hide
        Anurag Shekhar added a comment -

        reattaching with derby-2247-followup2.diff with Grant license to ASF

        Show
        Anurag Shekhar added a comment - reattaching with derby-2247-followup2.diff with Grant license to ASF
        Hide
        Anurag Shekhar added a comment -

        >1) Wouldn't it be simpler to write
        > BaseStorageFactory.createTemporaryFile() in terms of
        > File.createTempFile(String prefix, String suffix, File directory)?

        I thought it may be useful in case some other module decides to use this same service
        and want to generate file name based of some pattern.

        >5) LOBInputStream.read(byte[]) and read(byte[],int,int) contain this
        > piece of code:
        >+ int ret = control.read(b, off, len, pos);
        >+ if (ret > 0)

        { >+ pos += ret; >+ return ret; >+ }

        >+ return -1;
        >Since a call to InputStream.read(byte[]...) theoretically can return
        >0, I think (ret > 0) should be changed to (ret >= 0) or perhaps (ret
        >!= -1).

        this was taken care by one of the privious patch

        >11) LOBStreamControl.init() might ignore some exceptions.

        Added all skipped exception in unexpected exception.

        >1) Can control be final?

        ClobStreamControl extends from LOBStreamControl. ClobStreamControl is already final.

        >2) Comment why AIOOB is thrown instead of IOException?

        in case of BLOB_INVALID_OFFSET streams are expected to throw AIOOB.

        > 7) In write(byte[],int,int,long), all SQLExceptions except one (BLOB_INVALID_OFFSET) are silently ignored. >Is this intended?

        fixed

        > 8) In free(), is there a chance we ignore some exceptions? If not, add a comment? Or, if it is possible, >create a RuntimeException from the PAE and throw it as last resort?

        It won't be good idea to ignore exception from free as most of the time it will be a i/o error. If we throw a non runtime exception it will be trapped and recorded somewhere in the users of these methods hence making it easy to report problem.
        added a new throw statement to wrap all no IOException uder IOException.

        [BlobSetMethodsTest]
        > 9) Comment why the test is currently only run in embedded in suite().

        enabled it for client too

        >10) I think it would be good to split the testSetBytes into one test method for in-memory operation, and one >for on disk operation.

        fixed

        >11) In tearDown(), you could use "Statement stmt = createStatement()" and at last call super.tearDown().

        added
        [LobStreamTest]

        >12) JavaDoc for testReadWriteNoParameters (and the method name) is a little off.
        fixed

        >13) For the assertEquals comparing input and output, I think it should say "Output does not match input" or >something similar.
        fixed
        >14) Replace assertTrue(msg, false) with fail(msg).
        fixed

        >15) Comment why the test is currently only run in embedded in suite().

        these tests, the way they are written, are specific to embedded (testing with memory and then file system) and the client already has similar tests in jdbcapi/LobStreamsTest.

        Follow up comments from knuth

        >- long finalLen = (dataBytes != null) ? dataBytes.length + b.length
        >- : b.length;
        >- if (finalLen < MAX_BUF_SIZE)
        >+ if (pos + b.length < MAX_BUF_SIZE)
        > return updateData(b, off, len, pos);
        > else

        { > init(dataBytes, pos); > }

        >Shouldn't b.length have been len, in case we don't write the entire byte array?
        >And shouldn't the comparison use <= instead of <?

        changed

        >Actually, I'm also wondering whether this test in LOBStreamControl.write(int,long)
        > if (pos + 1 < MAX_BUF_SIZE) {
        >should have been (pos < MAX_BUF_SIZE).

        >And should this test in LOBStreamControl.truncate()
        if (size < Integer.MAX_VALUE && size < MAX_BUF_SIZE) {
        >have been (size <= MAX_BUF_SIZE)?

        fixed

        Show
        Anurag Shekhar added a comment - >1) Wouldn't it be simpler to write > BaseStorageFactory.createTemporaryFile() in terms of > File.createTempFile(String prefix, String suffix, File directory)? I thought it may be useful in case some other module decides to use this same service and want to generate file name based of some pattern. >5) LOBInputStream.read(byte[]) and read(byte[],int,int) contain this > piece of code: >+ int ret = control.read(b, off, len, pos); >+ if (ret > 0) { >+ pos += ret; >+ return ret; >+ } >+ return -1; >Since a call to InputStream.read(byte[]...) theoretically can return >0, I think (ret > 0) should be changed to (ret >= 0) or perhaps (ret >!= -1). this was taken care by one of the privious patch >11) LOBStreamControl.init() might ignore some exceptions. Added all skipped exception in unexpected exception. >1) Can control be final? ClobStreamControl extends from LOBStreamControl. ClobStreamControl is already final. >2) Comment why AIOOB is thrown instead of IOException? in case of BLOB_INVALID_OFFSET streams are expected to throw AIOOB. > 7) In write(byte[],int,int,long), all SQLExceptions except one (BLOB_INVALID_OFFSET) are silently ignored. >Is this intended? fixed > 8) In free(), is there a chance we ignore some exceptions? If not, add a comment? Or, if it is possible, >create a RuntimeException from the PAE and throw it as last resort? It won't be good idea to ignore exception from free as most of the time it will be a i/o error. If we throw a non runtime exception it will be trapped and recorded somewhere in the users of these methods hence making it easy to report problem. added a new throw statement to wrap all no IOException uder IOException. [BlobSetMethodsTest] > 9) Comment why the test is currently only run in embedded in suite(). enabled it for client too >10) I think it would be good to split the testSetBytes into one test method for in-memory operation, and one >for on disk operation. fixed >11) In tearDown(), you could use "Statement stmt = createStatement()" and at last call super.tearDown(). added [LobStreamTest] >12) JavaDoc for testReadWriteNoParameters (and the method name) is a little off. fixed >13) For the assertEquals comparing input and output, I think it should say "Output does not match input" or >something similar. fixed >14) Replace assertTrue(msg, false) with fail(msg). fixed >15) Comment why the test is currently only run in embedded in suite(). these tests, the way they are written, are specific to embedded (testing with memory and then file system) and the client already has similar tests in jdbcapi/LobStreamsTest. Follow up comments from knuth >- long finalLen = (dataBytes != null) ? dataBytes.length + b.length >- : b.length; >- if (finalLen < MAX_BUF_SIZE) >+ if (pos + b.length < MAX_BUF_SIZE) > return updateData(b, off, len, pos); > else { > init(dataBytes, pos); > } >Shouldn't b.length have been len, in case we don't write the entire byte array? >And shouldn't the comparison use <= instead of <? changed >Actually, I'm also wondering whether this test in LOBStreamControl.write(int,long) > if (pos + 1 < MAX_BUF_SIZE) { >should have been (pos < MAX_BUF_SIZE). >And should this test in LOBStreamControl.truncate() if (size < Integer.MAX_VALUE && size < MAX_BUF_SIZE) { >have been (size <= MAX_BUF_SIZE)? fixed
        Hide
        Anurag Shekhar added a comment -

        Sorry I had missed suresh's query and comment.

        1) LOBStreamControl.java : isValidPostion()
        if (pos > tmpFile.length())
        throw Util.generateCsSQLException(
        SQLState.BLOB_POSITION_TOO_LARGE, new Long(pos + 1));

        isValidPosition() is called on most of the read/write. I think calling
        a file length call can be expensive for each read/write call.

        I will check how can I reduce it. Probably having a length variable in the control class itself will help.

        2) Are blobs written to temp files for some special cases ?
        I was just using BaseJdbcTestCase.java: assertEquals(Blob b1, Blob b2)
        method in my test. It was triggering the writes to temp

        Yes if blob size exceeds 4k its written into the file and subsequent blob operations are operated on file.

        Show
        Anurag Shekhar added a comment - Sorry I had missed suresh's query and comment. 1) LOBStreamControl.java : isValidPostion() if (pos > tmpFile.length()) throw Util.generateCsSQLException( SQLState.BLOB_POSITION_TOO_LARGE, new Long(pos + 1)); isValidPosition() is called on most of the read/write. I think calling a file length call can be expensive for each read/write call. I will check how can I reduce it. Probably having a length variable in the control class itself will help. 2) Are blobs written to temp files for some special cases ? I was just using BaseJdbcTestCase.java: assertEquals(Blob b1, Blob b2) method in my test. It was triggering the writes to temp Yes if blob size exceeds 4k its written into the file and subsequent blob operations are operated on file.
        Hide
        Knut Anders Hatlen added a comment -

        Committed derby-2247-followup.diff with revision 522789.

        Show
        Knut Anders Hatlen added a comment - Committed derby-2247-followup.diff with revision 522789.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks for the follow-up patch, Anurag! It addresses most of my comments. However, I can't see that it addresses my comments 1, 5 and 11. Kristian's questions 1-3, 5 and 7-15 and Suresh's questions are also unanswered. Are you planning to address and/or comment on these questions as well?

        Two follow-up questions to the fix of for my comment #12 (LOBStreamControl.write(byte[],int,int,long)):

        • long finalLen = (dataBytes != null) ? dataBytes.length + b.length
        • : b.length;
        • if (finalLen < MAX_BUF_SIZE)
          + if (pos + b.length < MAX_BUF_SIZE)
          return updateData(b, off, len, pos);
          else { init(dataBytes, pos); }

        Shouldn't b.length have been len, in case we don't write the entire byte array?
        And shouldn't the comparison use <= instead of <?

        Actually, I'm also wondering whether this test in LOBStreamControl.write(int,long)
        if (pos + 1 < MAX_BUF_SIZE) {
        should have been (pos < MAX_BUF_SIZE).

        And should this test in LOBStreamControl.truncate()
        if (size < Integer.MAX_VALUE && size < MAX_BUF_SIZE) {
        have been (size <= MAX_BUF_SIZE)?

        As it is now, I don't think the last byte in the buffer will ever be used.

        Show
        Knut Anders Hatlen added a comment - Thanks for the follow-up patch, Anurag! It addresses most of my comments. However, I can't see that it addresses my comments 1, 5 and 11. Kristian's questions 1-3, 5 and 7-15 and Suresh's questions are also unanswered. Are you planning to address and/or comment on these questions as well? Two follow-up questions to the fix of for my comment #12 (LOBStreamControl.write(byte[],int,int,long)): long finalLen = (dataBytes != null) ? dataBytes.length + b.length : b.length; if (finalLen < MAX_BUF_SIZE) + if (pos + b.length < MAX_BUF_SIZE) return updateData(b, off, len, pos); else { init(dataBytes, pos); } Shouldn't b.length have been len, in case we don't write the entire byte array? And shouldn't the comparison use <= instead of <? Actually, I'm also wondering whether this test in LOBStreamControl.write(int,long) if (pos + 1 < MAX_BUF_SIZE) { should have been (pos < MAX_BUF_SIZE). And should this test in LOBStreamControl.truncate() if (size < Integer.MAX_VALUE && size < MAX_BUF_SIZE) { have been (size <= MAX_BUF_SIZE)? As it is now, I don't think the last byte in the buffer will ever be used.
        Hide
        Suresh Thalamati added a comment -

        While doing some testing I ran into the LOBStreamControl.java in
        the stack. Couple of minor questions.

        1) LOBStreamControl.java : isValidPostion()
        if (pos > tmpFile.length())
        throw Util.generateCsSQLException(
        SQLState.BLOB_POSITION_TOO_LARGE, new Long(pos + 1));

        isValidPosition() is called on most of the read/write. I think calling
        a file length call can be expensive for each read/write call.

        2) Are blobs written to temp files for some special cases ?
        I was just using BaseJdbcTestCase.java: assertEquals(Blob b1, Blob b2)
        method in my test. It was triggering the writes to temp
        file through LOBStreamControl.java , is this expected ?

        Show
        Suresh Thalamati added a comment - While doing some testing I ran into the LOBStreamControl.java in the stack. Couple of minor questions. 1) LOBStreamControl.java : isValidPostion() if (pos > tmpFile.length()) throw Util.generateCsSQLException( SQLState.BLOB_POSITION_TOO_LARGE, new Long(pos + 1)); isValidPosition() is called on most of the read/write. I think calling a file length call can be expensive for each read/write call. 2) Are blobs written to temp files for some special cases ? I was just using BaseJdbcTestCase.java: assertEquals(Blob b1, Blob b2) method in my test. It was triggering the writes to temp file through LOBStreamControl.java , is this expected ?
        Hide
        Anurag Shekhar added a comment -

        Description of patch
        Removed commented code.
        Corrected javadoc problems
        removed redundent checks
        removed check for pos before seek
        removed read write method with single parameneter (byte []) from stream and Control class
        Fix squlstate in truncate method
        Changed the SQLState for closed stream to XCL53 (non sql state error class)
        Added SQLState to the calls to StatdurdException

        Show
        Anurag Shekhar added a comment - Description of patch Removed commented code. Corrected javadoc problems removed redundent checks removed check for pos before seek removed read write method with single parameneter (byte []) from stream and Control class Fix squlstate in truncate method Changed the SQLState for closed stream to XCL53 (non sql state error class) Added SQLState to the calls to StatdurdException
        Hide
        Anurag Shekhar added a comment -

        thanks Kristian and Knut for the review. I will be submitting a follow up patch to address the issues you have pointed out.

        Show
        Anurag Shekhar added a comment - thanks Kristian and Knut for the review. I will be submitting a follow up patch to address the issues you have pointed out.
        Hide
        Knut Anders Hatlen added a comment -

        Some post-commit questions/suggestions:

        1) Wouldn't it be simpler to write
        BaseStorageFactory.createTemporaryFile() in terms of
        File.createTempFile(String prefix, String suffix, File directory)?

        2) EmbedBlob.setBinaryStream() has one check for (pos - 1 > length)
        and one check for (pos > length()). Should the second test also use
        pos-1 and/or could they be written as one test?

        3) The second use of BLOB_POSITION_TOO_LARGE in
        EmbedBlob.setBinaryStream() lacks the position argument.

        4) EmbedBlob.truncate() has this code:
        + if (len > length())
        + throw Util.generateCsSQLException(
        + SQLState.BLOB_NONPOSITIVE_LENGTH, new Long(pos));
        Isn't the SQL state wrong here?

        5) LOBInputStream.read(byte[]) and read(byte[],int,int) contain this
        piece of code:
        + int ret = control.read(b, off, len, pos);
        + if (ret > 0)

        { + pos += ret; + return ret; + }

        + return -1;
        Since a call to InputStream.read(byte[]...) theoretically can return
        0, I think (ret > 0) should be changed to (ret >= 0) or perhaps (ret
        != -1).

        6) Couldn't LOBInputStream.read(byte[]) be implemented as
        public int read(byte[] b) throws IOException

        { return read(b, 0, b.length); }

        ?

        7) Same comment for LOBOutputStream.write(byte[] b). Could be
        implemented as a call to write(b, 0, b.length).

        8) All the methods of LOBInputStream and LOBOutputStream start with
        + if (closed)
        + throw new IOException (
        + MessageService.getTextMessage(
        + SQLState.LANG_STREAM_CLOSED));
        Perhaps the code would be a bit clearer and easier to keep consistent
        if these checks were moved into a utility method?

        9) The patch introduced a new SQL state LANG_STREAM_CLOSED =
        "42Z12". Since it starts with 42, the generated exceptions will be
        of type SQLSyntaxErrorException, which according to the javadoc
        indicate "that the in-progress query has violated SQL syntax
        rules." Does this description match the situations where it is
        used?

        10) EmbedBlob's constructor has this code:
        + catch (IOException e)

        { + throw StandardException.newException (null, e); + }

        I'm a bit worried that passing null as message id could cause
        problems. I think we will get a NullPointerException when
        StandardException's constructor calls getSeverityFromIdentifier(null).

        11) LOBStreamControl.init() might ignore some exceptions.

        12) Is this calculation in LOBStreamControl.write() correct?
        + long finalLen = (dataBytes != null) ? dataBytes.length + b.length
        + : b.length;
        If (pos!=dataBytes.length) and/or (len!=b.length) finalLen will be too
        large, won't it?

        13) LOBStreamControl.write: Do we need the if? I guess seek() would be
        a no-op anyway if getFilePointer() == pos.
        + if (tmpFile.getFilePointer() != pos)
        + tmpFile.seek(pos);

        Show
        Knut Anders Hatlen added a comment - Some post-commit questions/suggestions: 1) Wouldn't it be simpler to write BaseStorageFactory.createTemporaryFile() in terms of File.createTempFile(String prefix, String suffix, File directory)? 2) EmbedBlob.setBinaryStream() has one check for (pos - 1 > length) and one check for (pos > length()). Should the second test also use pos-1 and/or could they be written as one test? 3) The second use of BLOB_POSITION_TOO_LARGE in EmbedBlob.setBinaryStream() lacks the position argument. 4) EmbedBlob.truncate() has this code: + if (len > length()) + throw Util.generateCsSQLException( + SQLState.BLOB_NONPOSITIVE_LENGTH, new Long(pos)); Isn't the SQL state wrong here? 5) LOBInputStream.read(byte[]) and read(byte[],int,int) contain this piece of code: + int ret = control.read(b, off, len, pos); + if (ret > 0) { + pos += ret; + return ret; + } + return -1; Since a call to InputStream.read(byte[]...) theoretically can return 0, I think (ret > 0) should be changed to (ret >= 0) or perhaps (ret != -1). 6) Couldn't LOBInputStream.read(byte[]) be implemented as public int read(byte[] b) throws IOException { return read(b, 0, b.length); } ? 7) Same comment for LOBOutputStream.write(byte[] b). Could be implemented as a call to write(b, 0, b.length). 8) All the methods of LOBInputStream and LOBOutputStream start with + if (closed) + throw new IOException ( + MessageService.getTextMessage( + SQLState.LANG_STREAM_CLOSED)); Perhaps the code would be a bit clearer and easier to keep consistent if these checks were moved into a utility method? 9) The patch introduced a new SQL state LANG_STREAM_CLOSED = "42Z12". Since it starts with 42, the generated exceptions will be of type SQLSyntaxErrorException, which according to the javadoc indicate "that the in-progress query has violated SQL syntax rules." Does this description match the situations where it is used? 10) EmbedBlob's constructor has this code: + catch (IOException e) { + throw StandardException.newException (null, e); + } I'm a bit worried that passing null as message id could cause problems. I think we will get a NullPointerException when StandardException's constructor calls getSeverityFromIdentifier(null). 11) LOBStreamControl.init() might ignore some exceptions. 12) Is this calculation in LOBStreamControl.write() correct? + long finalLen = (dataBytes != null) ? dataBytes.length + b.length + : b.length; If (pos!=dataBytes.length) and/or (len!=b.length) finalLen will be too large, won't it? 13) LOBStreamControl.write: Do we need the if? I guess seek() would be a no-op anyway if getFilePointer() == pos. + if (tmpFile.getFilePointer() != pos) + tmpFile.seek(pos);
        Hide
        Kristian Waagan added a comment -

        Patch 'derby-2247-v4-usingStoreFactory.diff' is equal to the previous patch, except that white space and various other formatting issues have been fixed. Further, the test 'TestBlobSetMethods' were renamed to 'BlobSetMethodsTest' to follow the JUnit naming convention.

        Tests ran without failures, and I committed the patch (v4) with revision 506918.

        I would appreciate a follow-up patch to address some of my comments:
        [LOBOutputStream]
        1) Can control be final?
        2) Comment why AIOOB is thrown instead of IOException?
        [LOBInputStream]
        3) Can control be final?
        [LOBStreamControl]
        4) Delete line that is commented out.
        5) JavaDoc for isValidPosition and -Offset, saying that they throw exceptions.
        6) Remove IOException from the throws-clause for isValidOffset?
        7) In write(byte[],int,int,long), all SQLExceptions except one (BLOB_INVALID_OFFSET) are silently ignored. Is this intended?
        8) In free(), is there a chance we ignore some exceptions? If not, add a comment? Or, if it is possible, create a RuntimeException from the PAE and throw it as last resort?
        [BlobSetMethodsTest]
        9) Comment why the test is currently only run in embedded in suite().
        10) I think it would be good to split the testSetBytes into one test method for in-memory operation, and one for on disk operation.
        11) In tearDown(), you could use "Statement stmt = createStatement()" and at last call super.tearDown().
        [LobStreamTest]
        12) JavaDoc for testReadWriteNoParameters (and the method name) is a little off.
        13) For the assertEquals comparing input and output, I think it should say "Output does not match input" or something similar.
        14) Replace assertTrue(msg, false) with fail(msg).
        15) Comment why the test is currently only run in embedded in suite().

        I'm not setting to fixed, as I hope there will be a follow-up patch

        Good work! Thanks!

        Show
        Kristian Waagan added a comment - Patch 'derby-2247-v4-usingStoreFactory.diff' is equal to the previous patch, except that white space and various other formatting issues have been fixed. Further, the test 'TestBlobSetMethods' were renamed to 'BlobSetMethodsTest' to follow the JUnit naming convention. Tests ran without failures, and I committed the patch (v4) with revision 506918. I would appreciate a follow-up patch to address some of my comments: [LOBOutputStream] 1) Can control be final? 2) Comment why AIOOB is thrown instead of IOException? [LOBInputStream] 3) Can control be final? [LOBStreamControl] 4) Delete line that is commented out. 5) JavaDoc for isValidPosition and -Offset, saying that they throw exceptions. 6) Remove IOException from the throws-clause for isValidOffset? 7) In write(byte[],int,int,long), all SQLExceptions except one (BLOB_INVALID_OFFSET) are silently ignored. Is this intended? 8) In free(), is there a chance we ignore some exceptions? If not, add a comment? Or, if it is possible, create a RuntimeException from the PAE and throw it as last resort? [BlobSetMethodsTest] 9) Comment why the test is currently only run in embedded in suite(). 10) I think it would be good to split the testSetBytes into one test method for in-memory operation, and one for on disk operation. 11) In tearDown(), you could use "Statement stmt = createStatement()" and at last call super.tearDown(). [LobStreamTest] 12) JavaDoc for testReadWriteNoParameters (and the method name) is a little off. 13) For the assertEquals comparing input and output, I think it should say "Output does not match input" or something similar. 14) Replace assertTrue(msg, false) with fail(msg). 15) Comment why the test is currently only run in embedded in suite(). I'm not setting to fixed, as I hope there will be a follow-up patch Good work! Thanks!
        Hide
        Anurag Shekhar added a comment -

        Thanks Narayanan for looking at the patch.

        >Pls mention reason for choosing buffer size as 4096, if there was a particular
        reason for choosing this value. Otherwise pls mention that it was a random
        value.

        it's a randomly selected value. added a comment to indicate that.

        >Can the Monitor and the DataFactory be resused instead of doing a findService,
        findServiceModule each time?

        It can be cached but init is a one time operation. If we want to cache it for a database we will have to add the varriable in connection. I don't think these calls are expensive enough to justify that.

        >String str = df.getStorageFactory().getTempDir().getPath(); (str is not being
        used anywhere)

        It was a leftover of my debugging code taken it off.

        >write methods
        >in all the three set methods. Is it possible that you can combine the code into one method that can
        be called from all the three set methods?

        these overloaded methods are calling out the overloaded methods in StorageRandomAccessFile so actually they are three diff. methods.

        >can materialized can be initialized to false during declaration?
        >can control be initialized to null during declaration?

        false is the default value for uninitialized blob and null is default for uninitialized objects so it doesn't makes any diff.

        >you check the following in the set methods
        >But would'nt isValidPostion(pos) in control.write take care of this and throw
        appropriate exception?

        I moved the check in not materialzed block as that won't go to LobStreamControl.

        >There are a few missing javadocs and some javadocs missing the @throws tags.

        added @throws

        Show
        Anurag Shekhar added a comment - Thanks Narayanan for looking at the patch. >Pls mention reason for choosing buffer size as 4096, if there was a particular reason for choosing this value. Otherwise pls mention that it was a random value. it's a randomly selected value. added a comment to indicate that. >Can the Monitor and the DataFactory be resused instead of doing a findService, findServiceModule each time? It can be cached but init is a one time operation. If we want to cache it for a database we will have to add the varriable in connection. I don't think these calls are expensive enough to justify that. >String str = df.getStorageFactory().getTempDir().getPath(); (str is not being used anywhere) It was a leftover of my debugging code taken it off. >write methods >in all the three set methods. Is it possible that you can combine the code into one method that can be called from all the three set methods? these overloaded methods are calling out the overloaded methods in StorageRandomAccessFile so actually they are three diff. methods. >can materialized can be initialized to false during declaration? >can control be initialized to null during declaration? false is the default value for uninitialized blob and null is default for uninitialized objects so it doesn't makes any diff. >you check the following in the set methods >But would'nt isValidPostion(pos) in control.write take care of this and throw appropriate exception? I moved the check in not materialzed block as that won't go to LobStreamControl. >There are a few missing javadocs and some javadocs missing the @throws tags. added @throws
        Hide
        V.Narayanan added a comment -

        Anurag's code looks very good. My comments are mostly nits.

        java/engine/org/apache/derby/impl/jdbc/LOBStreamControl.java
        ------------------------------------------------------------

        Pls mention reason for choosing buffer size as 4096, if there was a particular
        reason for choosing this value. Otherwise pls mention that it was a random
        value.

        init - line nos 72 and 74


        Can the Monitor and the DataFactory be resused instead of doing a findService,
        findServiceModule each time?

        line no 77
        ----------
        String str = df.getStorageFactory().getTempDir().getPath(); (str is not being
        used anywhere)

        line nos 171,196,229
        --------------------

        write methods

        I notice that you have repeated the code snippet

        if (isBytes) {
        if (pos + 1 < MAX_BUF_SIZE) {
        byte [] bytes =

        {(byte) b}

        ;
        updateData(bytes, 0, 1, pos);
        return pos + 1;
        } else

        { init(dataBytes, pos); }

        }
        if (tmpFile.getFilePointer() != pos)
        tmpFile.seek(pos);
        tmpFile.write(b);
        return tmpFile.getFilePointer();

        in all the three set methods. Is it possible that you can combine the code into one method that can
        be called from all the three set methods?

        java/engine/org/apache/derby/impl/jdbc/EmbedBlob.java
        -------------------------------------------------------
        can materialized can be initialized to false during declaration?
        can control be initialized to null during declaration?
        set methods
        -----------
        you check the following in the set methods

        if (pos - 1 > length())
        throw Util.generateCsSQLException(
        SQLState.BLOB_POSITION_TOO_LARGE, new Long(pos));
        if (pos < 1)
        throw Util.generateCsSQLException(
        SQLState.BLOB_BAD_POSITION, new Long(pos));

        But would'nt isValidPostion(pos) in control.write take care of this and throw
        appropriate exception?

        truncate
        -------
        can you move length checks inside truncate methods into control class similar to
        pos checks?

        Some general comments
        ---------------------
        There are a few missing javadocs and some javadocs missing the @throws tags.

        Show
        V.Narayanan added a comment - Anurag's code looks very good. My comments are mostly nits. java/engine/org/apache/derby/impl/jdbc/LOBStreamControl.java ------------------------------------------------------------ Pls mention reason for choosing buffer size as 4096, if there was a particular reason for choosing this value. Otherwise pls mention that it was a random value. init - line nos 72 and 74 Can the Monitor and the DataFactory be resused instead of doing a findService, findServiceModule each time? line no 77 ---------- String str = df.getStorageFactory().getTempDir().getPath(); (str is not being used anywhere) line nos 171,196,229 -------------------- write methods I notice that you have repeated the code snippet if (isBytes) { if (pos + 1 < MAX_BUF_SIZE) { byte [] bytes = {(byte) b} ; updateData(bytes, 0, 1, pos); return pos + 1; } else { init(dataBytes, pos); } } if (tmpFile.getFilePointer() != pos) tmpFile.seek(pos); tmpFile.write(b); return tmpFile.getFilePointer(); in all the three set methods. Is it possible that you can combine the code into one method that can be called from all the three set methods? java/engine/org/apache/derby/impl/jdbc/EmbedBlob.java ------------------------------------------------------- can materialized can be initialized to false during declaration? can control be initialized to null during declaration? set methods ----------- you check the following in the set methods if (pos - 1 > length()) throw Util.generateCsSQLException( SQLState.BLOB_POSITION_TOO_LARGE, new Long(pos)); if (pos < 1) throw Util.generateCsSQLException( SQLState.BLOB_BAD_POSITION, new Long(pos)); But would'nt isValidPostion(pos) in control.write take care of this and throw appropriate exception? truncate ------- can you move length checks inside truncate methods into control class similar to pos checks? Some general comments --------------------- There are a few missing javadocs and some javadocs missing the @throws tags.
        Hide
        Øystein Grøvlen added a comment -

        I agree with Anurag that it is best to use the database temp directory for temporary storage of LOBs.

        Show
        Øystein Grøvlen added a comment - I agree with Anurag that it is best to use the database temp directory for temporary storage of LOBs.
        Hide
        Anurag Shekhar added a comment -

        The two patch uses two different implementation for creating temporary files
        1. Uses system temp directory.
        2. uses database temp directory.

        I personally prefer 2nd because

        1. all the files related to one db is created in one location
        2. cleanup at the time of startup
        3. You can be sure that we have write access. If general tmp system is used, we might get problems not discovered during boot. Right now derby doesn't asks user to provide a writable temp directory unless its a readonly database.

        please give your views.

        Show
        Anurag Shekhar added a comment - The two patch uses two different implementation for creating temporary files 1. Uses system temp directory. 2. uses database temp directory. I personally prefer 2nd because 1. all the files related to one db is created in one location 2. cleanup at the time of startup 3. You can be sure that we have write access. If general tmp system is used, we might get problems not discovered during boot. Right now derby doesn't asks user to provide a writable temp directory unless its a readonly database. please give your views.
        Hide
        Anurag Shekhar added a comment -

        changes made in derby-2247v2-using_StoreFactory.diff from previous patch

        LobStreamControl class is using StorageFactory to create a file in database temporary file system.
        There is no change in policy file.
        both tests are now part of junit framework but still run under jdbc4 (jdk1.6) as they are using some jdbc4.0 methods (create and free). I will be enabling the network drivers set method test cases to test embedded driver too in a separate patch.

        Additional files modified in this patch
        java/engine/org/apache/derby/io/StorageFactory.java
        added a new method to create temporary file (StorageFactory.createTemporaryFile)

        java/engine/org/apache/derby/impl/io/BaseStorageFactory.java
        Implemented method createTemporaryFile. This method takes care of creating a unique file in database temporary file system.

        java/engine/org/apache/derby/io/StorageRandomAccessFile.java
        added a new method read(byte[] b, int off, int len). This method is used by LobStreamControl to read bytes of given length or the remaining bytes whichever is less. In the absence of this method it need to call read in loop. The only implementation of StorageRandomAccessFile also extends from RandomAccessFile so no new implementation is required.

        java/testing/org/apache/derbyTesting/functionTests/util/corruptio/CorruptRandomAccessFile.java
        java/testing/org/apache/derbyTesting/functionTests/util/corruptio/CorruptBaseStorageFactory.java

        added new methods in these class which forward the call to corresponding methods in torageRandomAccessFile and BaseStorageFactory.

        Show
        Anurag Shekhar added a comment - changes made in derby-2247v2-using_StoreFactory.diff from previous patch LobStreamControl class is using StorageFactory to create a file in database temporary file system. There is no change in policy file. both tests are now part of junit framework but still run under jdbc4 (jdk1.6) as they are using some jdbc4.0 methods (create and free). I will be enabling the network drivers set method test cases to test embedded driver too in a separate patch. Additional files modified in this patch java/engine/org/apache/derby/io/StorageFactory.java added a new method to create temporary file (StorageFactory.createTemporaryFile) java/engine/org/apache/derby/impl/io/BaseStorageFactory.java Implemented method createTemporaryFile. This method takes care of creating a unique file in database temporary file system. java/engine/org/apache/derby/io/StorageRandomAccessFile.java added a new method read(byte[] b, int off, int len). This method is used by LobStreamControl to read bytes of given length or the remaining bytes whichever is less. In the absence of this method it need to call read in loop. The only implementation of StorageRandomAccessFile also extends from RandomAccessFile so no new implementation is required. java/testing/org/apache/derbyTesting/functionTests/util/corruptio/CorruptRandomAccessFile.java java/testing/org/apache/derbyTesting/functionTests/util/corruptio/CorruptBaseStorageFactory.java added new methods in these class which forward the call to corresponding methods in torageRandomAccessFile and BaseStorageFactory.
        Hide
        Daniel John Debrunner added a comment -

        >> Why use the system temp folder and not the database's? The database's temp folder will be cleaned up on re-boot.
        >
        >> Could a existing store file be used so that encrpytion support would already be there?
        >
        > I was planning to use database tmp directory and one of the patch in the parent issue uses database tmp directory and StoreFactory but lattered in the disscussion it was concluded that the store api and tmp file system are not supposed to be used for creating random files which are not part of database.

        I thought that discussion was about using store code in the network client., whereas this is about using the code in embedded. With embedded the code is there and will by definition already have the correct security permissions set up.

        Show
        Daniel John Debrunner added a comment - >> Why use the system temp folder and not the database's? The database's temp folder will be cleaned up on re-boot. > >> Could a existing store file be used so that encrpytion support would already be there? > > I was planning to use database tmp directory and one of the patch in the parent issue uses database tmp directory and StoreFactory but lattered in the disscussion it was concluded that the store api and tmp file system are not supposed to be used for creating random files which are not part of database. I thought that discussion was about using store code in the network client., whereas this is about using the code in embedded. With embedded the code is there and will by definition already have the correct security permissions set up.
        Hide
        Anurag Shekhar added a comment -

        Q1 - The Blob set methods are part of JDBC 3 so can this test be in tests.jdbcapi so it can be run on all JVMs rather than just Java SE 6?

        Yes I should do that I will modify the location and test suit entries.

        Q2 - Could the test be written as a JUnit test and not an old harness test?

        They are actually junit tests I should add the entry in junit test suit.

        Show
        Anurag Shekhar added a comment - Q1 - The Blob set methods are part of JDBC 3 so can this test be in tests.jdbcapi so it can be run on all JVMs rather than just Java SE 6? Yes I should do that I will modify the location and test suit entries. Q2 - Could the test be written as a JUnit test and not an old harness test? They are actually junit tests I should add the entry in junit test suit.
        Hide
        Anurag Shekhar added a comment -

        >Why use the system temp folder and not the database's? The database's temp folder will be cleaned up on re-boot.

        >Could a existing store file be used so that encrpytion support would already be there?

        I was planning to use database tmp directory and one of the patch in the parent issue uses database tmp directory and StoreFactory but lattered in the disscussion it was concluded that the store api and tmp file system are not supposed to be used for creating random files which are not part of database.

        Show
        Anurag Shekhar added a comment - >Why use the system temp folder and not the database's? The database's temp folder will be cleaned up on re-boot. >Could a existing store file be used so that encrpytion support would already be there? I was planning to use database tmp directory and one of the patch in the parent issue uses database tmp directory and StoreFactory but lattered in the disscussion it was concluded that the store api and tmp file system are not supposed to be used for creating random files which are not part of database.
        Hide
        Daniel John Debrunner added a comment -

        java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestBlobSetMethods.java

        Test case for embedded blob set methods.

        java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/LobStreamTest.java

        Test case for blob stream.

        java/testing/org/apache/derbyTesting/functionTests/suites/jdbc40.runall
        added the tests part of jdbc40 test suite

        java/testing/org/apache/derbyTesting/functionTests/suites/DerbyNetClient.exclude


        Q1 - The Blob set methods are part of JDBC 3 so can this test be in tests.jdbcapi so it can be run on all JVMs rather than just Java SE 6?

        Q2 - Could the test be written as a JUnit test and not an old harness test?

        Show
        Daniel John Debrunner added a comment - java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestBlobSetMethods.java Test case for embedded blob set methods. java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/LobStreamTest.java Test case for blob stream. java/testing/org/apache/derbyTesting/functionTests/suites/jdbc40.runall added the tests part of jdbc40 test suite java/testing/org/apache/derbyTesting/functionTests/suites/DerbyNetClient.exclude Q1 - The Blob set methods are part of JDBC 3 so can this test be in tests.jdbcapi so it can be run on all JVMs rather than just Java SE 6? Q2 - Could the test be written as a JUnit test and not an old harness test?
        Hide
        Daniel John Debrunner added a comment -

        Why use the system temp folder and not the database's? The database's temp folder will be cleaned up on re-boot.

        Could a existing store file be used so that encrpytion support would already be there?

        Show
        Daniel John Debrunner added a comment - Why use the system temp folder and not the database's? The database's temp folder will be cleaned up on re-boot. Could a existing store file be used so that encrpytion support would already be there?
        Hide
        Anurag Shekhar added a comment -

        this patch doesn't address encryption issue when the data base is encrypted. I will submit another patch to add support for encryption in temporary file.

        Show
        Anurag Shekhar added a comment - this patch doesn't address encryption issue when the data base is encrypted. I will submit another patch to add support for encryption in temporary file.
        Hide
        Anurag Shekhar added a comment -

        please have a look at the comments in DERBY-1341 for some discussion about this patch.

        Show
        Anurag Shekhar added a comment - please have a look at the comments in DERBY-1341 for some discussion about this patch.
        Hide
        Anurag Shekhar added a comment -

        description of patch
        I have added set methods for blob in embedded driver. The set methods are provided by introducing a new class LOBStreamControl. This class uses a temporary file if the data size is more than 4k. Random access file is used to read/write to the file. This class provides methods to use the data as byte array and streams to read/write data bytes.

        new files
        java/engine/org/apache/derby/impl/jdbc/LOBStreamControl.java

        This class maintains the data bytes once the data is materialized, which can happen when
        1. The blob is smaller than one page in that case there won't be any reference to store pages in blob class.
        2. At the first call to setbytes or when a output stream is retrieved from the blob. In this case the data will be materialized in LOBStreamControl class.

        java/engine/org/apache/derby/impl/jdbc/LOBOutputStream.java
        java/engine/org/apache/derby/impl/jdbc/LOBInputStream.java

        This two classes operate on LOBStreamControl to read or write bytes.

        java/engine/org/apache/derby/impl/jdbc/EmbedBlob.java

        EmbedBlob class is modified to use LOBStreamControl. current implementation uses bytes array within the same class, now this is moved to LOBStreamControl. All read and write method is forwarded to LOBStreamControl if the data is materialized.

        java/shared/org/apache/derby/shared/common/reference/SQLState.java
        New sql state is added for closed stream.

        java/testing/org/apache/derbyTesting/functionTests/util/derby_tests.policy

        added permission to read/write/delete in system's temporary directory

        java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestBlobSetMethods.java

        Test case for embedded blob set methods.

        java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/LobStreamTest.java

        Test case for blob stream.

        java/testing/org/apache/derbyTesting/functionTests/suites/jdbc40.runall
        added the tests part of jdbc40 test suite

        java/testing/org/apache/derbyTesting/functionTests/suites/DerbyNetClient.exclude

        excluded the test cases from network driver.

        Show
        Anurag Shekhar added a comment - description of patch I have added set methods for blob in embedded driver. The set methods are provided by introducing a new class LOBStreamControl. This class uses a temporary file if the data size is more than 4k. Random access file is used to read/write to the file. This class provides methods to use the data as byte array and streams to read/write data bytes. new files java/engine/org/apache/derby/impl/jdbc/LOBStreamControl.java This class maintains the data bytes once the data is materialized, which can happen when 1. The blob is smaller than one page in that case there won't be any reference to store pages in blob class. 2. At the first call to setbytes or when a output stream is retrieved from the blob. In this case the data will be materialized in LOBStreamControl class. java/engine/org/apache/derby/impl/jdbc/LOBOutputStream.java java/engine/org/apache/derby/impl/jdbc/LOBInputStream.java This two classes operate on LOBStreamControl to read or write bytes. java/engine/org/apache/derby/impl/jdbc/EmbedBlob.java EmbedBlob class is modified to use LOBStreamControl. current implementation uses bytes array within the same class, now this is moved to LOBStreamControl. All read and write method is forwarded to LOBStreamControl if the data is materialized. java/shared/org/apache/derby/shared/common/reference/SQLState.java New sql state is added for closed stream. java/testing/org/apache/derbyTesting/functionTests/util/derby_tests.policy added permission to read/write/delete in system's temporary directory java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestBlobSetMethods.java Test case for embedded blob set methods. java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/LobStreamTest.java Test case for blob stream. java/testing/org/apache/derbyTesting/functionTests/suites/jdbc40.runall added the tests part of jdbc40 test suite java/testing/org/apache/derbyTesting/functionTests/suites/DerbyNetClient.exclude excluded the test cases from network driver.

          People

          • Assignee:
            Anurag Shekhar
            Reporter:
            Anurag Shekhar
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development