Explicit commit inside a java procedure makes a dynamic result sets passed out unavailable

• Key: DERBY-3304
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Mamta A. Satoor
• Reporter: Daniel John Debrunner
• Component/s: JDBC
• Affects Version/s: 10.4.1.3
• Fix Version/s: 10.3.3.0, 10.4.1.3

Description

Repro (Main.java) that shows changed behavior after svn 602991
(the patch committed for this issue). It seems a regression: (originally from Dag H. Wanvik attached to DERBY-1585)

An explicit commit inside a stored procedure makes a dynamic result sets passed out unavailable, even if the commit is executed *prior* to the result set; as in the repro.

Comments

[Daniel John Debrunner]

Dag H. Wanvik originally attached this to DERBY-1585

[Daniel John Debrunner]

I can't reproduce this problem, using Main.java I get::

$ java -cp classes commitInProc.Main
* Testing with org.apache.derby.jdbc.EmbeddedDriver
Got: APP

which shows that the ResultSet is returned correctly. I was using IBM's 1.5 jvm.

[Dag H. Wanvik]

Interesting. It seems to have been fixed as a side effect of 606106 which is for
DERBY-3037 (my sandbox wasn't up to date).
A comment there contains this piece of info which may be relevant:

> In order to achieve that, I have added following code in NoRowsResultSetImpl.close
> to take care of the activation
> + if (activation.isSingleExecution())
> + activation.close();

[Daniel John Debrunner]

Not sure how that code could be related, I modified the repro to use a PreparedStatement and continued to see the correct behaviour.

[Dag H. Wanvik]

My preliminary analysis shows that the added code in svn 606106 does
indeed make the difference, but I am not convinced all is kosher, see
below.

When the stored procedure (f2) commits, it leads to a call of
GenericLanguageConnectionContext#resetActivations, which in turn leads
to a call to CallStatementResultSet#close. At this point the dynamic
result set has not yet been constructed.

Here is the call stack (line numbers correspond to svn 606106).
Interestingly, CallStatementResultSet#close is called as part of
CallStatementResultSet#open... Is this correct?

   org.apache.derby.impl.sql.execute.NoRowsResultSetImpl.close(NoRowsResultSetImpl.java:393)
   org.apache.derby.impl.sql.execute.CallStatementResultSet.close(CallStatementResultSet.java:108)
   org.apache.derby.impl.sql.execute.BaseActivation.reset(BaseActivation.java:312)
   org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.resetActivations(GenericLanguageConnectionContext.java:2764)
   org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.doCommit(GenericLanguageConnectionContext.java:1113)
   org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.userCommit(GenericLanguageConnectionContext.java:991)
   org.apache.derby.impl.jdbc.TransactionResourceImpl.commit(TransactionResourceImpl.java:237)
   org.apache.derby.impl.jdbc.EmbedConnection.commit(EmbedConnection.java:1202)
   commitInProc.Main.f2(Main.java:95)
   org.apache.derby.exe.ac601a400fx0117x605dx1c0ex0000003d76c00.g0
   sun.reflect.NativeMethodAccessorImpl.invoke0(NativeMethodAccessorImpl.java)
   sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
   sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
   java.lang.reflect.Method.invoke(Method.java:597)
   org.apache.derby.impl.services.reflect.ReflectMethod.invoke(ReflectMethod.java:46)
   org.apache.derby.impl.sql.execute.CallStatementResultSet.open(CallStatementResultSet.java:74)
   org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:370)
   org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1234)
   org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:624)
   org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:556)
   commitInProc.Main.doSingleDriver(Main.java:58)
   commitInProc.Main.main(Main.java:83)


