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 needed now use the
isAutoCommitRequiredMethod()
-removed
void flowAutoCommitIfNotAutoCommitted()
It has been replaced with
if (isAutoCommitRequired())
resultSetCommitting()
If may make sense to put this method back and change its contents as the above
methods are almost always called one after the other.
-removed flowAutoCommitIfLastOpenResultSetWasJustClosed
replaced with
if (isAutoCommitRequired())
resultSetCommitting()
-add
public boolean isAutoCommitted()
For debug purposes
