This is the diff from trunk for the patch.

The patch is to change org.apache.derby.client.am.Connection according to the bug description. When running suites.All, SetTransactionIsolationTest test failed. I think the failure was due to the old behavior of DerbyNetClient. I change the test to reflect the new behavior. I did not take out the logic for the old behavior. If I need to, I will be happy to do that. The changes is made to trunk. I am running derbyall tests now.

Thanks Lily for looking at this issue. Here are some comments.

• I think it would be good to put the check after the trace call so the call will still get logged if tracing is on.
• In the test, I guess we can just remove the whole switch statment passCommitCheck etc. and just have.
  assertEquals(1,count); since the behavior is the same for the drivers. The comment in the beginning of the test is no longer relevant and maybe the fixture name should be changed to testSetTransactionIsolationDoesNotCommit() or some such.
• I guess there was already an issue for this DERBY-2064 which should be duped to this one, since you are posting your work here.
• I think calling getTransactionIsolation() will introduce another server round trip to get the isolation if it is not cached. Under what circumstances will the isolation be cached or not cached? Is this a performance issue or does it happen rarely?
• Also calling getTransactionIsolation() directly will cause the getTransactionIsolation call to get traced which it probably shouldn't. We might need to make a getTransactionIsolationX() which does the work but doesn't have the tracing call. and then have the X method called by getTransasctionIsolation() and setTransactionIsolation()

Thanks Kathey for looking at the patch. I change the check to be after trace. I will mark DERBY-2064 as dup of this bug. I think getTransactionIsolation() will introduce server round trip. I am not sure the performance impact will be huge. I add a check for supportsSessionDataCaching() to prevent introduce server round trip if it is not cache. getTransactionIsolationX()is introduce and setTransactionIsolation() and getTransactionIsolation() will be calling it. testSetTransactionIsolationCommitRollback will be checking for assertEquals(1, count) thanks to Kathey's great points. I run suits.all again and all the tests passed. I am running derbyall now.

FYI, there are some related comments (and code) in Connection.completeReset.

For
if (supportsSessionDataCaching() && level == getTransactionIsolationX())
return;

I think we need to remove supportsSessionDataCaching() because we don't want setTrasactionIsolation to behave differently and always commit if the server does not support session data caching.

It seems that _isolation is set to TRANSACTION_UNKNOWN for new connections. This means we will always make an extra round trip for the first setTransactionIsolation call on a connection. Can we initialize _isolation to READ_COMMITTED when session data caching is supported instead since that is the default for new connections?

After chatting with Kathey and Kristian, I made this new patch. I run suites.all and derbyall is running now. All tests passed on suites.all with this patch change.

When debugging with the existing tests, I do see we do round trip for the first setTransactionIsolation call on a connection. However, I am not totally sure it justifies setting _isolation to TRANSACTION_UNKNOWN. Kristian was telling me client driver will ask the server about isolation level if it is unknown. This might be avoidable if piggybacking is available.

I found some useful information about the piggybacking here: http://wiki.apache.org/db-derby/Derby3192Writeup
Thank you, Dyre!

Kathey wrote:

It seems that _isolation is set to TRANSACTION_UNKNOWN for new connections. This means we will always make an extra round trip for the first setTransactionIsolation call on a connection. Can we initialize _isolation to READ_COMMITTED when session data caching is supported instead since that is the default for new connections?

I'm slightly skeptical about this suggested change:
1) It seems we have had bugs regarding assumptions about and caching of the isolation level earlier.
2) What happens if we in the future add the option to configure the default isolation level on the server?
It seems I have already introduced an assumption in the client code that may break (hard-coded assumption that the default level is RC, limited to pooled/XA connections - see Connection.completeReset).
3) This happens once for each connection if setTransactionIsolation is called before any other round trip to the server, assuming session state caching is available. Shouldn't applications creating loads of connections use a connection pool?

It seems to me that the client should be informed about the default isolation level as part of the connection initialization sequence. The default value could be stored, and used later on connection resets etc. I don't know if the DRDA specification allows this somehow. One solution could be the product specific code points.

The latest patch looks good to me. Nitpicks:

• mixed tabs/spaces indentation at @@ -909,6 +909,9 @@

• mixed tabs/spaces indentation at @@ -1005,8 +1008,21 @@

• traceExit is used instead of traceEntry. Is this intended?
  (I see this is also done in some other places in the client code)

This patch fixs

