LOB locators are not released if the LOB columns are not accessed by the client

• Key: DERBY-3571
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Kristian Waagan
• Reporter: Kristian Waagan
• Component/s: JDBC, Network Client
• Affects Version/s: 10.3.2.1, 10.4.1.3, 10.5.1.1
• Fix Version/s: 10.3.3.0, 10.4.1.3, 10.5.1.1

Description

If the client creates a result set containing LOB locator columns and iterates through it without actually accessing the LOB columns, the locators are not released.
The amount of locators and their associated LOB objects causes the server to consume large amounts of memory and it eventually gets an OOME.

There are a few workarounds for this bug:
 a) Access and/or properly close the LOBs (i.e. Blob.free).
    This is partly dependent on DERBY-2892.
 b) Invoke Connection.commit (or rollback) periodically, which causes all locators on the connection to be released.

Comments

[Kristian Waagan]

'derby-3571-1a-client_track_lob_fix.diff' is the first attempt at a fix where the client keeps track of LOBs and closes them as appropriate.
The basic approach is:
  a) When the result set position is changed, check the current row (if any) for LOB columns with open locators.
  b) When the result set is closed, check the current row (if any).
  c) If a Blob or Clob object is created, inform the state tracker that its locator shall not be released.
  d) On connection commit/rollback, discard all client side LOB state as all locators are released by the server.

Currently there will be a callable procedure invocation (and thus a round trip) for each LOB column each time the result set position changes (i.e. rs.next or rs.relative). This is very ineffective, and for the current solution there are two optimizations that can be made:
  1) Implement a new stored procedure that takes a list of locators. All locators are freed in one round trip. The complication is to decide when the procedure should be invoked, i.e. there must typically be some kind of threshold (number of locators).
  2) Piggy-back locators on other requests to the server. Frees resources optimally at almost no extra cost. There are several variations, for instance doing it on the connection level or only on the resultset/cursor level (CNTQRY).

Since we don't do prefetching of result set data containing LOBs, the difference between the optimizations is less pronounced, but (1) will most likely leave resources open for a longer period of time on the server.

Another thing to consider is how important it is that this feature is very effective. Why query the database for data you don't need?


Please review and comment.
I'm running tests, and I have run the repro mentioned in DERBY-2892 without failure.

[Kristian Waagan]

Clearing patch available, as the patch 1a causes errors in the tests.
I still want comments on the approach taken, and I plan to fix the bug (NPE) as in patch 1b.
The problem is that the LOB state tracker is not always initialized at the time it is attempted accessed.

Hi Kristian,

I think the approach sounds good. Of course, having one of the
optimizations you mentioned would be good, preferably option 2. I
think there are valid and meaningful use cases where a user doesn't
look at all the LOBs fetched, so it would be worthwhile to optimize
it. For instance, you could have code structured like this

  ResultSet rs = stmt.executeQuery("select int1, int2, string1, blob1 from t");
  while (rs.next()) {
    ...
    if (someConditionIsTrue) {
        processBlob(rs.getBlob(4));
    }
    ...
  }

But optimization can come later. First step is to fix the leak.

I have read through the patch. Please see my (minor) comments below.

* NetConnection:
  - typo: "layber B" -> "layer B"

* Cursor:
  - typo: getLocatorProedures -> getLocatorProcedures

* NoOpLOBStateTracker:
  - class javadoc (and assert in noRelease()) indicates that the class
    is only used when the server doesn't support locators. Isn't it
    also used when the result doesn't contain LOBs?

  - should we have a static instance of this class, so that we don't
    have to allocate a new one each time a statement without LOBs is
    executed? Since it doesn't do anything, there's no thread-safety
    issues preventing us from sharing the same instance between all
    the threads, I think.

* ResultSet:

  - it would be good if keepLocatorColumnAlive() and
    createLOBColumnTracker() had javadoc comments

+ if (this.lobState != null) {
+ if (SanityManager.DEBUG) {
+ SanityManager.THROWASSERT(
+ "LOB state tracker already initialized.");
+ }
+ }

Perhaps the above code is clearer if it's written like this?
    if (SanityManager.DEBUG) {
        SanityManager.ASSERT(this.lobState == null, "...");
    }

