Re: Derby-213 Early Patch
It says in section 15.2.2 of the document says: "next() — moves the cursor forward one row. Returns true if the cursor is now positioned on a row and false if the cursor is positioned after the last row." I guess the way to reconcile both this statement and the item in section 9.1 is for the next method to return false when closed. This is a novel approach but I think it would require changes in both embedded and client. It also would not necessarily fix the the SQuirreL issue I've alluded to before so other method would need to be investigated as well. I will investigate StmtCloseFunTest.java further. Philip Dan Debrunner wrote: Philip Wilder wrote: In regards to your last challenge, I am inclined to agree with your position. If we consistently follow the specs over the javadocs that is one less point for confusion. However as Kathey points out, this may mean that there are "bugs" in the javadocs. If we adopt this philosophy then we can simplify the advanced cases I specified in my auto-commit document. One item I would really like to see in the JDBC 4.0 spec is changing this item found in section 9.1 (in relation to closing ResultSets): "if the cursor is forward only, then when invocation of its next method returns false" to this "if the cursor is forward only, then when invocation of its next method returns false. Further calls to next method on this ResultSet will throw an Exception." if this is what is truly meant. If this is the case then Embedded is not functioning to spec and Client (at least in part) is doing what it is suppose to (from this it follows that SQuirreL is also not following the spec). If not, the reverse is true. I don't think the JDBC spec (4.0 section 9.1, or 3.0 section 10.1) needs to be changed as you describe. The comment "if the cursor is forward only, then when invocation of its next method returns false" is describing when a ResultSet is closed. The question then becomes what should the behaviour of a ResultSet be if it is closed? For Statement and Connection objects is it required that most methods throw exceptions if they are closed. I'm not sure where this is defined, but I know Cloudscape followed it once we started running the JDBC portion of the J2EE TCK. See StmtCloseFunTest.java Dan.
Re: DERBY-213 Early Patch
Philip Wilder wrote: > In regards to your last challenge, I am inclined to agree with your > position. If we consistently follow the specs over the javadocs that is > one less point for confusion. However as Kathey points out, this may > mean that there are "bugs" in the javadocs. If we adopt this philosophy > then we can simplify the advanced cases I specified in my auto-commit > document. One item I would really like to see in the JDBC 4.0 spec is > changing this item found in section 9.1 (in relation to closing > ResultSets): > "if the cursor is forward only, then when invocation of its next method > returns false" > to this > "if the cursor is forward only, then when invocation of its next method > returns false. Further calls to next method on this ResultSet will throw > an Exception." > if this is what is truly meant. If this is the case then Embedded is not > functioning to spec and Client (at least in part) is doing what it is > suppose to (from this it follows that SQuirreL is also not following the > spec). If not, the reverse is true. I don't think the JDBC spec (4.0 section 9.1, or 3.0 section 10.1) needs to be changed as you describe. The comment "if the cursor is forward only, then when invocation of its next method returns false" is describing when a ResultSet is closed. The question then becomes what should the behaviour of a ResultSet be if it is closed? For Statement and Connection objects is it required that most methods throw exceptions if they are closed. I'm not sure where this is defined, but I know Cloudscape followed it once we started running the JDBC portion of the J2EE TCK. See StmtCloseFunTest.java Dan.
Re: DERBY-213 Early Patch
Thanks for the tips Dan. Attached to this list is a text file outlining some of the more important points of this patch. Also in case an executive summary would be useful to anyone there are a few of main points that I tried to address with this patch (in order of importance): - Remove the call to closeX() in the nextX() method: This is probably the reason SQuirreL isn't working (See derby and squirrel sql JDBC client in the derby-user mailing list). - Change the auto-commit criteria: We only want to auto-commit if there are no other open ResultSets associated with the Statement for which the current ResultSet is attached to. For example we only wish to auto-commit ResultSet SA sttached to statement S only if ResultSets SB and SC are already closed. - Properly marking auto-committed ResultSets: I created a markAutoCommitted() method in Connection which will mark *all* open ResultSets as auto-committed post auto-commit. In terms of resolution, regrettably we have not. I acknowledge that my work on this patch is tentative and subject to change. However, hopefully my work here should make the final patch easier when we have achieved the resolution we are after. In regards to your last challenge, I am inclined to agree with your position. If we consistently follow the specs over the javadocs that is one less point for confusion. However as Kathey points out, this may mean that there are "bugs" in the javadocs. If we adopt this philosophy then we can simplify the advanced cases I specified in my auto-commit document. One item I would really like to see in the JDBC 4.0 spec is changing this item found in section 9.1 (in relation to closing ResultSets): "if the cursor is forward only, then when invocation of its next method returns false" to this "if the cursor is forward only, then when invocation of its next method returns false. Further calls to next method on this ResultSet will throw an Exception." if this is what is truly meant. If this is the case then Embedded is not functioning to spec and Client (at least in part) is doing what it is suppose to (from this it follows that SQuirreL is also not following the spec). If not, the reverse is true. Let me know if anything I said here required clarification. Philip Dan Debrunner wrote: Philip Wilder wrote: I've attached an "alpha" patch for the DERBY-213 for anyone who is interested in the DERBY-213 issue. It should be noted that the patch is contingent upon our current interpretation of the JDBC specifications/documentation. Did we come to a resolution on your document? You asked for challenges and I challenged, but I didn't see any follow up. With a patch like this it is very useful to give an overview of what you are changing, the committers (or any other reviewers) are not mind-readers and it's hard to tell if you are fixing everything to match what is in your current interpretation or just making progress in that direction. Partial fixes are good progress, but it's useful when reviewing them to understand that the patch is only intended to address some of the issues. There have been threads on what makes a patch really useful and likely to be committed quickly, I guess these tips haven't made it to the web-site yet. :-( Dan. ## Connection ## - Change public void flowAutoCommit to public boolean flowAutoCommit In my Statement.resultSetCommitting() method I only wish to to mark the ResultSets as autoComitted if they Commit occurred properly. The boolean return value allows me to establish this. - Add public void markAutoComitted() Whenever an auto-commit is done all ResultSets associated with this Connection are auto-committed. This method will mark all ResultSets as associated with this connection as auto-committed. # Statement # - Change Criteria for what requires an auto-commit pre patch in the case of multiple ResultSets a commit was required if any of the ResultSets were not auto-committed. The isAutoCommitRequired() method checks to see if the last ResultSet being closed requires an auto-commit and that there are no other open ResultSets associated with this statement. - Change Allow the flowCloseRetrievedResultSets method to invoke an auto-commit when needed. (Change (read|write)CloseResultSets(numberOfResultSetsToClose, false) to (read|write)CloseResultSets(numberOfResultSetsToClose, true) ) -add public void resultSetCommitting() Method to auto-commit the connection -add public void markAutoCommitted() Method to mark all associated ResultSets as committed -add isAutoCommitRequired() Method to check to see if a commit is required as per the new auto-commit criteria. # ResultSet # -remove Reference to closeX() in the ResultSet We'll let auto-commit handle the closing in this instance. -change Many of the "ifAutoCommitted" method names were changed to enhance clarity. -change Checks to see if an auto-commit is neede
Re: DERBY-213 Early patch
Philip Wilder wrote: > I've attached an "alpha" patch for the DERBY-213 for anyone who is > interested in the DERBY-213 issue. It should be noted that the patch is > contingent upon our current interpretation of the JDBC > specifications/documentation. Did we come to a resolution on your document? You asked for challenges and I challenged, but I didn't see any follow up. With a patch like this it is very useful to give an overview of what you are changing, the committers (or any other reviewers) are not mind-readers and it's hard to tell if you are fixing everything to match what is in your current interpretation or just making progress in that direction. Partial fixes are good progress, but it's useful when reviewing them to understand that the patch is only intended to address some of the issues. There have been threads on what makes a patch really useful and likely to be committed quickly, I guess these tips haven't made it to the web-site yet. :-( Dan.
DERBY-213 Early patch
I've attached an "alpha" patch for the DERBY-213 for anyone who is
interested in the DERBY-213 issue. It should be noted that the patch is
contingent upon our current interpretation of the JDBC
specifications/documentation. This patch is meant to address both the
early close problem recently discussed in Derby-user and incorrect
auto-commit behavior for multiple ResultSets. It is *not* meant to
address the issue of DatabaseMetaData ResultSets invoking an
auto-commit. I may file this as a seperate issue in the future. I call
this patch an alpha patch because there is one known bug associated with
it and potentially other unspotted ones. The bug in question occurs in
the lang/updatableResultSet.java test
specifically a call to:
System.out.println("Have this call to rs.rowDeleted() just to make sure
the method does always return false? " + rs.rowDeleted());
rs.deleteRow(); //This line, approximately line 896 of the
test file.
System.out.println("Have this call to rs.rowDeleted() just to make
sure the method does always return false? " + rs.rowDeleted());
I will continue my attempts to establish why this is a bug but as always
input is appreciated.
Philip
Index: am/Connection.java
===
--- am/Connection.java (revision 191177)
+++ am/Connection.java (working copy)
@@ -515,10 +515,12 @@
}
// precondition: autoCommit_ is true
-public void flowAutoCommit() throws SqlException {
+public boolean flowAutoCommit() throws SqlException {
if (willAutoCommitGenerateFlow()) {
flowCommit();
+return true;
}
+return false;
}
public boolean willAutoCommitGenerateFlow() throws
org.apache.derby.client.am.SqlException {
@@ -1638,4 +1640,10 @@
inUnitOfWork_ = inUnitOfWork;
}
+public void markAutoCommitted() {
+java.util.ListIterator iter = openStatements_.listIterator();
+while(iter.hasNext()) {
+((Statement)iter.next()).markAutoCommitted();
+}
+}
}
Index: am/Statement.java
===
--- am/Statement.java (revision 191177)
+++ am/Statement.java (working copy)
@@ -459,6 +459,11 @@
} else {
flowCloseOutsideUOW();
}
+
+//The connection will not auto commit on close in the case of
+//multiple open resultsets. This call is here to insure proper
+//autocommit.
+connection_.flowAutoCommit();
} finally {
markClosed();
connection_.openStatements_.remove(this);
@@ -1240,33 +1245,29 @@
//
// This is fixed by passing a flag to our statement close processing that
prevents
// driving additional auto-commits after each statement close.
-// Connectino close drives its own final auto-commit.
+// Connection close drives its own final auto-commit.
//
boolean writeCloseResultSets(int number, boolean allowAutoCommits) throws
SqlException {
-boolean requiresAutocommit = false;
+ResultSet crs = resultSet_;
if (resultSetList_ != null) {
+//TODO Potential for ArrayIndexOutOfBoundsException. Fix?
for (int i = 0; i < number; i++) {
if (resultSetList_[i] != null) {
if (resultSetList_[i].openOnServer_) {
resultSetList_[i].writeClose();
}
-if (!resultSetList_[i].autoCommitted_ && allowAutoCommits)
{
-requiresAutocommit = true;
-}
}
}
+crs = resultSetList_[number - 1];
} else if (generatedKeysResultSet_ != null &&
generatedKeysResultSet_.openOnServer_) {
generatedKeysResultSet_.writeClose();
} else if (resultSet_ != null) {
if (resultSet_.openOnServer_) {
resultSet_.writeClose();
}
-if (!resultSet_.autoCommitted_ && allowAutoCommits) {
-requiresAutocommit = true;
-}
}
-if (connection_.autoCommit_ && requiresAutocommit &&
isAutoCommittableStatement_) {
-connection_.writeAutoCommit();
+if (connection_.autoCommit_ && isAutoCommitRequired(crs) &&
isAutoCommittableStatement_) {
+resultSetCommitting(true);
if (connection_.isXAConnection_) {
return (connection_.xaState_ ==
Connection.XA_T0_NOT_ASSOCIATED) ;
} else {
@@ -1283,8 +1284,18 @@
}
void readCloseResultSets(int number, boolean allowAutoCommits) throws
SqlException {
+/* The call to isAutoCommitRequired must be done before
+ * the readClose() is done. The read close can mark ResultSets
+ * closed. If these Resultsets were open in the writeCloseResultSets
+