• mixed tabs/spaces indentation at @@ -909,6 +909,9 @@
• mixed tabs/spaces indentation at @@ -1005,8 +1008,21 @@
• use traceEntry instead of traceExit

If it will be better for performance, I will be better to change the _isolation to READ_COMMITTED. However, we have to handle the assumption in the code and piggybacking situation. Is this the best place to change the current assumption?

Run Suites.all and derbyall against trunk 799169. No failure.

I posted a program to DERBY-4343 which would be interesting to run with this patch with tracing on to understand the server round trips in various scenarios. Of course the assertion failure would have to be fixed first. I am curious though if this patch might actually fix that bug.

Attaching patch 6a, which demonstrates the piggy-backing of session data on connection initialization.

Comments on the approach/patch is welcome.
Note the following:

• I inserted the PBSD (session data code point(s)) at the end of the ACCRDB reply. Is this allowed by the DRDA standard?
  It will only be sent if the client is Derby and the version is 10.6 or greater.
• I have not yet implemented the change suggested, where the isolation level is initialized to READ_COMMITTED unconditionally in the client.
• The extra completeInitialPiggyBackSchema method was added because the debug assert fails with NPE (the metadata object hasn't been created yet, this happens in completeConnection).
• My test run hasn't completed yet.

Thanks Kristian. I apply the patch. I don't know about your question to DRDA standard.
I run it against the repro from DERBY-4343 TestConnReuser.java. I got the ASSERT failure with defaultisolation change. This is my output from running TestConnReuse.java
$ java TestConnReuse
FirstConnection
count = 22
Seccond Connection. Reuse Connection
Third Connection. Set same isolation
Exception in thread "main" org.apache.derby.shared.common.sanity.AssertFailure:
ASSERT FAILED
at org.apache.derby.shared.common.sanity.SanityManager.ASSERT(SanityMana
ger.java:98)
at org.apache.derby.client.am.Connection.setTransactionIsolationX(Connec
tion.java:987)
at org.apache.derby.client.am.Connection.setTransactionIsolation(Connect
ion.java:915)
at org.apache.derby.client.am.LogicalConnection.setTransactionIsolation(
LogicalConnection.java:253)
at TestConnReuse.main(TestConnReuse.java:33)
---------------
Stack traces for all live threads:
Thread name=Finalizer id=3 priority=8 state=WAITING isdaemon=true
java.lang.Object.wait(Native Method)
java.lang.ref.ReferenceQueue.remove(Unknown Source)
java.lang.ref.ReferenceQueue.remove(Unknown Source)
java.lang.ref.Finalizer$FinalizerThread.run(Unknown Source)

Thread name=main id=1 priority=5 state=RUNNABLE isdaemon=false
java.lang.Thread.dumpThreads(Native Method)
java.lang.Thread.getAllStackTraces(Unknown Source)
org.apache.derby.shared.common.sanity.ThreadDump.getStackDumpString(Thre
adDump.java:34)
sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
java.lang.reflect.Method.invoke(Unknown Source)
org.apache.derby.shared.common.sanity.AssertFailure$1.run(AssertFailure.
java:165)

Do you think I am missing something?

After careful review the code, I think there might be some issue relate to DRDA protocal with the patch. When running suites.all test suite, testSetIsolationWithStatement failed with the following assertion:
There was 1 error:
1) testSetIsolationWithStatement(org.apache.derbyTesting.functionTests.tests.jdb
capi.J2EEDataSourceTest)org.apache.derby.client.am.XaException: XAER_DUPID : Err
or executing a XAResource.start(), server returned XAER_DUPID.
at org.apache.derby.client.net.NetXAResource.throwXAException(NetXAResou
rce.java:756)
at org.apache.derby.client.net.NetXAResource.start(NetXAResource.java:64
7)
at org.apache.derbyTesting.functionTests.tests.jdbcapi.J2EEDataSourceTes
t.testSetIsolationWithStatement(J2EEDataSourceTest.java:1623)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:
109)

Looking at NetXAResource.start with Xid xid2 = new cdsXid(1, (byte) 93, (byte) 103);
xar.start(xid2, XAResource.TMNOFLAGS); on J2EEDataSourceTest, I am thinking it might be relate to change of behavior of DRDA protocal. I need to further investigate on this.

With repro on Derby-4343 (Please refer to second attachment file TestConnReuse.java), if the third connection does not set the transaction isolation level to Connection.TRANSACTION_READ_COMMITTED but Connection.TRANSACTION_READ_UNCOMMITTED, we can get third connection without assertion.