+ if (type == Types.BLOB) {
+ tmpIndexes[lobCount] = i +1; // Convert to 1-based index.
+ tmpIsBlob[lobCount++] = true;
+ } else if (type == Types.CLOB) {
+ tmpIndexes[lobCount] = i +1; // Convert to 1-based index.
+ tmpIsBlob[lobCount++] = false;
+ }

Perhaps the above code could be more compact, like this?
    if (type == Types.BLOB || type == Types.CLOB) {
        tmpIndexes[lobCount] = i + 1; // Convert to 1-based index.
        tmpIsBlob[lobCount++] = (type == Types.BLOB);
    }

[Kristian Waagan]

Thanks for the comments Knut Anders.
This is work in progress, so the next patch will have some changes.

I will fix the typos, and also add more comments/JavaDoc. I renamed the method keepLocatorColumnAlive to markLOBAsAccessed, in hope that it gives more meaning...
See below regarding the rest of your comments.

* NoOpLOBStateTracker
  Yes, that is a good idea. It is now a singleton.
  It is also correct that it will be used for result sets without LOBs.

* Fixed assert.

* Rewrote code block for compactness.
  On a side note, this code will only be in 10.3. For 10.4 and trunk I plan to make a few changes so it is no longer required to know if it is a Blob or a Clob. This requires two changes; a small refactoring (issue not yet created) and optimization 1 mentioned in my earlier comment.


I will upload a replacement patch shortly, which addresses other problems in 1a.

[Kristian Waagan]

'derby-3571-1b-client_track_lob_fix.diff' replaces version 1a.
It addresses the comments made by Knut Anders, and the following changes:
 * LOB state object properly initialized
 * Check if positioned on insert row before checking current row (see the closeX-method).
 * Changed some state maintenance in LOBStateTracker.

I ran suites.All without failures (except the JMX failure), but I made some last minutes changes so I'm rerunning.
I plan to get this code into 10.3, 10.4 and trunk. The optimization will be handled under a separate JIRA and will only go into 10.4 and trunk.

[Kristian Waagan]

New revision of the patch ready for review.

[Kristian Waagan]

'derby-3571-1c-client_track_lob_fix.diff' replaces an int array with a bool array in LOBStateTracker, as only two states are required.
Also contains some JavaDoc changes.

Tests ran cleanly.
I'm considering 1c for commit.

[Kristian Waagan]

'derby-3571-1d-client_track_lob_fix.diff' introduces a LOBStateTracker interface and two implementations of it: SingleReleaseLOBTracker and NoOpLOBTracker. As a follow-up patch for DERBY-3575, I plan to add MultipleReleaseLOBTracker.

The trackers will use different mechanisms to release the locators. A final step, which I don't plan to implement for 10.4, is to piggy-back locators to release to the server on other requests. This approach is why I chose to add the locatorsForRelease method.

To sum of the mechanism used by the trackers:
 * NoOpLOBTracker: no release
 * SingleReleaseLOBTracker: a stored procedure call for each locator to release, invoked at every result set navigational methods and rs.close.
 * MultipleReleaseLOBTracker: a stored procedure call for a set of locators to release, invocation time not yet decided.

I'm running tests, and if they pass I plan to commit this patch later today.

[Kristian Waagan]

The tests ran cleanly, but a commit for DERBY-2892 caused a minor conflict.
I have updated the patch (revision 1e), and I'm running a subset of the tests to see if the two patches fare well together.

I still expect to commit the patch shortly.

[Knut Anders Hatlen]

Just a quick control question:

The plan is to have three different mechanisms for releasing LOBs in the client driver?

  a) SingleReleaseLOBTracker if the server version is 10.3
  b) MultipleReleaseLOBTracker if the server version is 10.4
  c) some yet to be implemented piggybacking protocol if the server version is 10.5 or higher

Won't this add up to a fair amount of code that, once newer versions are released, ends up being more or less dead and untested code? Or is there a way some of the mechanisms could be consolidated or perhaps phased out later?

[Knut Anders Hatlen]