Now, this close leads to *another* recursive call to
CallStatementResultSet.close, but this time isOpen is false, so
NoRowsResultSetImpl.close returns immediately. At this point we have
CallStatementResultSet#open, #close and #close(frame 2!) on the stack.
   
   org.apache.derby.impl.sql.execute.NoRowsResultSetImpl.close(NoRowsResultSetImpl.java:334)
   org.apache.derby.impl.sql.execute.CallStatementResultSet.close(CallStatementResultSet.java:108)
   org.apache.derby.impl.sql.execute.BaseActivation.reset(BaseActivation.java:312)
   org.apache.derby.impl.sql.execute.BaseActivation.close(BaseActivation.java:346)
   org.apache.derby.impl.sql.execute.NoRowsResultSetImpl.close(NoRowsResultSetImpl.java:396)
   org.apache.derby.impl.sql.execute.CallStatementResultSet.close(CallStatementResultSet.java:108)
   org.apache.derby.impl.sql.execute.BaseActivation.reset(BaseActivation.java:312)
   org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.resetActivations(GenericLanguageConnectionContext.java:2764)
   org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.doCommit(GenericLanguageConnectionContext.java:1113)
   org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.userCommit(GenericLanguageConnectionContext.java:991)
   org.apache.derby.impl.jdbc.TransactionResourceImpl.commit(TransactionResourceImpl.java:237)
   org.apache.derby.impl.jdbc.EmbedConnection.commit(EmbedConnection.java:1202)
   commitInProc.Main.f2(Main.java:95)
   org.apache.derby.exe.ac601a400fx0117x605dx1c0ex0000003d76c00.g0
   sun.reflect.NativeMethodAccessorImpl.invoke0(NativeMethodAccessorImpl.java)
   sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
   sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
   java.lang.reflect.Method.invoke(Method.java:597)
   org.apache.derby.impl.services.reflect.ReflectMethod.invoke(ReflectMethod.java:46)
   org.apache.derby.impl.sql.execute.CallStatementResultSet.open(CallStatementResultSet.java:74)
   org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:370)
   org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1234)
   org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:624)
   org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:556)
   commitInProc.Main.doSingleDriver(Main.java:58)
    commitInProc.Main.main(Main.java:83)

No dynamic result set is closed here since none has yet been
constructed. Anyway, next, as part of the commit call chain, the
activation of the call statement is also closed.

Now, *before* svn 606106, the activation for the call statement is
closed later; as shown here (numbers correspond to svn 606105):
       
   org.apache.derby.impl.sql.execute.CallStatementResultSet.close(CallStatementResultSet.java:149)
   org.apache.derby.impl.sql.execute.BaseActivation.reset(BaseActivation.java:312)
   org.apache.derby.impl.sql.execute.BaseActivation.close(BaseActivation.java:346)
   org.apache.derby.impl.sql.GenericActivationHolder.close(GenericActivationHolder.java:463)
   org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:397)
   org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1234)
   org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:624)
   org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:556)
   commitInProc.Main.doSingleDriver(Main.java:58)
   commitInProc.Main.main(Main.java:83)

At line 149 of CallStatementResultSet, the dynamic result set is
closed and is thus unavailable.

But after *606106*, the activation has already been closed *before*
the dynamic result set is assigned, so the dynamic result set is not
closed as part of closing the activation when the call to f2 finishes
and is thus available!

Not sure what to make of all this, but it seems weird that the commit
should close the CallStatementResultSet when is in the process of
being opened. Also, the fact that the commit closes the activation of
the call statement looks wrong(?).

[Daniel John Debrunner]

Nice findings!

> Interestingly, CallStatementResultSet#close is called as part of
> CallStatementResultSet#open... Is this correct?

 I think it is confusing. Language result sets that do not return rows tend to do this because all of their work is performed in the open(), thus to free up any resources their open() call also calls close(). Now since the close will be called from some external source anyway, I think it would be cleaner if the open-close cycle was driven from the outside and not from within the open.

From what you have found, it looks like a CallStatementResultSet should always be held across commits, though since that logic is applied to result sets that return rows, I don't know what real effect it would have here.

[Daniel John Debrunner]

Since holdability is at the activation, not the result set, I wonder if this would make sense in CallStatementResultSet's constructor?

  a.setResultSetHoldability(true);

[Dag H. Wanvik]

Tried this, but it is not sufficient, it seems..
BaseActivation.reset will close the result set even if holdability is true, cf. this line:

if (!resultSetHoldability || !resultSet.returnsRows()) {
// would really like to check if it is open,
// this is as close as we can approximate that.
resultSet.close();

Since CallStatementResultSet extends NoRowsResultSetImpl, the call to resultSet.returnsRows() returns false, so the second condition holds, and close ensues. NoRowsResultSetImpl#returnsRows is final so CallStatementResultSet can't really override it. I tried it though, by removing the final, but then I get an assert error since EmbedStatement#executeStatement, which also calls ResultSet#returnsRows will now try to create a result set (line 1249 in EmbedStatement.java). Which is reasonable ;-)
Maybe the test in BaseActivation.reset can test for CallStatementResultSet? Not OO, though..Thinking aloud, maybe a new interface method of ResultSet: canReturnDynamicResultSet?

[Mamta A. Satoor]

i will investigate more into this.

[Mamta A. Satoor]

Following seems to say that CallStatementResultSet#open makes a direct call to CallStatementResultSet#close but that is not true. When I check the code for CallStatementResultSet#open, I do not see a call to close method. The call to close from open does happen in MiscResultSet#open but that ResultSet does not come into play when dealing with CallStatementResultSet.

****************************
> Interestingly, CallStatementResultSet#close is called as part of
> CallStatementResultSet#open... Is this correct?

 I think it is confusing. Language result sets that do not return rows tend to do this because all of their work is performed in the open(), thus to free up any resources their open() call also calls close(). Now since the close will be called from some external source anyway, I think it would be cleaner if the open-close cycle was driven from the outside and not from within the open.
*****************************

[Mamta A. Satoor]

I added a junit test case for the test case provided by Dag to this Jira entry. The changes went into the trunk as part of revision 614292. The commit comments were as follows

Adding a junit test for the standalone test case provided by Dag for DERBY-3304. Here, we are adding a Java procedure which does a commit and then returns a resultset back to the caller. The resultset should not get closed as part of the commit.

[Dag H. Wanvik]

I meant only that CallStatementResultSet#close is called (indirectly) from CallStatementResultSet#open (see stack trace in https://issues.apache.org/jira/browse/DERBY-3304?focusedCommentId=12557476#action_12557476).

[Mamta A. Satoor]

Sorry for misinterpreting your comment, Dag. You are right, in this specific test case, where there is a commit inside the java stored procedure, close is getting called on CallStatementResultSet which is just getting opened. I think work on DERBY-3037 will help resolve this issue too. In DERBY-3037, we have an example of Java Stored routine which is a function, which also does a commit inside it. And that commit causes the resultset that will be returned by the function to close. I am hoping to get some pointers on how to recognize which resultsets should be closed and which should be left often.

[Mamta A. Satoor]

Based on the research/comments on this issue and Dan's comment in DERBY-3037 "What is the purpose of commit() closing the language result sets? If it's to close JDBC ResultSets that are marked close at commit then a possibility is to only close language result sets that return rows (returnRows() returns true). That would leave language result sets that do not return rows open, but by definition (I think) those are the ones that are actively executing and the very ones we don't want to close. :-) ", I thought of keeping the language resultsets that do not return rows open when BaseActivation.reset() is trying to decide what resultsets to close. This will ensure that we do not close the CallStatementResultSet(since it does not return rows) while it is still being constructed when the java procedure issues commit/rollback. The change involved is as follows
$ svn stat -q
M java\engine\org\apache\derby\impl\sql\execute\BaseActivation.java
$ svn diff
Index: java/engine/org/apache/derby/impl/sql/execute/BaseActivation.java
===================================================================
--- java/engine/org/apache/derby/impl/sql/execute/BaseActivation.java (revision 617013)
+++ java/engine/org/apache/derby/impl/sql/execute/BaseActivation.java (working copy)
@@ -334,7 +334,8 @@
        {
                // if resultset holdability after commit is false, close it
                if (resultSet != null) {
- if (!resultSetHoldability || !resultSet.returnsRows()) {

+ //Do not close resultsets that do not return any rows.
+ if ((!resultSetHoldability && resultSet.returnsRows()==true)){
                                // would really like to check if it is open,
                                // this is as close as we can approximate that.
                                resultSet.close();

So, as can be seen from diff above, rather than always closing the resultsets that do not return rows, we want to leave them alone.

This works good for CallStatementResultSet but this is causing a problem for DML like update for instance below.

The following ij script demonstrates the problem with the patch. The script creates 2 tables with foreign key relationship. Then it tries to violate the foreign key by doing an update. With my changes, Derby fails to generate any statistics for update. This is because the statistics collection happens in the language resultset close method but since with my patch, we do not close the resultset if it does not return row, we do not get a chance to collect the statistics. Without my patch, the statistics get collected in language resultset close which is called by BaseActivation.reset

connect 'jdbc:derby:c:/dellater/db;create=true';
drop table basic;
drop table p;
create table p (ccharForBitData char(1) for bit data not null, unindexed smallint, cchar char(10) not null,
constraint pk1 primary key (cchar, ccharForBitData));
insert into p values (x'22', 33, '22');
create table basic (cint int, cchar char(10), ccharForBitData char(1) for bit data, unindexed int);
insert into basic values (22, '22', x'22', 22);
alter table basic add constraint fk1 foreign key (cchar, ccharForBitData) references p;
maximumdisplaywidth 2000;
call SYSCS_UTIL.SYSCS_SET_RUNTIMESTATISTICS(1);
update p set ccharForBitData = x'22', cchar = CAST(unindexed as CHAR(10));
values SYSCS_UTIL.SYSCS_GET_RUNTIMESTATISTICS();

To avoid this statistics collection problem, we could change language resultset close criteria in BaseActivation.reset to follow Dag's suggestion "Maybe the test in BaseActivation.reset can test for CallStatementResultSet?' but like Dag said, "Not OO, though". Will like to hear if anyone has any comments?

[Daniel John Debrunner]

+ //Do not close resultsets that do not return any rows.
+ if ((!resultSetHoldability && resultSet.returnsRows()==true)){

Minor comment, but I think this code would be a lot easier for future readers to understand if the "double negative" comment was switched into a positive comment.
I.e. the comment should state what the desired behaviour is, not what you don't want to happen.
Then switching the order of the tests around, would make the code more naturally follow the comment.

  // Close result sets that return rows and are not held across commit. This is to implement
  // closing JDBC result sets that are CLOSE_CURSOR_ON_COMMIT at commit time.
  if (resultSet.returnsRows() && !resultSetHoldability)

[Daniel John Debrunner]

I think the issue with the statistics is that functionality is being implemented in the incorrect location.

Activation.reset() states that its role is to reset the activation for future executions. Using Activation.reset() to implement closing open JDBC result sets on a commit() just seems the wrong approach. Having an explicit method for commit that performed the close of a result set as required would be cleaner. Then Activation.reset() could have a clear purpose.

Longer term the fix to DERBY-2485 would address this as then a result set that needed to be closed upon commit would simply register itself with the transaction manager such that it received notification of a commit.

[Mamta A. Satoor]

I agree that Activation.reset should be able to just close the resultset associated with it without having to look at holdability or returnsRows() method. This functionality of looking at holdability and returnsRows should be implemented separately. I will work on cleaning up code for Activation.reset and putting the functionality required by commit in the appropriate method.

[Mamta A. Satoor]

Committed changes into trunk with revision 618788. The commit comments were as follows

***********************************************
DERBY-3304

This commit addresses two issues. First of all, it cleanups up reset method in BaseActivation which was doing more than just bringing the Activation back to pre-execution state. The method had to make itself aware of holdability and what kind of resultset it was dealing with before closing or clearing the row of the resultset. The reason for this behavior is commit code path was relying on Activation.reset to do more than just bringing the activation to pre-execution state. I fixed this by moving this code from BaseActivation.reset to GenericLanguageConnectionContext.resetActivations.

Additionally, in the new code in GenericLanguageConnectionContext.resetActivations, I added the code to not close the language result sets associated with activations that do not return rows even if activation may have holdability set to false. This will ensure that a commit inside a java procedure will not inadvertantly close the resultset associated with the java procedure call.
***********************************************

I ran the Junit tests and derbyall on my Windows XP machine with jdk1.6 and saw no new failures.

As part of my committing the changes for this jira entry, I copied some of the cleanup work(as shown below) from BaseActivation.reset into new code in GenericLanguageConnectionContext.resetActivations
a.clearHeapConglomerateController();
if (!a.isSingleExecution())
a.clearWarnings();

This code was always getting executed at the time of commit before my commit and because of that, I decided to copy it in GenericLanguageConnectionContext.resetActivations If anyone has any comments on this, please let me know.

[Mamta A. Satoor]

I will work on migrating this change into 10.3 codeline

[Daniel John Debrunner]

I think that revision 618788 makes this code in resetActivations() unnecessary. It was used to force held cursors into non-held state to be closed on rollback() in Activation.reset(), since reset() used to check the holdability. Your change is good in that it removes this link between two methods that should be independent of each other (ie. resetActivations() should not have had internal knowledge of what reset() was doing). :-)

/*
** andClose true means we are here for rollback.
** In case of rollback, we don't care for holding
** cursors and that's why I am resetting holdability
** to false for all activations just before rollback
*/
if (andClose)
a.setResultSetHoldability(false);


Now (and to some extent before the change) the javadoc for resetActivations() is incorrect, in addition the method name is misleading. It would be good to clean this up.

[Daniel John Debrunner]

Even the 'andClose' parameter of resetActivations is misleading, it means forRollback (I think this was true before 618788)

[Mamta A. Satoor]

The commit comments for 618788 did not include the bit about why I added following to GenericLanguageConnectionContext.resetActivations
a.clearHeapConglomerateController();
if (!a.isSingleExecution())
a.clearWarnings();

Because of this, I (Andrew changed it for me since I kept getting errors while using svn propedit --revprop -r 618788 svn:log) changed the commit comments for 618788 to read as follows

*************************start of new commit comments associated with revision 618788*****************************
This commit addresses two issues.

First of all, it cleanups up reset method in BaseActivation which was doing more than just bringing the Activation back to pre-execution state. The method had to make itself aware of holdability and what kind of resultset it was dealing with before closing or clearing the row of the resultset. The reason for this behavior is commit code path was relying on Activation.reset to do more than just bringing the activation to pre-execution state. I fixed this by moving this code from BaseActivation.reset to GenericLanguageConnectionContext.resetActivations.

Additionally, in the new code in GenericLanguageConnectionContext.resetActivations, I added the code to not close the language result sets associated with activations that do not return rows even if activation may have holdability set to false. This will ensure that a commit inside a java procedure will not inadvertantly close the resultset associated with the java procedure call. Additionally, I copied some of the cleanup work(as shown below) from BaseActivation.reset into new code in GenericLanguageConnectionContext.resetActivations
   a.clearHeapConglomerateController();
   if (!a.isSingleExecution())
      a.clearWarnings();

This code above was always getting executed at the time of commit before my commit and because of that, I decided to copy it in GenericLanguageConnectionContext.resetActivations. If anyone has any comments on this, please let me know.
*************************end of new commit comments associated with revision 618788*****************************

[Mamta A. Satoor]

I need to admit that I do not completely understand why BaseActivation.reset() has following code
updateHeapCC = null;
// REMIND: do we need to get them to stop input as well?

if (!isSingleExecution())
clearWarnings();

But since this piece of code was getting exectued for an activation at the time of commit, I decided to copy similar code in GenericLanguageConnectionContext.resetActivations since a commit code path now is not going to goto BaseActivation.reset().

Would appreciate if someone knows more about this code and can share if this piece of code is needed at the time of commit or not. I *quickly* looked at JDBC Connection,commit api and did not see anything about clearing the warnings at the time of commit. So, maybe we do not need to clear the warnings in GenericLanguageConnectionContext.resetActivations when a commit is executed. I am even more unclear about the purpose of a.clearHeapConglomerateController(); at the time of commit. But I copied it just to be on the safe side so that we execute the same set of code as before my checkin.

Does anyone have any insight into whether or not this code is needed at commit time?

[Mamta A. Satoor]

Since we do not reset the activations at the time of commit, the name of the method, resetActivations() in GenericLanguageConnectionContext is misleading. I am not too creative with names but I am thinking of changing the name resetActivations to activationRelatedCleanup. If anyone has any other suggestion for the name change, please let me know.

In addition, i am changing the javadoc for resetActivations to reflect what it is really doing. The new javadoc comments that I am planning to have are as follows
/**
If we are called as part of rollback code path, then we will reset all
the activations.

If we are called as part of commit code path, then we will do one of
the following if the activation has resultset assoicated with it
1)Close result sets that return rows and are not held across commit.
2)Clear the current row of the resultsets that return rows and are
held across commit.
3)Leave the result sets untouched if they do not return rows

Additionally, clean up (close()) activations that have been
marked as unused during statement finalization.

@exception StandardException thrown on failure
*/

In addition, I am checking to see if following code can be removed from resetActivations
/*
** andClose true means we are here for rollback.
** In case of rollback, we don't care for holding
** cursors and that's why I am resetting holdability
** to false for all activations just before rollback
*/
if (andClose)
a.setResultSetHoldability(false);

[Daniel John Debrunner]

mamta> I am thinking of changing the name resetActivations to activationRelatedCleanup.

The purpose of the method is to handle (process?) activations at end transaction time, thus maybe something like:

  endTransactionActivationHandling

[Daniel John Debrunner]

Thanks for the info about the moved code, I missed it in the jira comment.

I think the clearing of warnings on commit can go, as you say it's not required for a commit. I looked at it and think it has the potential to incorrectly clear warnings so that the application never sees them. I don't think there's a code path today that is subject to the bug, but one could appear in the future. So I'd say remove the clearing of warnings since it's not required and a no-op at the moment.

For the update conglomerate clearing, I think that's code that is in the wrong location, probably should be in the result set that set that field. That's probably a separate issue.

[Mamta A. Satoor]

Dan, thanks for your feedback.

I will work on renaming the method and removing the clear warning code from the commit path.

What I am not sure about is update conglomerate clearing. You suggested that it can be taken up as a separate issue. Does that mean that for now(until we address that new jira entry), we should continue to do update conglomerate clearing at the time of commit?

[Mamta A. Satoor]

After re-reading Dan's comment about update conglomerate clearing, I think Dan is suggesting that we leave the update conglomerate clearing code as it is for now and tackle that as a separate jira entry. Based on this and other comments, I have just commited changes into trunk which will take care of removing the clear warning code in the commit code path, fix the javadoc for resetActivations, rename resetActivations and remove the holdability false setting for activations at the time rollback code path. These changes went in as revision 619279. I will merge 619279 and 618788 into 10.3 codeline. I will also see if I can add some more testing for these changes.

[Mamta A. Satoor]

Created jira entry DERBY-3396(At the end transaction time(through commit/rollback), is clearing of the conglomerate (used for scans for update and delete) for an activation happening in the right place?) for the issue discovered while working on the current jira entry.

[Mamta A. Satoor]

Made some code cleanup changes into GenericLanguageConnectionContext.endTransactionActivationHandling This will make code easier to understand and maintain. The changes went as part of revision 619772 into trunk.

[Mamta A. Satoor]

Added a junit test case for number of code changes for transaction ending time that went in as part of DERBY-3304. This new test will check those code changes for specific case of rollback inside a java procedure call. The test went into trunk(revision 627673) and 10.3 codeline(revision 627674).

The test case runs in embedded mode to show the correct behavior for rollback inside a java procedure causing all the resultsets to close. The test case for now has been commented out for network server mode and it should be enabled when DERBY-3414 is fixed. The reason for network server test disability is that rollback inside the procedure does not close all the resultsets in Network server mode.

[Mamta A. Satoor]

I just finished making code changes and added a test case with revision 629926. These code changes ensure that a user initiated rollback through JDBC Connection object will not close the resultsets that do not return rows. More detailed info on the patch can be found through the commit comments for 629926.

As next 2 tasks, I will merge these changes into 10.3 codeline and I will work on the case where a rollback is caused because of an exception. In such a case, we should close all the resultsets no matter if they return rows or not.

[Knut Anders Hatlen]

I read through the commit log and found a couple of nits in commit
#629926:

I think this change in EmbedStatement could be simplified:

- if (lrs.isClosed)
- return null;
+ try {
+ //following will check if the JDBC ResultSet or the language
+ //ResultSet is closed. If yes, then it will throw an exception.
+ //So, the exception indicates that the ResultSet is closed and
+ //hence we should ignore it.
+ lrs.checkIfClosed("");
+ } catch (SQLException ex) {
+ return null;
+ }

EmbedResultSet.isClosed() does the same thing, I think, so the above
code could be written as "if (lrs.isClosed()) return null;", which
looks a bit clearer to me. (Note, it's the isClosed _method_, not the
field.)

In GenericLanguageConnectionContext.endTransactionActivationHandling()
I saw this change:

+ ResultSet activationResultSet = null;
+ boolean resultsetReturnsRows = false;
+ if (a.getResultSet() != null) {
+ activationResultSet = a.getResultSet();
+ resultsetReturnsRows = activationResultSet.returnsRows();
+ }

There's no reason to initialize activationResultSet to null first and
modify it inside the if statement. Just set it to a.getResultSet()
when it is initialized. Perhaps this would be clearer:

    final ResultSet activationResultSet = a.getResultSet();
    final boolean resultsetReturnsRows =
        (activationResultSet != null) && activationResultSet.returnsRows();

Also, since resultsetReturnsRows is always false when
activationResultSet is null, it's not necessary to check that
activationResultSet is not null before checking that
resultsetReturnsRows is true later in that method.

[Mamta A. Satoor]

Knut, I appreciate your code review. I will work on it.

[Mamta A. Satoor]

Before addressing Knut's comments, I wanted to commit changes that were already in my codeline. The changes went in as revision 631108 into trunk. These changes ensured that when an exception is raised, the rollback caused by it will cleanup CallableStatementResultset such that all the resultsets associated with it are closed and CallableStatementResultset is available for reuse during next execution. More info can be found in commit comments.

Next, I will work on migrating these changes into 10.3 codeline and addressing Knut's comments on revision 629926.

[Mamta A. Satoor]

Merged 631108 from trunk into 10.3.2.2 codeline with revision 631302.

[Mamta A. Satoor]

Knut suggested if we can use EmbedResultSet.isClosed() rather than EmbedResultSet.checkIfClosed() which requires us to catch the exception and determine if the resultset is closed. I had gone that path when I was working on the patch but EmbedResultSet.isClosed() is doing more than just checking if the JDBC resultset or language resultset is closed. It causes following additional code to execute

EmbedResultSet.isClosed() calls EmbedResultSet.checkExecIfClosed which has the following extra piece of code
        // Currently disconnected, i.e. a detached gobal transaction
        if (appConn == null)
            throw Util.noCurrentConnection();
            
        if (appConn.isClosed()) {
            closeCurrentStream();
            isClosed = true;
            throw Util.noCurrentConnection();
        }

I can't remember which test failed when I used EmbedResultSet.isClosed() rather than EmbedResultSet.checkIfClosed() but the cuplrit was the connection related code in checkExecIfClosed.

[Mamta A. Satoor]

Made some code cleanup changes based on the 2nd half of Knut's review comments for revision 629926. The code cleanup went in as revision 631481 into 10.4 codeline.

629926 and 631481 are not merged into 10.3 codeline yet because they depend on merge of DERBY-3404 into 10.3. The merge of DERBY-3404 into 10.3 is running into locking issues.

[Mamta A. Satoor]

Forgot to add a comment about 2 trivial checkins, 631307(trunk) and 631311(10.3), that I made today. These checkins fixed the comments in LangProcedureTest in both trunk and 10,3 codeline. I was referring to incorrect jira entry in the test comments when I added the tests for engine changes.

[Mamta A. Satoor]

Number of fixes have gone in this jira entry and I think I have tackled all the issues related to this jira entry through them. The only thing I am aware of is that revisions 629926 and 631481 from trunk have not been merged into 10.3 codeline because they depend on merge of fix for DERBY-3404 into 10.3 and that merge is running into locking issues.

If I have missed any other issue on this jira entry, please let me know.

[Knut Anders Hatlen]

Regarding isClosed() vs checkIfClose(), when you saw a test failing, was that before or after the fix for DERBY-3404 was checked in? I had to change a couple of tests because they would throw different exceptions with and without the fix (could change from connection closed to result set closed), so it might be that it's a similar problem here. It sounds a bit strange to me that we can return ResultSets where isClosed() returns false. But then perhaps the problem is that appConn is null when this code is executed because the ownership for the ResultSet is being transferred from the nested connection to the top-level connection?

Anyway, it's not that many code lines extra, so don't spend too much energy on it. Thanks for checking it out and addressing my other comments.

[Mamta A. Satoor]

I haven't investigated why the isClosed() not working but using isClosed() makes .jdbc4/ResultSetTest.testGetHoldability() run into null pointer exception at line 1525. This test seems to indicate that for some reason, the dynamic resultset from the Java procedure has been closed hence unavailable whereas the test expects it to be open,

[Mamta A. Satoor]

Checked in 628130, 629926 and 631481 into 10.3.2.2 codeline.

[Mamta A. Satoor]

The changes for this jira issue are in 10.4 and 10.3 codelines.