This is the assertion trace if the third connection set the transcationIsolation to Connection.TRANSACTION_READ_COMMITTED.
Exception in thread "main" org.apache.derby.shared.common.sanity.AssertFailure:
ASSERT FAILED
at org.apache.derby.shared.common.sanity.SanityManager.ASSERT(SanityMana
ger.java:98)
at org.apache.derby.client.am.Connection.setTransactionIsolationX(Connec
tion.java:987)
at org.apache.derby.client.am.Connection.setTransactionIsolation(Connect
ion.java:915)
at org.apache.derby.client.am.LogicalConnection.setTransactionIsolation(
LogicalConnection.java:253)
at TestConnReuse.main(TestConnReuse.java:33)

Any guideline on the cause of reason is highly appreciated.

Hi Kristian or Knut:
Would you mind provide any information to 6b patch which has Kristian's piggy backing change of session data change cause TestConnReuse to give exception on Connection.setTransactionIsolationX at the first connection when the isolation level is TRANSACTION_READ_COMMITTED? Thanks, Lily

Hi Lily,

I'm afraid I'm not able to reproduce the assert failure when I apply the 6b patch on head of trunk and run TestConnReuse. I tried to change the isolation levels as described above, but I may have got it wrong. Perhaps you could attach an updated version of TestConnReuse.java that shows the problem with no modification? Thanks.

Thank you, Knut. You are right. The repro is passing with 6b. I did not merge right after Connection.java has newer version. However, I am still seeing J2EEDataSourceTest exception from DERBY-1325 repro. I am seeing exception as following:
1) testSetIsolationWithStatement(org.apache.derbyTesting.functionTests.tests.jdb
capi.J2EEDataSourceTest)org.apache.derby.client.am.XaException: XAER_DUPID : Err
or executing a XAResource.start(), server returned XAER_DUPID.
at org.apache.derby.client.net.NetXAResource.throwXAException(NetXAResou
rce.java:756)
at org.apache.derby.client.net.NetXAResource.start(NetXAResource.java:64
7)
at org.apache.derbyTesting.functionTests.tests.jdbcapi.J2EEDataSourceTes
t.testSetIsolationWithStatement(J2EEDataSourceTest.java:1623)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:
109)

I am thinking this is related to piggy backing of session data. However, I am all open to new suggestion.

Post the diff after merge Connection.java

I think the more relevant failure is the first one that occurs:

1) testGlobalLocalInterleaf(org.apache.derbyTesting.functionTests.tests.jdbcapi.
J2EEDataSourceTest)junit.framework.AssertionFailedError: expected:<1> but was:<2
>
at org.apache.derbyTesting.functionTests.tests.jdbcapi.J2EEDataSourceTes
t.assertConnectionState(J2EEDataSourceTest.java:3341)
at org.apache.derbyTesting.functionTests.tests.jdbcapi.J2EEDataSourceTes
t.testGlobalLocalInterleaf(J2EEDataSourceTest.java:1501)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.
java:48)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAcces
sorImpl.java:37)
at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:
109)
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
)
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 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

Around line 1406 we end a global transaction:
assertConnectionState(
ResultSet.CLOSE_CURSORS_AT_COMMIT,
Connection.TRANSACTION_READ_UNCOMMITTED,
false, ReadOnly, cs1);

xar.end(xid, XAResource.TMSUCCESS);

Then we get a new logical connection in between (with isolation READ_COMMITTED) and then

about 1499 we join the global transaction
xar.start(xid, XAResource.TMJOIN);
cs1 = xac.getConnection();
// re-join with new handle X1
assertConnectionState(
ResultSet.CLOSE_CURSORS_AT_COMMIT,
Connection.TRANSACTION_READ_UNCOMMITTED,
false, ReadOnly, cs1);

But we get the error
J2EEDataSourceTest)junit.framework.AssertionFailedError: expected:<1> but was:<2
>
So the isolation level was not restored to READ_UNCOMMITED when we joined. I haven't looked at the patch, but I think this is the issue that would be best to look at first. I think the DUPID error may be just a cascading failure that occurs because of this one.

Here is a reproduction for the problem with the c patch where the isolation level is not restored after rejoining the global transaction. ReproIsoLost and utilXID, Run like
java ReproIsoLost

with the patch we get:
FAIL: Isolation is:TRANSACTION_READ_COMMITTED instead of READ_UNCOMMITTED