All elements of SingleReleaseLOBTracker.trackColumn[] are false until checkCurrentRow() is called (normally on ResultSet.next()). Doesn't this mean that the LOBs in the first row returned from the query won't be released until the transaction is completed?

In SingleReleaseLOBTracker.checkCurrentRow(), should trackColumn[i] be set to true unconditionally rather than in the else block? Whether or not column i were accessed in row n shouldn't affect whether we release the locator in row n+1, should it?

Nit: SingleReleaseLOBTracker.noRelease() doesn't need the sanity check variable (indexFound) if "break" is replaced with "return", and "ASSERT" is replaced with "THROWASSERT". Actually, since columns[] is a sorted int array (isn't it?), it should be possible to simplify this method further by using Arrays.binarySearch().

Typo in ResultSet.createLOBColumnTracker(): "simply" -> "simplify"

[Kathey Marsden]

In SingleReleaseLOBTracker,
I am sure I am missing something, but looking at the patch, I don't understand how the elements of the trackColumn array are ever set to true, so that the lobs get released. Shouldn't they be initialized to true in the constructor and reset to true in discardState()? Then they would get set to false by noRelease() when the lob is accessed.

[Knut Anders Hatlen]

Should we also add !connection_.autoCommit_ as a condition for invoking checkCurrentRow() in ResultSet.closeX()? As it is now, we'll send a release command to the server right before ResultSet.close() triggers a commit, but in that case the commit would have released the locator anyway.

[Kathey Marsden]

You can ignore my comment. I didn't see Knut's before I posted.

[Kristian Waagan]

Thanks for all the reviewing Knut and Kathey.

This is still rather "unstable" code, and I have newer versions in the pipeline. Let me comment on the simple things first.

a) No release of LOBs in the first row.
  This is a bug introduced when I made it a requirement that checkCurrentRow should only be called on a vaild row. I'll fix this.

b) Setting trackColumn to true in checkCurrentRow.
  Won't it always be true with the current code? To enter the if-block it must be true, and we don't need to set it again. Otherwise we enter the else-block and set it to true. I do see the point of setting it to true unconditionally for readability though. In a newer patch (not published yet), I have renamed "trackColumn" to "accessed".

c) noRelease
  I'll rewrite it using Arrays and remove the check variable. Another question is whether an exception should be thrown in any case, as providing an invalid index seems like a programming error.
  Further, I was thinking about using noRelease to thrown an exception if the LOB column had already been accessed, but there might be other approaches one can use to make Derby fail if a LOB column is accessed twice.

d) The comment with the typo will be removed, and the code in createLOBColumTracker partly rewritten.
   We now need a functioning tracker also when locators are unsupported.

e) !connection_.autoCommit_ condition for calling checkCurrentRow
   A good further optimization. Thanks. I think we need to think about how to handle this for statements with multiple result sets though, as that might cause the autocommit to be skipped even though the connection has autocommit set.


Regarding the control question above, yes there will be multiple release mechanisms. I have thought of these names:
 * NoReleaseLOBTracker
 * SingleReleaseLOBTracker
 * BatchReleaseLOBTracker
 * A tracker using piggybacking.

Originally I introduced an interface and created different implementations for the tracker to avoid maintaining state I didn't need. Now that we plan to use the tracker to also track accesses to LOB columns, we need some common code in all of the implementations.
Would it perhaps be better to have only one tracker implementation and instead implement multiple "releasers"?
I think the tracker should be able to determine the proper release mechanism itself in the constructor.

Any other ideas?
If not, I will implement a first version of the approach I have described and post a patch for review later today.

[Knut Anders Hatlen]

b) You're of course right, I had missed that. No need to set it to true, except perhaps for readability. The rename sounds like a good idea.

c) It's probably not necessary to throw the assert or an exception, since Arrays.binarySearch() returns a negative value if it cannot be found, and then we'll get an ArrayIndexOutOfBoundsException immediately when we update the array, so the error won't go unnoticed. By the way, is markAccessed a better name than noRelease for this method?

e) Such an optimization could wait if it is too much work. Seems like the easiest way to get this information is to factor out some of the logic from Statement.resultSetCommitting().

