Details
-
Sub-task
-
Status: Closed
-
Major
-
Resolution: Fixed
-
None
-
None
Description
DERBY-2347 adds the possibility to send locators between client and server instead of LOB values. This has not been activated yet, since the client implementation does not currently support locators. This report is for supporting the locators for Blob objects. Another JIRA issue will be made for Clob.
This work will be made in several steps:
1. Blob methods and ResultSet.getXXX methods
2. PreparedStatement and CallableStatement methods
3. ResultSet.updateXXX methods
4. Connection.createBlob()
There is dependencies between these steps and it might be that the Locator implementation cannot be exposed until everything has been done. At least, doing just step 1, gives testing errors because tests use Blobs fetched from DB as parameters to prepared statements. I would guess tests for updatable result sets, needs the combination of 1. and 3.
Attachments
Attachments
- blob_v2.diff
- 29 kB
- Oystein Grovlen
- blob.diff
- 29 kB
- Oystein Grovlen
- blob-followup_v2.diff
- 14 kB
- Oystein Grovlen
- blob-followup_v2.diff
- 14 kB
- Oystein Grovlen
- blob-followup.diff
- 13 kB
- Oystein Grovlen
- blobtestfix.diff
- 12 kB
- Oystein Grovlen
- enableblobloc.diff
- 16 kB
- Oystein Grovlen
Issue Links
- blocks
-
DERBY-2506 Adding the locator information to FD:OCA descriptor (FDODSC) andFD:OCA data (FDODTA) of the SQLDTA objects
- Closed
-
DERBY-2587 Connection.createClob() and Connection.createBlob() need to return locator support enabled LOB objects in the NetworkClient
- Closed
-
DERBY-2702 Enable Clob locator support between NetworkServer and NetworkClient and modify tests that experience changed behaviour due to this(enabling Clob Locators).
- Closed
- is part of
-
DERBY-208 Add support to retrieve lobs for Network Server by locator rather than matierializing the LOB
- Closed
- relates to
-
DERBY-6923 Passing a ClientClob or ClientBlob from another connection to an INSERT statement results in 'null' being written.
- Open
Activity
I have derbyall and the Junit all suite. Got one failure in AuthenticationTest.testGreekCharacters() that does not seem related to my patches. I have also rerun the test 6 times without getting the error.
Uploaded a new version of this patch, blob_v2.diff. The only change is that I have made Lob.getLocator public (instead of protected). This is needed by some of the other JIRAs that is depending on this patch.
Am running regression tests against blog_v2.diff now.
The patch looks good to me. I have a couple small nits, none of which would block me from committing this work:
1) In the header comment for BlobLocatorOutputStream, I don't understand the reference to ByteArrayInputStream
2) There are several places in the code where the magic number -1 is used. For instance, the initialization of Lob.locator_. I think it would be more readable and less brittle if this magic number had a friendly name.
3) I wonder if Lob.getLocatorLength() should be abstract and the appropriate subclass could just throw an unimplementedFeature exception right now. It looks to me as though it would be a coding error if, in the released product, a -1 leaked out of this method.
Committed blob_v2.diff at subversion revision 533268. The regression tests ran cleanly for me too.
Thanks for committing this Rick. I have responded to your comments
below and the attached patch blob-followup.diff adresses those
comments. In addition, this patch contains the following changes:
- My last-minute changes to Lob.sqlLength() introduced a major bug
that try to materialize the Lob also when locators are used. (Would
cause NPE when locators were enabled.) This has been fixed.
- Blob.getBinaryStream(long, long) has been implemented. I had
missed this since it was added after I started this work. This
required changes to BlobLocatorInputStream to handle non-default
start position and length.
- The indentation of the new files,
BlobLocatorInputStream.java/BlobLocatorOutputStream.java was
irregular in the version that was checked in. (I do not think it
was this way in my patch.) I have reindented these files.
> Rick Hillegas commented on DERBY-2496:
> --------------------------------------
> The patch looks good to me. I have a couple small nits, none of
> which would block me from committing this work:
>
> 1) In the header comment for BlobLocatorOutputStream, I don't
> understand the reference to ByteArrayInputStream
Sorry, cut&paste error. Have been fixed.
>
> 2) There are several places in the code where the magic number -1 is
> used. For instance, the initialization of Lob.locator_. I think
> it would be more readable and less brittle if this magic number
> had a friendly name.
I have added a constant, Lob.INVALID_LOCATOR, that is used instead of
the magic number where appropriate. I have not changed the use of -1
to signal that there is no more data on a stream since the
specification says -1 should be returned, and using a named constant
would just create confusion.
>
> 3) I wonder if Lob.getLocatorLength() should be abstract and the
> appropriate subclass could just throw an unimplementedFeature
> exception right now. It looks to me as though it would be a
> coding error if, in the released product, a -1 leaked out of this
> method.
I agree that this should be done, but I would like to wait with this
change since it will probably cause coflicts with Narayanan's work on
Clob. Once, Narayanan has added Clob.getLocatorLength(), I will make
Lob.getLocatorLength abstract.
Forgot to state that blob-followup.diff, changes the following files:
M java/client/org/apache/derby/client/net/NetCursor.java
M java/client/org/apache/derby/client/am/BlobLocatorOutputStream.java
M java/client/org/apache/derby/client/am/Blob.java
M java/client/org/apache/derby/client/am/BlobLocatorInputStream.java
M java/client/org/apache/derby/client/am/Lob.java
and that I am currently running the test suites and will report back when they are finished.
Thanks for the changes, Øystein. I am afraid I'm having some trouble applying blob-followup.diff. My patch tool could not apply the changes in BlobLocatorInputStream. So I tried applying them by hand. I ended up with compile errors in Blob.java because:
1) Blob.getBinaryStream() throws SQLException, not SqlException
2) But that method creates a BlobLocatorInputStream. The constructors for BlobLocatorInputStream throw SqlException, not SQLException.
Can you regenerate the patch and let me know what your exception signatures are for Blob.getBinaryStream() and for the constructors of BlobLocatorInputStream?
> Rick Hillegas commented on DERBY-2496:
> --------------------------------------
>
> Thanks for the changes, Øystein. I am afraid I'm having some
> trouble applying blob-followup.diff. My patch tool could not apply
> the changes in BlobLocatorInputStream. So I tried applying them by
> hand. I ended up with compile errors in Blob.java because:
Sorry about that. I do not understand how I got that to compile. I
have attached a new version of the patch, blob-followup_v2.diff where
I make sure to catch the SqlException. I also updated my sandbox, so
hopefully you will be able to apply this version.
Thanks for the new patch, Øystein. Looks good to me and the tests run cleanly. Committed at subversion revision 535027.
there's a javadoc warning and SANITY build message resulting from the patch:
-
-
- Failure
[javadoc]
trunk\java\client\org\apache\derby\client\am\BlobLocatorInputStream.java:87:
warning - @param argument "offset" is not a parameter name.
[javadoc] 1 warning
- Failure
-
-
-
- Failure\buildZip.txt
[java] SANITY >>>
/org/apache/derby/client/am/BlobLocatorInputStream.class
- Failure\buildZip.txt
-
Re the second, a SanityManager.ASSERT(..) statement needs to have an
if (SanityManager.DEBUG) block, similar to what was put in with revision
533876, after the previous check in for bug DERBY-2496 with revision
533268.
Looks like the BlobLocatorInputStream problem has already been fixed. I
will fix the javadoc issue. Thank You for pointing this out.
Unchecking patch available since it seems there are no patches pending commit.
The patch, blobtestfix.diff, changes a few Junit tests to not expect Blob objects to valid after transaction has committed. This is done in two ways:
1) Delay the closing of the result set until the Blob has been accessed.
2) Switch off autocommit.
The first change is used where possible, but where Blob objects obtained from different statements are to be compared, autocommit can not be used. It is safe to apply these test changes before the use of locators are enabled since it a more restrictive lifespan for Blob objects are assumed.
The patch also contains two protocol fixes uncovered while running the tests with locators enabled:
- The type of locator requested from the server depends on the nullability of the column
- If the column value of zero is received, this is not a locator, but means that length of LOB column is 0.
Also added bufferering to InputStreams in order to significantly reduce the run time of some test cases.
derbyall has been run without errors. Junit All suite has been run with the regular procedureInTrigger and encryption test errors.
File-by-file explanation of changes:
M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java
- Adjusted tests to not expect Blob objects to be valid after
transaction commit.
M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/PreparedStatementTest.java
- Adjusted tests to not expect Blob objects to be valid after
transaction commit. - Rewrote testSetBlobLengthless to take advantage of the existence of
Connection.createBlob
M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java
- Adjusted tests to not expect Blob objects to be valid after
transaction commit.
M java/client/org/apache/derby/client/net/NetCursor.java
- If the column value is zero, a locator was not sent. Instead, this
means that the length of the LOB is 0. Changed locator() to return
INVALID_LOCATOR when the column value is zero.
M java/client/org/apache/derby/client/net/NetStatementRequest.java
- If column is not nullable, a non-nullable locator must be requested.
Otherwise, the server will send an extra byte for nullability which
is not expected by the client.
M java/client/org/apache/derby/client/am/BlobLocatorOutputStream.java
- Added an assert that checks that the underlying Blob object is
locator based.
M java/client/org/apache/derby/client/am/Blob.java
- Add buffering for InputStreams by wrapping BlobLocatorInputStream in
a BufferedInputStream. (Doing the same for OutputStreams is not that
straight-forward since that requires that the stream is flushed
before the statement is executed.)
M java/client/org/apache/derby/client/am/BlobLocatorInputStream.java
- Removed a TAB. Client code is supposed to be a TAB-free zone!
The attached patch, enableblobloc.diff, contains the the necessary
changes to enable the use of locators for Blob. The patch addresses
the following issues:
- Enable the use of locators.
- Modification of tests that experience changed behavior with locators
- If stored procedures reports that the given locator can not be
found, it is assumed that this is because transaction has been
terminated and an "Blob accessed after commit" error is
reported. This makes the client driver and the embedded driver
report the same error for this case.
I am currently running Junit All and derbyall and will report back if
any problems is seen. (The tests have been earlier successfully run
on a slightly different version of the patch)
A file-by-file description of the changes:
M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/BlobClob4BlobTest.java
- testBlobAfterCommit is changed to test that Blobs on the
client is no longer valid after transaction has committed.
M java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/_Suite.java
- Removed SuicideOfStreamingTest since Layer B Streaming will no
longer be used in the scenario tested.
M java/client/org/apache/derby/client/net/NetResultSetRequest.java
- If ResultSet contain Blob columns, request locator to be sent
instead of Blob value.
M java/client/org/apache/derby/client/am/Connection.java
- If a valid locator is obtained from server, create a locator
based LOB object.
M java/client/org/apache/derby/client/am/Blob.java
- Removed an unecessary import added by earlier patch
M java/client/org/apache/derby/client/am/CallableLocatorProcedures.java
- Upon an error, for all procedure calls that specify an locator,
check whether the error was caused by an invalid locator. If
yes, report BLOB_ACCESSED_AFTER_COMMIT, instead. Due to a
problem with JDBC 4 drivers, the patch treat all errors during
execution of a stored procedure this way. This is because for
JDBC 4, the actual error is not propagated to the server. I have
tried to find a way to fix that, but have decided to wait for the
changes that Knut Anders has announced inDERBY-2472to see if
that may improve things.
A release note is needed to describe the changes to the lifespan of Blob objects.
derbyall ran without failures. Junit All had two well-known intermittent failures (procedureInTrigger and NistScripts).
> Due to a problem with JDBC 4 drivers, the patch treat all errors
> during execution of a stored procedure this way. This is because for
> JDBC 4, the actual error is not propagated to the server. I have
> tried to find a way to fix that, but have decided to wait for the
> changes that Knut Anders has announced in DERBY-2472 to see if that
> may improve things.
I think this is DERBY-1440, for which I have uploaded a patch you
could test.
enableblobloc.diff looks good to me. Committed revision 542011.
One question: What should happen with SuicideOfStreamingTest? Should it be removed or should it be modified?
FYI, now that DERBY-2692 has been committed, it should be possible to use getNextException() and getSQLState() to check whether the stored procedure failed because the locator was invalid.
This issue needs a releasenote.
Also, a decision/action re SuicideOfStreamingTest is pending.
Can the check for invalid locator with getNextException() and getSQLState() be implemented before 6/14? Does it need to be?
If not, please make a separate issue so this can be closed...
Release Note: I think it will be best to make a single release note for all locator work and attach that to DERBY-208. I will write a release note and attach it to there by early next week.
suicideOfStreamingTest: The test tested error behavior for Layer B streaming. The scenario in the test no longer involve Layer B streaming. Maybe the test should be changed to a scenario that still use Layer B streaming. I am not an expert here, so I suggest making a new JIRA where this can be discussed.
getNextException etc.: Handled in DERBY-2770.
Taking off existing application impact. It's set at the top level, which is where the release note lives...
The attached patch, blob.diff, adds support for locators for Blob methods and ResultSet.getXXX methods. Note that the use of locators for Blob is still not enabled. The other steps mentioned in this Jira is also needed before locators can be used.
Another thing to note about this patch is that the implementation of locator based streams are pretty basic. No bufferering is done. Hence, the byte[] versions of read/write should be used if performance is a concern. (The parameterless versions of read/write will only send one byte per client/server round-trip.)
More detailed description of changes:
M java/client/org/apache/derby/client/net/NetStatementRequest.java
M java/client/org/apache/derby/client/net/NetCursor.java
M java/client/org/apache/derby/client/am/Cursor.java
M java/client/org/apache/derby/client/am/Blob.java
M java/client/org/apache/derby/client/am/Lob.java
A java/client/org/apache/derby/client/am/BlobLocatorOutputStream.java
A java/client/org/apache/derby/client/am/BlobLocatorInputStream.java