I noticed, with the patch, if I add

Statement s = cs1.createStatement();
s.executeUpdate("create table foo (i int)");

just before
xar.end(xid, XAResource.TMSUCCESS);

I get on the subsequent cs1 = xaconn.getConnection();
$ java ReproIsoLost
2010-04-27 00:18:34.201 GMT : Apache Derby Network Server - 10.7.0.0 alpha - (93
8130:938140M) started and ready to accept connections on port 1597
Exception in thread "main" java.sql.SQLNonTransientConnectionException: An error
occurred during connect reset and the connection has been terminated. See chai
ned exceptions for details.
at org.apache.derby.client.am.SQLExceptionFactory40.getSQLException(SQLE
xceptionFactory40.java:70)
at org.apache.derby.client.am.SqlException.getSQLException(SqlException.
java:358)
at org.apache.derby.client.ClientPooledConnection.getConnection(ClientPo
oledConnection.java:259)
at org.apache.derby.client.ClientXAConnection.getConnection(ClientXAConn
ection.java:70)
at ReproIsoLost.main(ReproIsoLost.java:51)
Caused by: org.apache.derby.client.am.DisconnectException: An error occurred dur
ing connect reset and the connection has been terminated. See chained exception
s for details.
at org.apache.derby.client.am.Connection.reset(Connection.java:2175)
at org.apache.derby.client.ClientPooledConnection.getConnection(ClientPo
oledConnection.java:245)
... 2 more

In the derby.log I have:
, (SESSIONID = 3), (DATABASE = wombat), (DRDAID = NF000001.GC36-867504605894726310

), Failed Statement is: SET CURRENT ISOLATION = CS

ERROR X0Y77: Cannot issue set transaction isolation statement on a global transaction that is in progress because it would have implicitly committed the global transaction.

at org.apache.derby.iapi.error.StandardException.newException(StandardException.java:276)

at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.setIsolationLevel(GenericLanguageConnectionContext.java:2650)

at org.apache.derby.impl.sql.execute.SetTransactionIsolationConstantAction.executeConstantAction(SetTransactionIsolationConstantAction.java:86)

at org.apache.derby.impl.sql.execute.MiscResultSet.open(MiscResultSet.java:61)

at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(GenericPreparedStatement.java:436)

at org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:317)

at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1232)

at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:625)

at org.apache.derby.impl.jdbc.EmbedStatement.executeUpdate(EmbedStatement.java:175)

at org.apache.derby.iapi.jdbc.BrokeredStatement.executeUpdate(BrokeredStatement.java:113)

at org.apache.derby.impl.drda.DRDAConnThread.parseEXCSQLIMM(DRDAConnThread.java:5133)

at org.apache.derby.impl.drda.DRDAConnThread.processCommands(DRDAConnThread.java:755)

at org.apache.derby.impl.drda.DRDAConnThread.run(DRDAConnThread.java:295)

Cleanup action completed

This passes with 10.5 and presumably without the patch.

At this point, I am hesitate to keep working on piggy-backing fix due to the fact that we can not execute a statement in the local transaction when we rejoin to the global transaction per Kathey's example( please see ReproIsoLost.java). With DERBY-4314-5 fix, we don't commit when calling setTransactionIsolation(). However, we do have performance issue when connection is not cache. We will have server round-trip when we first call seTransactionIsolation when creating the connection, we will have two round trip when isolation is not set to READ_COMMITTED (check + change) In any case, DERBY-4314-5 do fix setTransctonIsolation() not perform commit issue. Therefore, I am leading toward to submit DERBY-4314-5 change and open a new JIRA for the performance issue. From the new JIRA, we can investigate further about the piggybacking issue and beyond and have conclusion on this issue and DERBY-4343. Is this sound like a reasonable solution for this issue?

Attached patch 6d, which is a combined patch with some additional XA changes in the client.

Can someone with more knowledge about XA have a look?
Briefly described, I have made the client not reset the isolation level when we are doing a join or resume. Instead, we pick up the piggy-backed value from the server. The server, I believe, is keeping track of the correct value for the isolation level .

The full regression tests are running, I verified that the tests that failed with the previous patch(es) passed.

