Description

The methods Connection.commit() and Connection.rollback() in the client driver cause a round-trip to the server even if the commit/rollback is unnecessary (i.e. there is nothing to commit or roll back).
Comments suggest (see below) that this can be optimized, such that the commands are flowed to the server only when required. It can be seen that this optimization has been used other places in the client driver. Never the less, it must be checked that this optimization doesn't have side-effects.

This issue came up in connection with connection pooling, where a pool implementation always issued a rollback to make sure there was no active transaction on the connection handed out.

From Connection.flowCommit:
// Per JDBC specification (see javadoc for Connection.commit()):
// "This method should be used only when auto-commit mode has been disabled."
// However, some applications do this anyway, it is harmless, so
// if they ask to commit, we could go ahead and flow a commit.
// But note that rollback() is less harmless, rollback() shouldn't be used in auto-commit mode.
// This behavior is subject to further review.

// if (!this.inUnitOfWork)
// return;
// We won't try to be "too smart", if the user requests a commit, we'll flow a commit,
// regardless of whether or not we're in a unit of work or in auto-commit mode.
//

Activity

Change the code in Connection.flowcommit() to save the round trip
if (!this.inUnitOfWork_)
return;

Run suites.AllPackages and test_04_undefinedAndIllegal and test_04_undefinedAndIllegal fails.
1) test_04_undefinedAndIllegal(org.apache.derbyTesting.functionTests.tests.lang.Bo
oleanValuesTest)junit.framework.ComparisonFailure: getBoolean() on BLOB_COL expect
ed:<...2005> but was:<...4000>
at org.apache.derbyTesting.junit.BaseJDBCTestCase.assertSQLState(BaseJDBCT
estCase.java:762)
at org.apache.derbyTesting.functionTests.tests.lang.BooleanValuesTest.vet_
getBooleanException(BooleanValuesTest.java:430)
at org.apache.derbyTesting.functionTests.tests.lang.BooleanValuesTest.vet_
getBooleanIsIllegal(BooleanValuesTest.java:410)
at org.apache.derbyTesting.functionTests.tests.lang.BooleanValuesTest.test
_04_undefinedAndIllegal(BooleanValuesTest.java:332)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.ja
va:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccesso
rImpl.java:25)
at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:10
9)
at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
at junit.extensions.TestSetup.run(TestSetup.java:23)
at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
Caused by: java.sql.SQLException: Invalid cursor state - no current row.

However, these two tests fail when I don't have Connect.flowcommit() change either.