Using a single shared tracker implementation sounds good. My concern is that we add the BatchReleaseLOBTracker only for the 10.4 release, and get some code that we need to maintain forever for backward compatibility (like the stored procedure to release a range of LOBs) although it's only needed for a single minor release. Perhaps, if the plan is to implement the piggybacking for 10.5, it would be a better long-term solution to skip the BatchReleaseLOBTracker and let 10.4 use the less efficient SingleReleaseLOBTracker?

[Kristian Waagan]

'derby-3571-2a-simple_release.diff' deprecates patch revision 1.

The patch implements only the simple release mechanism (one locator at a time). Thanks to Knut Anders for commenting on the compatibility issues.
I have addressed comment (c), but chose to skip/postpone item (e), and added some simple tests. The tests are mainly for checking that the code works to some degree, it doesn't check that the code works optimally or entirely correct.

I have also added a workaround in checkCurrentRow for something that is really a problem in ResultSet.
I am not easily able to determine if the current row has changed on all the navigational methods, and checkCurrentRow will fail if called twice on the same row (the locators are already released). I added lastSeenLocator, and upon release time the current locator is checked with the last one seen for the column. If they are equal, checkCurrentRow returns immediately.

I looked at using information in ResultSet to avoid calling the method multiple times on the same row, but couldn't find a solution quickly. If you know about one, let me know :)
As an example of how to provoke the problem, consider calling rs.absolute(X) twice or rs.moveToCurrentRow() twice.

I have run derbynet, jdbc4 and jdbcapi without errors. I'm currently running suites.All and derbyall.

It is my opinion that this patch may change the premises for DERBY-2892 and possibly DERBY-3583. It would therefore be nice if we could get this patch in as soon as possible.

Please review.

[Knut Anders Hatlen]

I've tried out the 2a patch and it seems to work as intended. My only nits are:

  - LOBStateTracker.checkCurrentRow(): couldn't Arrays.fill() be moved inside the if block?
  - should discardState() and markAccessed() check the release flag?
  - should ResultSet.createLOBColumnTracker() use LOBStateTracker.NO_OP_TRACKER instead of allocating a new when serverSupportsLocators() returns false?

[Kristian Waagan]

Committed patch 2a to trunk with revision 643819. I'll wait a little before I backport to 10.4.

Thanks for the continued review Knut Anders.
The answer to your latest questions are either no or yes to all. It depends on whether the tracker shall be used to track LOB accesses to allow the client to throw an exception if a LOB column is accessed more than once.
If we don't want to track accesses, all your suggestions are valid.

I don't think we have reached consensus on what to do. Related issues are DERBY-3583 and DERBY-2892.
People should comment on the former if they have opinions on whether or not to allow multiple calls to the various getter methods for LOB columns (except for those returning a stream).


I'm not resolving the issue, because I believe there might be some follow-up changes.
Also, can anyone confirm that the new test can be run on J2ME platforms?

[Kristian Waagan]

I have seen derbyall/derbylang/subquery.diff fail a few times with the patch. It happens only on Java SE 6, and when it is run as part of derbyall. So far I haven't been able to reproduce running the test individually.
The first part of the diff looks like this:
< Hash Join ResultSet:
1431a1431
> Nested Loop Join ResultSet:
1544 del
< Hash Table ResultSet (13):
1544a1544
> Project-Restrict ResultSet (13):

[Thomas Nielsen]

The diffs you see IMHO look like the optimizer is choosing different solutions for the subquery flattening?

[Kathey Marsden]

It seems highly unlikely that this client only patch could have caused such a failure. Did you perhaps recently change your jdk version and this is what is causing the issue.

[Kristian Waagan]

Backported the fix (trunk revision 643819) to the 10.4 branch with revision 645438.
I will file another Jira for the comments Knut Anders posted, as they should be addressed if we decide not to throw exceptions for accessing a LOB column multiple times (in the non-stream cases).

[Kristian Waagan]

Filed DERBY-3601 for the clean up actions.
I will close this issue shortly after the test results are in. Problems / bugs with the implementation should be filed as separate issues.

[Kristian Waagan]

Thanks Kathey for backporting the fix(es) to 10.3.
No problems with the changes have been identified, I'm closing the issue.