This is the version I intent to submit for this fix.
This fix avoid assertion like DERBY-4343 by short-circuiting setTransactionIsolation.
For case as users obtaining the pooled connection for the third time, the variable
isolation_ is reset Connection.completeReset. If users call setTransactionIsolation and executed, the server does not send any piggybacking update because the isolation level has not changed.
Isolation_ remain as UNKNOWN until getTransactionIsolation is called
or a different statement causing a change of the isolation level
is executed. As mention before, we should think about change this to READ_UNCOMMITTED.
With introducing getTransactionIsolationX and assertion is never reach. Therefore, no assertion.
The client driver acts as embedded as without commit action when setTransactionIsolation is called.
Performance concern: This fix does not improve performance, it will just move the server round-trip. In some cases performance will be worse for the initial setTransactionIsolation call, depending on
whether isolation_ value being set to READ_COMMITTED or not. If it is, one round-trip (check + short-circuit). If it is not, two round-trip (check + change).

I don't fully understand piggybacking. Any suggestion is welcome. I simply think this is a good place to address this bug and DERBY-4343. I will suggestion to open a new JIRA for the performance concern.

I run suits.all and derbyall.

Patch 6d ran without failures.

If I'm not mistaken, Lily's latest patch is a "sub-patch" of 6d, so committing it here and creating a new issue for the piggy-backing is fine with me.

Thanks Kristian for combined patch with some additional XA changes in the client. I believe the fix also address the server does not send any piggybacking update and improve the performance concern since we are getting the right isolation level. We still need to think about whether to check isolation_ value to TRANSACTION_READ_COMMITTED. With this patch, I also add more comment on setTransactionIsolation since that was why we commit when call setTransactionIsolation.

Suite.all and derby tests passed.

I will look at the 7b-combine patch and commit. Thanks Lily and Kristian for this collaborative effort !

Thanks to Kathey and Kristian, with #940620, setTransactionIsolation will not commit and it behaves the same for network server and embedded server. The nightly tests are coming up.

Change fix version information and add patch available

The issue DERBY-2064, marked as a duplicate, has Release Note Needed set.
Is a release note needed?
If so, it should be written and the issue should be resolved.

Yes, I think we should have a release note saying that the client Driver will no longer commit on setTransactionIsolation if the isolation level does not change. Users relying on the previous behavior to commit their transaction, should now add an explicit commit to their code.

See Writing a release note at: http://wiki.apache.org/db-derby/ReleaseNoteProcess

Thanks to Kathey, I am able to prepare the release note for setTransactionIsolation change. The change was made to client to match the embedded behavior and to improve client performance.

Hi Lily,

Here are my comments on the release note:
— Summary of Change
a) Typo in method name. Maybe use <tt></tt> around method names? Include the API class, i.e. Connection.getTransactionIsolation?
I.e. "<tt>Connection.setTransactionIsolation</tt> in the Derby client driver will not issue a commit if the isolation level does not change."
— Symptoms Seen by Applications Affected by Change
b) "Application" -> "Applications"
— Incompatibilities with Previous Release
c) "In previous releases, <tt>Connection.setTransactionIsolation</tt> in the Derby client driver would issue a commit even if the isolation level did not change."
d) Skip the rest of the paragraph, it is already mentioned under summary of change?
— Rationale for Change
e) " ...embedded behavior..." -> "...behavior of the embedded driver..."
— Application Changes Required
f) "transactions" -> "transaction"?

Use your own judgment on the suggestions

Thanks,

Thanks Kristian for reviewing the release note. I make the change accordingly.
a) Typo in method name. Maybe use <tt></tt> around method names? Include the API class, i.e. Connection.getTransactionIsolation?
I.e. "<tt>Connection.setTransactionIsolation</tt> in the Derby client driver will not issue a commit if the isolation level does not change."
<tt></tt> has been added to API class for the releaseNote of DERBY-4314.
— Symptoms Seen by Applications Affected by Change
b) "Application" -> "Applications"
This has been changed.
— Incompatibilities with Previous Release
c) "In previous releases, <tt>Connection.setTransactionIsolation</tt> in the Derby client driver would issue a commit even if the isolation level did not change."
The change has been made accordingly.
d) Skip the rest of the paragraph, it is already mentioned under summary of change?
As it for me, it is clearer to have it there. I decide to keep it there. I hope that is okay with everybody. If not, we can take it out.
— Rationale for Change
e) " ...embedded behavior..." -> "...behavior of the embedded driver..."
Changed.
— Application Changes Required
f) "transactions" -> "transaction"?
Fix the typo.

Thank you so much for all the help,
Lily

reopen to add label