Lily Wei
added a comment - 22/May/10 00:07 Change the code in Connection.flowcommit() to save the round trip
if (!this.inUnitOfWork_)
return;
Run suites.AllPackages and test_04_undefinedAndIllegal and test_04_undefinedAndIllegal fails.
1) test_04_undefinedAndIllegal(org.apache.derbyTesting.functionTests.tests.lang.Bo
oleanValuesTest)junit.framework.ComparisonFailure: getBoolean() on BLOB_COL expect
ed:<...2005> but was:<...4000>
at org.apache.derbyTesting.junit.BaseJDBCTestCase.assertSQLState(BaseJDBCT
estCase.java:762)
at org.apache.derbyTesting.functionTests.tests.lang.BooleanValuesTest.vet_
getBooleanException(BooleanValuesTest.java:430)
at org.apache.derbyTesting.functionTests.tests.lang.BooleanValuesTest.vet_
getBooleanIsIllegal(BooleanValuesTest.java:410)
at org.apache.derbyTesting.functionTests.tests.lang.BooleanValuesTest.test
_04_undefinedAndIllegal(BooleanValuesTest.java:332)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.ja
va:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccesso
rImpl.java:25)
at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:10
9)
at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
at junit.extensions.TestSetup.run(TestSetup.java:23)
at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
Caused by: java.sql.SQLException: Invalid cursor state - no current row.
However, these two tests fail when I don't have Connect.flowcommit() change either.
On BooleanTestsValue.java,
public void test_04_undefinedAndIllegal() throws Exception
{
Connection conn = getConnection();
vet_getBooleanIsIllegal( conn, "BLOB_COL" ); <<<====This is the failure comes from
I will keep looking to see why these two tests failed with or without Connection.flowcommit(() change.

The problems with BooleanValuesTest were fixed in DERBY-4674. You probably ran with java 1.6.0_13 after my initial check-in that disabled the test if Xalan was not available. Rick later re-enabled the test and made it run cleanly on platforms that didn't have the required XML libraries, so I believe it should be running fine now if you do an svn update first.

Knut Anders Hatlen
added a comment - 25/May/10 09:44 Hi Lily,
The problems with BooleanValuesTest were fixed in DERBY-4674 . You probably ran with java 1.6.0_13 after my initial check-in that disabled the test if Xalan was not available. Rick later re-enabled the test and made it run cleanly on platforms that didn't have the required XML libraries, so I believe it should be running fine now if you do an svn update first.

Knut Anders Hatlen
added a comment - 25/May/10 16:44 Hi Lily. From the output you posted above, it looks like the test is running successfully (it says OK - 10 tests). Or is there some other problem you are seeing?

Thank you so much, Kristian. That is such a good suggestion. I will give it a try.

I add the patch for Connection.flowcommit() so it won't flow if we don't need to perform the unnecessary round trip.
Please see DERBY-4653-2.diff. I also try to add the test for testing saving commit. I add an extra commit on J2EEDataSourceTest to test the fix. However, if the behavior go back to the round-trip scenario, the test has no way knowing other than functionality is working. Does anyone has a good suggest to test the DRDA protocol for commit making round trip or not?

About making Connection.flowrollback not doing the round trip, It is not as straight forward as detecting isUnitofWork_ only. I think the check has to happen along with isXAConnection_ and where we are in the rollback state. I will keep working on that today. However, I do think that it is safe to commit DERBY-4653-2.diff. I will suggest to open a new JIRA for rollback only depend on how to test Connection.flowcommit DRDA related protocol.

Lily Wei
added a comment - 26/May/10 15:36 Thank you so much, Kristian. That is such a good suggestion. I will give it a try.
I add the patch for Connection.flowcommit() so it won't flow if we don't need to perform the unnecessary round trip.
Please see DERBY-4653 -2.diff. I also try to add the test for testing saving commit. I add an extra commit on J2EEDataSourceTest to test the fix. However, if the behavior go back to the round-trip scenario, the test has no way knowing other than functionality is working. Does anyone has a good suggest to test the DRDA protocol for commit making round trip or not?
About making Connection.flowrollback not doing the round trip, It is not as straight forward as detecting isUnitofWork_ only. I think the check has to happen along with isXAConnection_ and where we are in the rollback state. I will keep working on that today. However, I do think that it is safe to commit DERBY-4653 -2.diff. I will suggest to open a new JIRA for rollback only depend on how to test Connection.flowcommit DRDA related protocol.

However, if the behavior go back to the round-trip scenario, the test has no way knowing other than functionality is working. Does anyone has a good suggest to test the DRDA protocol for commit making round trip or not?

A few suggestions (none of them really optimal):

add different trace points on the client for the cases where a commit is flowed or not, then parse the trace file
(this change could actually help debugging in production environments)

enable statement logging on the server and parse the log looking for the number of commits (assuming an "unnecessary" commit produces a log message)

some kind of timed test doing as many commits as possible (hard to set the thresholds, probably not a viable solution?)

add special debug code, or exploit poor state encapsulation in the client driver
(for instance, can the DRDA "sequence number" be used, or more likely, maybe the client side "transaction id"?)

Another related task would be to inspect the code to make sure the variable 'inUnitOfWork_' is in sync with reality.

Kristian Waagan
added a comment - 26/May/10 17:51 Lily wrote:
However, if the behavior go back to the round-trip scenario, the test has no way knowing other than functionality is working. Does anyone has a good suggest to test the DRDA protocol for commit making round trip or not?
A few suggestions (none of them really optimal):
add different trace points on the client for the cases where a commit is flowed or not, then parse the trace file
(this change could actually help debugging in production environments)
enable statement logging on the server and parse the log looking for the number of commits (assuming an "unnecessary" commit produces a log message)
some kind of timed test doing as many commits as possible (hard to set the thresholds, probably not a viable solution?)
add special debug code, or exploit poor state encapsulation in the client driver
(for instance, can the DRDA "sequence number" be used, or more likely, maybe the client side "transaction id"?)
Another related task would be to inspect the code to make sure the variable 'inUnitOfWork_' is in sync with reality.

Thanks Dag and Kristian.
I did use ClientDataSource and ClientDataSource.setTraceDirectory to see that the change for save round trip for the commit. I manually reading the trace file "_sds_0" to see the second commit() does not have that many trace information like SEND BUFFER: RDBCMN ... ENDDUOWRM ... Thanks Kathey for helping me understand those complicated message. There is SaveRoundClientDS.java and trace file "_sds_0" in the attachment to show the test code and trace information. To parse the trace file in the test suite, is there something available already? Can someone elaborate on this idea?

Lily Wei
added a comment - 27/May/10 16:07 Thanks Dag and Kristian.
I did use ClientDataSource and ClientDataSource.setTraceDirectory to see that the change for save round trip for the commit. I manually reading the trace file "_sds_0" to see the second commit() does not have that many trace information like SEND BUFFER: RDBCMN ... ENDDUOWRM ... Thanks Kathey for helping me understand those complicated message. There is SaveRoundClientDS.java and trace file "_sds_0" in the attachment to show the test code and trace information. To parse the trace file in the test suite, is there something available already? Can someone elaborate on this idea?

Would it be good enough to simply iterate over the lines in the trace file, counting occurrences of RDBCMM and RDBRLLBCK?

I modified your repro, adding some more commits and rollbacks. When doing a manual grep on the trace file, I got 50 matches without your patch, and 17 with.
To parse the file you should use SupportFilesSetup and PrivilegedFileOpsForTests.

Kristian Waagan
added a comment - 01/Jun/10 07:19 Would it be good enough to simply iterate over the lines in the trace file, counting occurrences of RDBCMM and RDBRLLBCK?
I modified your repro, adding some more commits and rollbacks. When doing a manual grep on the trace file, I got 50 matches without your patch, and 17 with.
To parse the file you should use SupportFilesSetup and PrivilegedFileOpsForTests.

I think that sounds like a good idea to check the file for the RDBCMM string and to make the name more predictable, you might want to use traceFile instead of traceDirectory. Also I think it makes sense to check in the commit part of this patch while working through the rollback issues in another patch, but it would be good to do so with the regression test for the commit portion.

Kathey Marsden
added a comment - 01/Jun/10 15:55 I think that sounds like a good idea to check the file for the RDBCMM string and to make the name more predictable, you might want to use traceFile instead of traceDirectory. Also I think it makes sense to check in the commit part of this patch while working through the rollback issues in another patch, but it would be good to do so with the regression test for the commit portion.

Thanks to Kristian and Kathey.
I fix the avoid traffic for flowrollback(). We need to just flow rollback when we are in XAConnection.
The patch passed suites.allpackages and derbyall.

To test the fix:
1. Will it be okay if we just reflect the method in ..am.Connection.getTransactionID() and make sure the next transaction is after first commit()()/rollback is a predictable number. After the test is run, there will always be a trace file hanging around for manual inspection. It could be cheating a little bit. However, it is a little easier than test open the trace file and search for the number of occurrences of RDBCMM or something like that.

Lily Wei
added a comment - 02/Jun/10 22:18 Thanks to Kristian and Kathey.
I fix the avoid traffic for flowrollback(). We need to just flow rollback when we are in XAConnection.
The patch passed suites.allpackages and derbyall.
To test the fix:
1. Will it be okay if we just reflect the method in ..am.Connection.getTransactionID() and make sure the next transaction is after first commit()()/rollback is a predictable number. After the test is run, there will always be a trace file hanging around for manual inspection. It could be cheating a little bit. However, it is a little easier than test open the trace file and search for the number of occurrences of RDBCMM or something like that.

Lily Wei
added a comment - 04/Jun/10 15:27 The fix is the same as before, we flowcommit() when it is safe and we flowrollback() when we are in XAConnection. I run it against Suites.All and Derbyall and they all passed.
With Kathey's help, I add new test to test just Connection.flowcommit() and Connection.flowrollback(). The different signature testConnectionFlowCommitRollback() is added to J2EEDataSourceTest.
In this test, when we flow commit, we check the transaction increase by 1.
When we are testing rollback, we rollback the transaction and execute an insert statement, the transaction increase by 2.
With the little repro, manual check the trace file to see the DRDA protocol trace save the round trip for flowcommit() and flowrollback() is also done.

Sorry for the short post (it's late here now), but I think there are some issues with the patch.
Feel free to have another look at it right away, I'll have a look on Monday in any case.

Also, I'll be happy to help you write the code parsing the trace file (it's good to keep the existing test) , i.e. something like:

obtain first connection, run test sequence without redundant commits

parse trace file to obtain baseline for number of commits

obtain second connection, run test sequence with many of extra commits

parse trace file to obtain number of commits

compare the commit counts

In this case it might be good to factor out the test sequence code into a method with a boolean telling wheter or not to do the extra commits.
It's fine if you don't implement this test, I may have a go at it if you don't

Kristian Waagan
added a comment - 04/Jun/10 20:37 Sorry for the short post (it's late here now), but I think there are some issues with the patch.
Feel free to have another look at it right away, I'll have a look on Monday in any case.
Also, I'll be happy to help you write the code parsing the trace file (it's good to keep the existing test) , i.e. something like:
obtain first connection, run test sequence without redundant commits
parse trace file to obtain baseline for number of commits
obtain second connection, run test sequence with many of extra commits
parse trace file to obtain number of commits
compare the commit counts
In this case it might be good to factor out the test sequence code into a method with a boolean telling wheter or not to do the extra commits.
It's fine if you don't implement this test, I may have a go at it if you don't

Thank you so much, Kristian. It is late over there. We are so international. Not a problem about the short post. I was not totally clear in turn of the issues with the patch.

It will be great to have your help on code parsing and run test sequence with all the necessary steps for commits. This will be more tests on top of the existing test. And, it will be very complete tests.

Lily Wei
added a comment - 04/Jun/10 23:07 Thank you so much, Kristian. It is late over there. We are so international. Not a problem about the short post. I was not totally clear in turn of the issues with the patch.
It will be great to have your help on code parsing and run test sequence with all the necessary steps for commits. This will be more tests on top of the existing test. And, it will be very complete tests.

Here are my comments on patch 'DERBY-4653-4_withcommitrollbacktest.diff':
— J2EEDataSourceTest
1) The test should be added to the client suite, since it is only relevant for the client driver.
2) The method comment could well be changed to a JavaDoc comment (/* -> /**)
3) Can the logic for skipping the test (i.e. wheter Connection.getTransactionID exists)
be moved into the suite method?)
If you create a static method to check if the method exists, if can be used both in the
suite method and in J2EEDataSourceTest.getConnectionID. There is one complication, and that is
that you cannot (or should not) obtain a connection in the suite method. Is passing the ClientDriver
class good enough?
If this gets too messy, just let it be. My concern with short-circuiting the tests themselves, is that
the will be listed as run and passed, even though they haven't really been run.
Does anyone have any options on this? Have we solved this problem earlier?
4) Instead of checking for a specific number of rows (X -14 >=0) ) in the system table
(which we don't really know), is it good enough to assert
rs.next() == true, rs.getInt() >= 0, rs.next() == false ?

— Connection
5) Wrong comment? If we are in a connection, we do want to commit, right?
6) Can you explain the reasoning behind the changes in flowRollback?
I don't understand them, as I'm not that familiar with XA.

— LogicalConnection
7) I don't understand the comment about embedded, nor why we have to add this method.
If it is to enable testing with logical connections (i.e. with XADataSource or CPDataSource),
maybe it is better to parse the trace file? In any case it would be nice to test a
"normal connection", since there is some special code for when the connection is originating
from CP-|XADataSource.

Possible improvements:
a) Add test parsing the trace file.
b) Test with XA as well (since there is special code for it).

Kristian Waagan
added a comment - 07/Jun/10 13:48 Hi Lily,
Here are my comments on patch ' DERBY-4653 -4_withcommitrollbacktest.diff':
— J2EEDataSourceTest
1) The test should be added to the client suite, since it is only relevant for the client driver.
2) The method comment could well be changed to a JavaDoc comment (/* -> /**)
3) Can the logic for skipping the test (i.e. wheter Connection.getTransactionID exists)
be moved into the suite method?)
If you create a static method to check if the method exists, if can be used both in the
suite method and in J2EEDataSourceTest.getConnectionID. There is one complication, and that is
that you cannot (or should not) obtain a connection in the suite method. Is passing the ClientDriver
class good enough?
If this gets too messy, just let it be. My concern with short-circuiting the tests themselves, is that
the will be listed as run and passed, even though they haven't really been run.
Does anyone have any options on this? Have we solved this problem earlier?
4) Instead of checking for a specific number of rows (X -14 >=0) ) in the system table
(which we don't really know), is it good enough to assert
rs.next() == true, rs.getInt() >= 0, rs.next() == false ?
— Connection
5) Wrong comment? If we are in a connection, we do want to commit, right?
6) Can you explain the reasoning behind the changes in flowRollback?
I don't understand them, as I'm not that familiar with XA.
— LogicalConnection
7) I don't understand the comment about embedded, nor why we have to add this method.
If it is to enable testing with logical connections (i.e. with XADataSource or CPDataSource),
maybe it is better to parse the trace file? In any case it would be nice to test a
"normal connection", since there is some special code for when the connection is originating
from CP-|XADataSource.
Possible improvements:
a) Add test parsing the trace file.
b) Test with XA as well (since there is special code for it).

wrt to some of Kristian's comments and a few things I talked to Lily about today.

I too think it would be good to test just for client and with regular, pooled, and xa datasources.

I think if the test runs just for client we should always count on the getTransactionID() method being there and can take out the skipping logic all together. I think the system table query can come out all together. I tend to think the getTransactionID() check is good enough, but the protocol trace parsing would be ok too.

I agree the comment about embedded should come out of am.LogicalConnection.

For the flowRollback() change I think the condition to return if not inUnitOfWork should not be in the if block. I think the StatementPoolingTest failures you were seeing with it set that way may be because the state of inUnitOfWork may be wrong somehow where hold cursors and pooled connections are involved, probably an existing issue that needs to be tracked down before committing this change. Lily and I tried to reproduce it outside the test with the code but we were not quite able to get in the same state as StatementPoolingTest.

Kathey Marsden
added a comment - 09/Jun/10 00:33 - edited wrt to some of Kristian's comments and a few things I talked to Lily about today.
I too think it would be good to test just for client and with regular, pooled, and xa datasources.
I think if the test runs just for client we should always count on the getTransactionID() method being there and can take out the skipping logic all together. I think the system table query can come out all together. I tend to think the getTransactionID() check is good enough, but the protocol trace parsing would be ok too.
I agree the comment about embedded should come out of am.LogicalConnection.
For the flowRollback() change I think the condition to return if not inUnitOfWork should not be in the if block. I think the StatementPoolingTest failures you were seeing with it set that way may be because the state of inUnitOfWork may be wrong somehow where hold cursors and pooled connections are involved, probably an existing issue that needs to be tracked down before committing this change. Lily and I tried to reproduce it outside the test with the code but we were not quite able to get in the same state as StatementPoolingTest.

Thanks Kathey for showing me around and making this patch to this stage and explain all the issues to me.

With this patch, it has the intention fix for Connection.flowcommit() and Connection.flowrollback(). However, when running suites.All, some tests failed. i.e. jdbcapi.StatementPollingTest

The change of the patch which answer the issue Kristian point out. Thanks Kristian for the looking at the patch and beyond.

J2EEDataSourceTest
1. The test gets added to getClientSuite(). The test is only running on regular client, pooled, and xa datasources.
2. Change to javadoc as Kristian point out.
3. Please see comment in BaseJdbcTestCase.java
4. No need to use the system table as part of test anymore.
BaseJdbcTestCase.java
5. getClientTransactonID(Connection conn) is in BaseJdbcTestCase.java so it is closer in suites than J2EEDataSourceTest and the comment is as following
"If we are not in a transaction, we don't want to flow commit. We just return"
6. For flowRollback: Like Kathey said, the original change is only for XAConnection which does not make sense. With the new change, server flow rollback if inUnitOfWork_ indicates we are not in a transaction, we return. However, with this change, StatementPoolingTest failed
at org.apache.derbyTesting.functionTests.tests.jdbcapi.StatementPoolingTest.doTestResultSetCloseForHoldability(StatementPoolingTest.java:591)
at org.apache.derbyTesting.functionTests.tests.jdbcapi.StatementPoolingTest.resTestHoldCursorsOverCommit(StatementPoolingTest.java:522)

I am in the middle of evaluating StatementPoolingTest failed and what change the value of inUnitOfWork_ when the test trying to close the connection and whether it has to with CachingLogicalConnection40. Will the test close the connection that is not the connection it performs rollback? What could be change the value of inUnitOfWork_ within StatementPoolingTest?

Lily Wei
added a comment - 09/Jun/10 05:41 - edited Thanks Kathey for showing me around and making this patch to this stage and explain all the issues to me.
With this patch, it has the intention fix for Connection.flowcommit() and Connection.flowrollback(). However, when running suites.All, some tests failed. i.e. jdbcapi.StatementPollingTest
The change of the patch which answer the issue Kristian point out. Thanks Kristian for the looking at the patch and beyond.
J2EEDataSourceTest
1. The test gets added to getClientSuite(). The test is only running on regular client, pooled, and xa datasources.
2. Change to javadoc as Kristian point out.
3. Please see comment in BaseJdbcTestCase.java
4. No need to use the system table as part of test anymore.
BaseJdbcTestCase.java
5. getClientTransactonID(Connection conn) is in BaseJdbcTestCase.java so it is closer in suites than J2EEDataSourceTest and the comment is as following
"If we are not in a transaction, we don't want to flow commit. We just return"
6. For flowRollback: Like Kathey said, the original change is only for XAConnection which does not make sense. With the new change, server flow rollback if inUnitOfWork_ indicates we are not in a transaction, we return. However, with this change, StatementPoolingTest failed
at org.apache.derbyTesting.functionTests.tests.jdbcapi.StatementPoolingTest.doTestResultSetCloseForHoldability(StatementPoolingTest.java:591)
at org.apache.derbyTesting.functionTests.tests.jdbcapi.StatementPoolingTest.resTestHoldCursorsOverCommit(StatementPoolingTest.java:522)
I am in the middle of evaluating StatementPoolingTest failed and what change the value of inUnitOfWork_ when the test trying to close the connection and whether it has to with CachingLogicalConnection40. Will the test close the connection that is not the connection it performs rollback? What could be change the value of inUnitOfWork_ within StatementPoolingTest?

I was looking at why StatementPoolingTest.test.doTestResultSetCloseorHoldability failure issue.
When the first time it gets to this signature, conn.rollback() will flow. At that time CachingLoginalConnection40.physicalConnection_.inUnitofWork_ is false which is expected.
However, when it gets to conn.close() which call StatementCacheInteractor.closeOpenLogicalStatements line 224 logicalStatement.close() the variable inUnitofWork_ become true. I am not familiar with closeOpenLogicalStatements synchronized method. Does this only get change in some sort of multi-thread situation?

When StatementPoolingTest get to doTestResultSetCloseorHoldability() the second time, the variable of physicalConnection_.inUnitofWork_ remains false when close are completely done.

What need to change so we can rely on inUnitofWork_ to be the flag to decide whether we can flow rollback or not?

Lily Wei
added a comment - 10/Jun/10 04:22 - edited I was looking at why StatementPoolingTest.test.doTestResultSetCloseorHoldability failure issue.
When the first time it gets to this signature, conn.rollback() will flow. At that time CachingLoginalConnection40.physicalConnection_.inUnitofWork_ is false which is expected.
However, when it gets to conn.close() which call StatementCacheInteractor.closeOpenLogicalStatements line 224 logicalStatement.close() the variable inUnitofWork_ become true. I am not familiar with closeOpenLogicalStatements synchronized method. Does this only get change in some sort of multi-thread situation?
When StatementPoolingTest get to doTestResultSetCloseorHoldability() the second time, the variable of physicalConnection_.inUnitofWork_ remains false when close are completely done.
What need to change so we can rely on inUnitofWork_ to be the flag to decide whether we can flow rollback or not?

Thanks to Kathey. I got to know more in turn of why Connection.close() after Connection.flowrollback() will cause error "Cannot close a connection while a transaction is still active" for StatementPoolingTest. As the ReproTransInProgressAttempt.java shows, if we have commit between we are in ResultSet, we should not flowrollback. If flowrollback remains unchanged, it will detect the state of the statement and resultSet and keep all the flag as inUnitOfWork etc in the right place and Connection.close() will not run into error as "Cannot close a connection while a transaction is still active".
Due to this reason, I am leading toward to not fix flowrollback() to save round trip and just keep the change for flowcommit() to improve performance.

Lily Wei
added a comment - 16/Jun/10 05:42 Thanks to Kathey. I got to know more in turn of why Connection.close() after Connection.flowrollback() will cause error "Cannot close a connection while a transaction is still active" for StatementPoolingTest. As the ReproTransInProgressAttempt.java shows, if we have commit between we are in ResultSet, we should not flowrollback. If flowrollback remains unchanged, it will detect the state of the statement and resultSet and keep all the flag as inUnitOfWork etc in the right place and Connection.close() will not run into error as "Cannot close a connection while a transaction is still active".
Due to this reason, I am leading toward to not fix flowrollback() to save round trip and just keep the change for flowcommit() to improve performance.

Lily Wei
added a comment - 18/Jun/10 05:09 This patch is without Connection.flowrollback performance improvement fix.
With the test, it is using the same approach as transaction id for Connection.flowcommit save round-trip fix. Move the getClientTransactionID to more test suite level - BaseJDBCTestCase.java
It is ready to commit. Running Suites.All passed. I am running derbyall test suite now.

Lily recommends putting in the change first for commit and has filed a second issue for rollback, That fix is perhaps not worth the change, because all open result sets would need to be checked before optimizing out the rollback flow.
She filed another issue for rollback DERBY-4687
, so I am changing the summary for this issue to cover just commit. I looked at the patch DERBY-4653-7_withflowcommittest.diff and I think it looks very good except a couple of stale comments. I will attach the revised patch and will commit Monday unless there are more comments.

Kathey Marsden
added a comment - 18/Jun/10 23:45 Lily recommends putting in the change first for commit and has filed a second issue for rollback, That fix is perhaps not worth the change, because all open result sets would need to be checked before optimizing out the rollback flow.
She filed another issue for rollback DERBY-4687
, so I am changing the summary for this issue to cover just commit. I looked at the patch DERBY-4653 -7_withflowcommittest.diff and I think it looks very good except a couple of stale comments. I will attach the revised patch and will commit Monday unless there are more comments.

I attached an updated version of 'DERBY-4653-7_withflowcommittest_comment_update_diff.txt', with the following changes:

simplified the reflection code somewhat

JavaDoc edits

replaced tabs with spaces and some formatting changes (long lines)

The patch looks okay, I have the following comments:
a) I'm not thrilled by adding the LogicalConnection.getTransactionID method just for testing purposes (when we have another mechanism we could use), but I won't object.
b) The argument 'expectednumtransaction' isn't used.
c) The comment in the actual test is slightly inaccurate. Since the test case is run with autocommit on, the commit will be flowed at rs.close() such that none of the calls to commit() will cause a flow to the server.

I'm +0.9 for commit

I'll need to read up on the problems regarding rollback. I suppose most connection pools will issue a rollback rather than a commit to be sure the connection state is "clean", which is why it would be good to find an optimization for rollback as well.

Kristian Waagan
added a comment - 22/Jun/10 09:31 I attached an updated version of ' DERBY-4653 -7_withflowcommittest_comment_update_diff.txt', with the following changes:
simplified the reflection code somewhat
JavaDoc edits
replaced tabs with spaces and some formatting changes (long lines)
The patch looks okay, I have the following comments:
a) I'm not thrilled by adding the LogicalConnection.getTransactionID method just for testing purposes (when we have another mechanism we could use), but I won't object.
b) The argument 'expectednumtransaction' isn't used.
c) The comment in the actual test is slightly inaccurate. Since the test case is run with autocommit on, the commit will be flowed at rs.close() such that none of the calls to commit() will cause a flow to the server.
I'm +0.9 for commit
I'll need to read up on the problems regarding rollback. I suppose most connection pools will issue a rollback rather than a commit to be sure the connection state is "clean", which is why it would be good to find an optimization for rollback as well.

I committed Kristian's version of DERBY-4653 and will leave to Lily to submit a follow up patch for the issues pointed out in his last comment. I think the trace file parsing test might be pretty fragile, so am not such a big fan.

I agree that rollback is important. I will post a thought to issue DERBY-4687,

Kathey Marsden
added a comment - 22/Jun/10 22:22 I committed Kristian's version of DERBY-4653 and will leave to Lily to submit a follow up patch for the issues pointed out in his last comment. I think the trace file parsing test might be pretty fragile, so am not such a big fan.
I agree that rollback is important. I will post a thought to issue DERBY-4687 ,

Lily Wei
added a comment - 22/Jun/10 22:53 This patch take out the unused variable expectednumtransaction and set the test case to run with autocommit off so we will flow on first commit and does not flow the second commit.

Kristian Waagan
added a comment - 23/Jun/10 11:34 Thanks Kathey and Lily.
I committed the follow-up patch with revision 957164.
Since Connection.setAutoCommit(boolean) may theoretically issue a commit, I moved it up before the call to fetch the transaction id.

Attached patch 8a, which implements an alternative test parsing the client connection trace file.
It's a bit more extensive than the existing test, but may not add any extra value.
Uploaded for reference, as it may come in useful for debugging purposes.

Kristian Waagan
added a comment - 23/Jun/10 11:39 Attached patch 8a, which implements an alternative test parsing the client connection trace file.
It's a bit more extensive than the existing test, but may not add any extra value.
Uploaded for reference, as it may come in useful for debugging purposes.