Derby
  1. Derby
  2. DERBY-1016

javax.transaction.xa.forget (Xid) raises XAER_NOTA exception instead of XA_PROTO on a prepared transaction

    Details

    • Urgency:
      Normal
    • Issue & fix info:
      High Value Fix, Known fix, Newcomer, Release Note Needed, Repro attached

      Description

      javax.transaction.xa.forget (Xid) raises XAER_NOTA exception instead of XA_PROTO on a prepared transaction

      I posted a question to derby-dev about this and heard no response so am assuming it is indeed a bug.

      in the XA+
      specification, it seems like xa_forget should only be valid for a
      heuristically completed transaction, so should be XAER_PROTO
      and not XAER_NOTA.

      In xaStateTran.sql we have this case:

      – get back into prepared state
      xa_start xa_noflags 50;
      insert into xastate values(2);
      xa_end xa_success 50;
      xa_prepare 50;

      select * from global_xactTable where gxid is not null order by gxid;

      – the following should error XAER_NOTA
      xa_forget 50;

      The user code I am looking at handles forget like this. They expect
      XAER_PROTO in this case.

      try

      { xaRes.forget(xidList[i]); System.out.print("XA-Transaction [" + (i+1) + "] Forgotten. \n" ); }

      catch (XAException XAeForget) {
      if ( XAeForget.errorCode ==
      XAException.XAER_PROTO )

      { System.out.print("XA-Transaction [" + (i+1) + "] not heuristically completed yet - Rolling Back instead. \n" ); xaRes.rollback(xidList[i]); System.out.print("XA-Transaction [" + (i+1) + "] Rolled Back. \n" ); }

      if ( XAeForget.getMessage() != null ) {
      System.out.println("XAException " +
      XAeForget.getMessage() );

      1. ReproDerby1016.java
        3 kB
        Kathey Marsden
      2. utilXid.java
        2 kB
        Kathey Marsden
      3. DERBY-1016_Patch_1.diff
        0.5 kB
        Ravinder Reddy
      4. DERBY-1016.patch
        2 kB
        Tiago R. Espinha
      5. DERBY-1016.patch
        3 kB
        Tiago R. Espinha
      6. derby1016-donotcommit.diff
        4 kB
        Jayaram Subramanian
      7. derby1016-stat.txt
        0.9 kB
        Jayaram Subramanian
      8. runoutputNov30.out
        1 kB
        Jayaram Subramanian
      9. DERBY-1016.diff
        4 kB
        Myrna van Lunteren
      10. releaseNote.html
        1 kB
        Jayaram Subramanian
      11. releaseNote.html
        1 kB
        Rick Hillegas
      12. releaseNote.html
        1 kB
        Rick Hillegas

        Activity

        Hide
        Knut Anders Hatlen added a comment -

        [bulk update] Close all resolved issues that haven't been updated for more than one year.

        Show
        Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.
        Hide
        Kathey Marsden added a comment -

        I think that looks great. Thanks Rick and Jayaram.

        Show
        Kathey Marsden added a comment - I think that looks great. Thanks Rick and Jayaram.
        Hide
        Rick Hillegas added a comment -

        Attaching a new version of the release note, which hopefully clarifies the point Kathey raised. If this still doesn't look correct, feel free to wordsmith it further.

        Show
        Rick Hillegas added a comment - Attaching a new version of the release note, which hopefully clarifies the point Kathey raised. If this still doesn't look correct, feel free to wordsmith it further.
        Hide
        Kathey Marsden added a comment -

        I think the release note makes it sound like we throw XAER_PROTO in all cases of forget() even if the transaction does not exist. If the transaction does not exist, we still throw XAER_NOTA.

        So we throw XAER_PROTO for forget() with an active transaction id. XAER_NOTA if the transaction does not exist. Prior behavior was we would throw XAER_NOTA in all cases when forget() was called.

        Show
        Kathey Marsden added a comment - I think the release note makes it sound like we throw XAER_PROTO in all cases of forget() even if the transaction does not exist. If the transaction does not exist, we still throw XAER_NOTA. So we throw XAER_PROTO for forget() with an active transaction id. XAER_NOTA if the transaction does not exist. Prior behavior was we would throw XAER_NOTA in all cases when forget() was called.
        Hide
        Rick Hillegas added a comment -

        Thanks for the first draft of the release note, Jayaram. I have re-worded it slightly. Feel free to correct it further if you think I garbled it.

        It may be that your classpath did not include the class tree generated by the Derby build. ReleaseNoteReader is compiled into the class tree but it isn't put into any of the Derby jars.

        Thanks,
        -Rick

        Show
        Rick Hillegas added a comment - Thanks for the first draft of the release note, Jayaram. I have re-worded it slightly. Feel free to correct it further if you think I garbled it. It may be that your classpath did not include the class tree generated by the Derby build. ReleaseNoteReader is compiled into the class tree but it isn't put into any of the Derby jars. Thanks, -Rick
        Hide
        Jayaram Subramanian added a comment -

        Please find the attached release notes. But i was not able to validate the release notes, since i kept getting classnotfound exception when trying to run java org.apache.derbyBuild.ReleaseNoteReader releaseNote.html.

        Show
        Jayaram Subramanian added a comment - Please find the attached release notes. But i was not able to validate the release notes, since i kept getting classnotfound exception when trying to run java org.apache.derbyBuild.ReleaseNoteReader releaseNote.html.
        Hide
        Rick Hillegas added a comment -

        Hello,

        This issue is marked as needing a release note but no release note has been provided. Could someone please supply a release note? Thanks.

        Show
        Rick Hillegas added a comment - Hello, This issue is marked as needing a release note but no release note has been provided. Could someone please supply a release note? Thanks.
        Hide
        Kathey Marsden added a comment -

        Although a behavior correction, this is a behavior change so I think needs a release note and is probably not appropriate for backport.

        Show
        Kathey Marsden added a comment - Although a behavior correction, this is a behavior change so I think needs a release note and is probably not appropriate for backport.
        Hide
        Myrna van Lunteren added a comment -

        It seems to me the diff in the failing xaSimpleNegative test was exactly in the case for which this bug was logged, this is from xaSimpleNegative.out:
        ij(XA)> xa_prepare 1;
        ij(XA)> – can only forget heuristically completed transaction
        xa_forget 1;
        IJ ERROR: XAER_PROTO

        So I updated the master with revision 1210972.
        resolving again.

        Show
        Myrna van Lunteren added a comment - It seems to me the diff in the failing xaSimpleNegative test was exactly in the case for which this bug was logged, this is from xaSimpleNegative.out: ij(XA)> xa_prepare 1; ij(XA)> – can only forget heuristically completed transaction xa_forget 1; IJ ERROR: XAER_PROTO So I updated the master with revision 1210972. resolving again.
        Hide
        Myrna van Lunteren added a comment -

        Reopening because this check in caused a test failure in jdbcapi/xaSimpleNegative.sql:

            • Start: xaSimpleNegative jdk1.6.0_24 derbyall:xa 2011-12-06 01:06:38 ***
              193 del
              < IJ ERROR: XAER_NOTA
              193a193
              > IJ ERROR: XAER_PROTO
              Test Failed.
        Show
        Myrna van Lunteren added a comment - Reopening because this check in caused a test failure in jdbcapi/xaSimpleNegative.sql: Start: xaSimpleNegative jdk1.6.0_24 derbyall:xa 2011-12-06 01:06:38 *** 193 del < IJ ERROR: XAER_NOTA 193a193 > IJ ERROR: XAER_PROTO Test Failed.
        Hide
        Myrna van Lunteren added a comment -

        I committed the last patch (my modifications based on Jayaram's) with revision 1210686.

        Show
        Myrna van Lunteren added a comment - I committed the last patch (my modifications based on Jayaram's) with revision 1210686.
        Hide
        Myrna van Lunteren added a comment -

        Thanks Jayaram for that patch.

        There are a couple of concerns with your patch.
        Firstly, the .stat file seems to list a number of files that have conflicts.
        Secondly, there were some unnecessary whitespace differences, some unnecessary brackets, and there were some left-over references to java.io.PrintStream - probably from some debugging attempts.
        Usually, before submitting a patch, it makes sense to review your svn diff result and carefully evaluate your changes...

        Apart from that, the patch looked fine to me.
        However, it seems that the forget method in EmbedXAResource that was changed for this test would return either an XAER_NOTA or XAER_PROTO, and so perhaps it would be nice to have an additional test case for the XAER_NOTA.
        I've uploaded a patch that makes those minor adjustments.

        However, I have two further questions:

        • before submitting this latest patch, you mentioned seeing the test fail. What happened between then and now? Did the problem get resolved by doing a clean build (after ant clobber) perhaps?
        • Did you run suites.All?

        I ran jdbcapi._Suite which I think is enough to test the latest patch. That patch is now ready for review.

        Show
        Myrna van Lunteren added a comment - Thanks Jayaram for that patch. There are a couple of concerns with your patch. Firstly, the .stat file seems to list a number of files that have conflicts. Secondly, there were some unnecessary whitespace differences, some unnecessary brackets, and there were some left-over references to java.io.PrintStream - probably from some debugging attempts. Usually, before submitting a patch, it makes sense to review your svn diff result and carefully evaluate your changes... Apart from that, the patch looked fine to me. However, it seems that the forget method in EmbedXAResource that was changed for this test would return either an XAER_NOTA or XAER_PROTO, and so perhaps it would be nice to have an additional test case for the XAER_NOTA. I've uploaded a patch that makes those minor adjustments. However, I have two further questions: before submitting this latest patch, you mentioned seeing the test fail. What happened between then and now? Did the problem get resolved by doing a clean build (after ant clobber) perhaps? Did you run suites.All? I ran jdbcapi._Suite which I think is enough to test the latest patch. That patch is now ready for review.
        Hide
        Jayaram Subramanian added a comment -

        Attaching the fix for review. Also attaching the test output.

        Show
        Jayaram Subramanian added a comment - Attaching the fix for review. Also attaching the test output.
        Hide
        Jayaram Subramanian added a comment -

        Thanks Kathy,
        As per the issue history, to make the xa_forget to throw the xaer_proto error, modified the patched EmbedXAResource.java from

        throw new XAException(tranState.isPrepared ? XAException.XAER_PROTO: XAException.XAER_NOTA)
        to
        throw new XAException(XAException.XAER_PROTO)

        After making the above change, tried running attached fixture testForgetExceptionDerby1016. But i get the exception below.

        1) testForgetExceptionDerby1016(org.apache.derbyTesting.functionTests.tests.jdbcapi.XATransactionTest)junit.framework.AssertionFailedError:
        FAIL: Got unexpected exception null errorCode: -4 calling forget on a prepared transaction expected:<-6> but was:<-4>
        at org.apache.derbyTesting.functionTests.tests.jdbcapi.XATransactionTest.testForgetExceptionDerby1016(XATransactionTest.java:447)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:48)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:116)
        at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
        at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
        at junit.extensions.TestSetup.run(TestSetup.java:25)
        at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
        at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
        at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
        at junit.extensions.TestSetup.run(TestSetup.java:25)
        at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)

        Show
        Jayaram Subramanian added a comment - Thanks Kathy, As per the issue history, to make the xa_forget to throw the xaer_proto error, modified the patched EmbedXAResource.java from throw new XAException(tranState.isPrepared ? XAException.XAER_PROTO: XAException.XAER_NOTA) to throw new XAException(XAException.XAER_PROTO) After making the above change, tried running attached fixture testForgetExceptionDerby1016. But i get the exception below. 1) testForgetExceptionDerby1016(org.apache.derbyTesting.functionTests.tests.jdbcapi.XATransactionTest)junit.framework.AssertionFailedError: FAIL: Got unexpected exception null errorCode: -4 calling forget on a prepared transaction expected:<-6> but was:<-4> at org.apache.derbyTesting.functionTests.tests.jdbcapi.XATransactionTest.testForgetExceptionDerby1016(XATransactionTest.java:447) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:48) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:116) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
        Hide
        Jayaram Subramanian added a comment -

        Hi
        1) In the attaced test case suite, should we add a case for XAER_NOTA scenario
        2) In the atttached test case should we test for a positive scenario where XA_FORGET goes through without any issues
        3) Also in simillar lines should we test how XA_FORGET behaves after a commit or rollback.

        Show
        Jayaram Subramanian added a comment - Hi 1) In the attaced test case suite, should we add a case for XAER_NOTA scenario 2) In the atttached test case should we test for a positive scenario where XA_FORGET goes through without any issues 3) Also in simillar lines should we test how XA_FORGET behaves after a commit or rollback.
        Hide
        Kathey Marsden added a comment -

        Hi Jayaram,

        Thank you for looking at this issue. If you read through the comments you will see that Tiago did most of the work to figure out what the behavior should be. So the code fix itself should probably just be a few lines plus you will need to add a test to XATest, but you may want to familiarize yourself with XA first.

        Here is some reference material that you might find helpful in learning about XA
        The XA Open Specification
        https://www2.opengroup.org/ogsys/jsp/publications/PublicationDetails.jsp?publicationid=11144
        The JTA Specification
        http://www.oracle.com/technetwork/java/javaee/jta/index.html

        Show
        Kathey Marsden added a comment - Hi Jayaram, Thank you for looking at this issue. If you read through the comments you will see that Tiago did most of the work to figure out what the behavior should be. So the code fix itself should probably just be a few lines plus you will need to add a test to XATest, but you may want to familiarize yourself with XA first. Here is some reference material that you might find helpful in learning about XA The XA Open Specification https://www2.opengroup.org/ogsys/jsp/publications/PublicationDetails.jsp?publicationid=11144 The JTA Specification http://www.oracle.com/technetwork/java/javaee/jta/index.html
        Hide
        Tiago R. Espinha added a comment -

        Unassigning as I'm working on DERBY-728 and other people might pick this up in the meanwhile.

        Show
        Tiago R. Espinha added a comment - Unassigning as I'm working on DERBY-728 and other people might pick this up in the meanwhile.
        Hide
        Kathey Marsden added a comment -

        That behavior sounds reasonable to me and provides the most intuitive user feedback I think. We should have a release note describing the cases that change.

        Show
        Kathey Marsden added a comment - That behavior sounds reasonable to me and provides the most intuitive user feedback I think. We should have a release note describing the cases that change.
        Hide
        Tiago R. Espinha added a comment -

        I have been discussing this issue with Kathey and Myrna on IRC and it is generally hard to reach an optimal behavior, mostly because the XA specification is too vague and not very clear on this issue.

        On MySQL for example, we are able to invoke a forget on a transaction right after we start it, whereas this is not possible in DB2. Also, running the attached repro against MySQL makes it fail with XAER_RMFAIL:
        XAER_RMFAIL: The command cannot be executed when global transaction is in the ACTIVE state

        Still, since we don't have heuristic transactions in Derby, forget will always return an error and never really succeed. This way, we thought the best behavior is to throw XAER_NOTA (not a transaction) whenever the XID doesn't exist and throw XAER_PROTO for all the cases where the XID exists.

        Should no one object to this change, I'll create a patch and go through with it.

        Show
        Tiago R. Espinha added a comment - I have been discussing this issue with Kathey and Myrna on IRC and it is generally hard to reach an optimal behavior, mostly because the XA specification is too vague and not very clear on this issue. On MySQL for example, we are able to invoke a forget on a transaction right after we start it, whereas this is not possible in DB2. Also, running the attached repro against MySQL makes it fail with XAER_RMFAIL: XAER_RMFAIL: The command cannot be executed when global transaction is in the ACTIVE state Still, since we don't have heuristic transactions in Derby, forget will always return an error and never really succeed. This way, we thought the best behavior is to throw XAER_NOTA (not a transaction) whenever the XID doesn't exist and throw XAER_PROTO for all the cases where the XID exists. Should no one object to this change, I'll create a patch and go through with it.
        Hide
        Kathey Marsden added a comment -

        Just to add in another opinion, I consulted with my colleague with the JCC team (DB2) and he feels XAER_PROTO is correct. He said:
        "I would say that XAER_PROTO is more appropriate as the resource manager is aware of the XID in prepared state."

        Show
        Kathey Marsden added a comment - Just to add in another opinion, I consulted with my colleague with the JCC team (DB2) and he feels XAER_PROTO is correct. He said: "I would say that XAER_PROTO is more appropriate as the resource manager is aware of the XID in prepared state."
        Hide
        Kathey Marsden added a comment -

        DB2 also returns XAER_PROTO for the case where forget() is called after start() (This case now returns XAER_NOTA with the patch)

        My understanding is that Informix also returns XAER_PROTO for both cases but I have not confirmed that myself.

        Tiago found Postgress returned XAER_NOTA in both cases.

        A couple things to note when trying to sort out the spec and our behavior.
        1) There are no positive cases for forget() with Derby I think. We do not have a method for heuristic completion, so it is just a matter of which error to throw. In the xa spec under xa_fprget() it says:

        XAER_NOTA - The specified XID is not known by the resource manager as a heuristically completed XID.

        XAER_PROTO - The routine was invoked in an improper context.

        2) For the original case on which this Jira was based, the user was trying to write a generic recovery program which would forget transactions that had been heuristically completed and rollback if they were not heuristically completed but rather still in a prepared state. He would first try the forget() and then if that gave an XAER_PROTO would attempt the rollback(). The workaround we gave was to also check for XAER_NOTA.

        3) It is interesting to note that for other calls XAER_NOTA has a different description.
        XAER_NOTA - The specified XID is not known by the resource manager.

        I wish I understood some more about heuristic completion but tend to think that since the only way to differentiate a prepared vs heuristically completed transaction returned by recover() is to call forget, the user was not really calling it in an improper context and someone went out of their way to put that extra condition on the XAER_NOTA exception desription for forget(), so my somewhat uneasy opinion is that XAER_NOTA is most appropriate whenever forget() is called with Derby. Opinions?

        Show
        Kathey Marsden added a comment - DB2 also returns XAER_PROTO for the case where forget() is called after start() (This case now returns XAER_NOTA with the patch) My understanding is that Informix also returns XAER_PROTO for both cases but I have not confirmed that myself. Tiago found Postgress returned XAER_NOTA in both cases. A couple things to note when trying to sort out the spec and our behavior. 1) There are no positive cases for forget() with Derby I think. We do not have a method for heuristic completion, so it is just a matter of which error to throw. In the xa spec under xa_fprget() it says: XAER_NOTA - The specified XID is not known by the resource manager as a heuristically completed XID. XAER_PROTO - The routine was invoked in an improper context. 2) For the original case on which this Jira was based, the user was trying to write a generic recovery program which would forget transactions that had been heuristically completed and rollback if they were not heuristically completed but rather still in a prepared state. He would first try the forget() and then if that gave an XAER_PROTO would attempt the rollback(). The workaround we gave was to also check for XAER_NOTA. 3) It is interesting to note that for other calls XAER_NOTA has a different description. XAER_NOTA - The specified XID is not known by the resource manager. I wish I understood some more about heuristic completion but tend to think that since the only way to differentiate a prepared vs heuristically completed transaction returned by recover() is to call forget, the user was not really calling it in an improper context and someone went out of their way to put that extra condition on the XAER_NOTA exception desription for forget(), so my somewhat uneasy opinion is that XAER_NOTA is most appropriate whenever forget() is called with Derby. Opinions?
        Hide
        Tiago R. Espinha added a comment -

        Me and Kathey have been testing the repro in this issue under both PostgreSQL and DB2. As it turns out, PostgreSQL fails and DB2 passes. The failure I get on PostgreSQL reads as follows:
        FAIL: got unexpected exception Heuristic commit/rollback not supported errorCode: -4 calling forget on a prepared transaction

        It is throwing an error code -4. If we check the XAException class we can see that the error being thrown is actually an XAER_NOTA as opposed to the expected XAER_PROTO.

        However, per Kathey's testing, we do get XAER_PROTO on DB2.

        Show
        Tiago R. Espinha added a comment - Me and Kathey have been testing the repro in this issue under both PostgreSQL and DB2. As it turns out, PostgreSQL fails and DB2 passes. The failure I get on PostgreSQL reads as follows: FAIL: got unexpected exception Heuristic commit/rollback not supported errorCode: -4 calling forget on a prepared transaction It is throwing an error code -4. If we check the XAException class we can see that the error being thrown is actually an XAER_NOTA as opposed to the expected XAER_PROTO. However, per Kathey's testing, we do get XAER_PROTO on DB2.
        Hide
        Myrna van Lunteren added a comment -

        I messed up my comment. I had modified the .out to check that it would make the test pass, and instead of from the original .out, I pasted my comment using the modified one.

        So....
        xaSimpleNegative has 2 cases of using of xa_forget:
        1:
        ij(XA)> – start one
        xa_start xa_noflags 1;
        ij(XA)> – ERROR: can only forget heuristically completed transaction
        xa_forget 1;
        IJ ERROR: XAER_PROTO <-- returns XAER_NOTA after the patch

        2.:
        ij(XA)> xa_prepare 1;
        ij(XA)> – can only forget heuristically completed transaction
        xa_forget 1;
        IJ ERROR: XAER_NOTA <-- returns XAER_PROTO after the patch

        I'm sorry for the confusion.
        So, based on this, I think the result after the patch makes perfect sense based on the change the patch does...But are both correct?

        Show
        Myrna van Lunteren added a comment - I messed up my comment. I had modified the .out to check that it would make the test pass, and instead of from the original .out, I pasted my comment using the modified one. So.... xaSimpleNegative has 2 cases of using of xa_forget: 1: ij(XA)> – start one xa_start xa_noflags 1; ij(XA)> – ERROR: can only forget heuristically completed transaction xa_forget 1; IJ ERROR: XAER_PROTO <-- returns XAER_NOTA after the patch 2.: ij(XA)> xa_prepare 1; ij(XA)> – can only forget heuristically completed transaction xa_forget 1; IJ ERROR: XAER_NOTA <-- returns XAER_PROTO after the patch I'm sorry for the confusion. So, based on this, I think the result after the patch makes perfect sense based on the change the patch does...But are both correct?
        Hide
        Tiago R. Espinha added a comment -

        I checked the specification that Kathey posted and here's what it has to say about forgetting transactions:
        [XAER_NOTA]
        The specifiedXIDis not knownbythe resource manager as a heuristically
        completedXID.

        and

        [XAER_PROTO]
        The routine was invoked in an improper context. See Chapter 6 for details.

        On further reading, it says the following:
        If a resource manager does not recognise ∗xid, the function fails, returning
        [XAER_NOTA].

        So, I think this is pretty clear that XAER_NOTA is specifically for when we attempt to do an operation on a transaction that is not recognized by the resource manager. Therefore, I think that as long as the transaction exists, the xa_forget call should always return XAER_PROTO in the event of an exception.

        Show
        Tiago R. Espinha added a comment - I checked the specification that Kathey posted and here's what it has to say about forgetting transactions: [XAER_NOTA] The specifiedXIDis not knownbythe resource manager as a heuristically completedXID. and [XAER_PROTO] The routine was invoked in an improper context. See Chapter 6 for details. On further reading, it says the following: If a resource manager does not recognise ∗xid, the function fails, returning [XAER_NOTA] . So, I think this is pretty clear that XAER_NOTA is specifically for when we attempt to do an operation on a transaction that is not recognized by the resource manager. Therefore, I think that as long as the transaction exists, the xa_forget call should always return XAER_PROTO in the event of an exception.
        Hide
        Kathey Marsden added a comment -

        I have not looked closely at this issue or the XA spec in a long time, but my gut instinct is that we should still return XAER_PROTO for this case:
        (XA)> xa_prepare 1;
        ij(XA)> – can only forget heuristically completed transaction
        xa_forget 1;
        IJ ERROR: XAER_PROTO <--returns XAER_NOTA after patch

        The new behavior seems to contradict the description of the issue: javax.transaction.xa.forget (Xid) raises XAER_NOTA exception instead of XAERR_PROTO on a prepared transaction

        and in general you would think that if the transaction exists you shouldn't get XAER_NOTA (no transaction)

        Please consult the XA Spec to clarify what the behavior should be.
        http://www.opengroup.org/publications/catalog/c193.htm

        Show
        Kathey Marsden added a comment - I have not looked closely at this issue or the XA spec in a long time, but my gut instinct is that we should still return XAER_PROTO for this case: (XA)> xa_prepare 1; ij(XA)> – can only forget heuristically completed transaction xa_forget 1; IJ ERROR: XAER_PROTO <--returns XAER_NOTA after patch The new behavior seems to contradict the description of the issue: javax.transaction.xa.forget (Xid) raises XAER_NOTA exception instead of XAERR_PROTO on a prepared transaction and in general you would think that if the transaction exists you shouldn't get XAER_NOTA (no transaction) Please consult the XA Spec to clarify what the behavior should be. http://www.opengroup.org/publications/catalog/c193.htm
        Hide
        Tiago R. Espinha added a comment -

        If I understood this properly, XA.forget does return different exceptions depending on the status of the transaction. With this in mind, we probably need to change one occurrence of the XA.forget in the harness test to accommodate for the fact that the exception being thrown for this specific case is now different.

        Is this correct?

        Show
        Tiago R. Espinha added a comment - If I understood this properly, XA.forget does return different exceptions depending on the status of the transaction. With this in mind, we probably need to change one occurrence of the XA.forget in the harness test to accommodate for the fact that the exception being thrown for this specific case is now different. Is this correct?
        Hide
        Myrna van Lunteren added a comment -

        I forgot to say - the test case in xaTransactionTest looks good.

        Show
        Myrna van Lunteren added a comment - I forgot to say - the test case in xaTransactionTest looks good.
        Hide
        Myrna van Lunteren added a comment -

        Thanks Tiago for your email to the list indicating you ran suites.All successfully.
        However, I thought it prudent to run derbyall (with ibm16 and sane classes), and it popped one new test failure (apart from a failure in unit/T_RawStoreFactory.unit, similar to the closed DERBY-654, which we can ignore here because I often get it on my machine with this configuration):
        jdbcapi/xaSimpleNegative.sql. This test apparently already had a test for the XaResource.forget...
        And as expected, it now shows XAER_PROTO instead of XAER_NOTA.
        However, it has another test case of XAResource.forget, and that now returns XAER_NOTA instead of XAER_PROTO

        Looking at the code change, this is logical, but I'm just not sure that we're doing the right thing.
        Should both cases return XAER_PROTO?

        I looked at the diff for a while, but would like to get the opinion of someone who understands/knows the XA specs better....
        This is the flip/flop section:
        -------------------------------------------------
        ij(XA)> – start one
        xa_start xa_noflags 1;
        ij(XA)> – ERROR: can only forget heuristically completed transaction
        xa_forget 1;
        IJ ERROR: XAER_NOTA <---returns XAER_PROTO after patch
        ij(XA)> delete from APP.negative;
        1 row inserted/updated/deleted
        ij(XA)> xa_end xa_success 1;
        ij(XA)> – ERROR: now try some bad flag
        xa_start xa_suspend 1;
        IJ ERROR: XAER_INVAL
        ij(XA)> – ERROR: now try some bad flag
        xa_start xa_fail 1;
        IJ ERROR: XAER_INVAL
        ij(XA)> xa_prepare 1;
        ij(XA)> – can only forget heuristically completed transaction
        xa_forget 1;
        IJ ERROR: XAER_PROTO <--returns XAER_NOTA after patch
        --------------------------------------------------------------

        Show
        Myrna van Lunteren added a comment - Thanks Tiago for your email to the list indicating you ran suites.All successfully. However, I thought it prudent to run derbyall (with ibm16 and sane classes), and it popped one new test failure (apart from a failure in unit/T_RawStoreFactory.unit, similar to the closed DERBY-654 , which we can ignore here because I often get it on my machine with this configuration): jdbcapi/xaSimpleNegative.sql. This test apparently already had a test for the XaResource.forget... And as expected, it now shows XAER_PROTO instead of XAER_NOTA. However, it has another test case of XAResource.forget, and that now returns XAER_NOTA instead of XAER_PROTO Looking at the code change, this is logical, but I'm just not sure that we're doing the right thing. Should both cases return XAER_PROTO? I looked at the diff for a while, but would like to get the opinion of someone who understands/knows the XA specs better.... This is the flip/flop section: ------------------------------------------------- ij(XA)> – start one xa_start xa_noflags 1; ij(XA)> – ERROR: can only forget heuristically completed transaction xa_forget 1; IJ ERROR: XAER_NOTA <---returns XAER_PROTO after patch ij(XA)> delete from APP.negative; 1 row inserted/updated/deleted ij(XA)> xa_end xa_success 1; ij(XA)> – ERROR: now try some bad flag xa_start xa_suspend 1; IJ ERROR: XAER_INVAL ij(XA)> – ERROR: now try some bad flag xa_start xa_fail 1; IJ ERROR: XAER_INVAL ij(XA)> xa_prepare 1; ij(XA)> – can only forget heuristically completed transaction xa_forget 1; IJ ERROR: XAER_PROTO <--returns XAER_NOTA after patch --------------------------------------------------------------
        Hide
        Tiago R. Espinha added a comment -

        It is possible that the upgrade tests were skipped. I didn't use the derbyTesting.oldReleasePath but network trouble isn't completely out of the picture. I will give it another spin today with derby.tests.trace=true and redirecting the output to a file, then maybe I can see which ones were skipped.

        I'm just worried that this might be a Mac issue, as Macs run Apple's own JDK. In the meanwhile I'm doing my test runs on my trustworthy Ubuntu VM, just to be safe.

        Show
        Tiago R. Espinha added a comment - It is possible that the upgrade tests were skipped. I didn't use the derbyTesting.oldReleasePath but network trouble isn't completely out of the picture. I will give it another spin today with derby.tests.trace=true and redirecting the output to a file, then maybe I can see which ones were skipped. I'm just worried that this might be a Mac issue, as Macs run Apple's own JDK. In the meanwhile I'm doing my test runs on my trustworthy Ubuntu VM, just to be safe.
        Hide
        Myrna van Lunteren added a comment -

        Tiago, maybe the upgrade tests, or not all of them, did get run? Did you maybe run with -DderbyTesting.oldReleasePath set to an old directory? Or perhaps you didn't run with it set and there was a problem getting to 'http://svn.apache.org/repos/asf/db/derby/jars'? (like, either that http server was down or you have some sort of firewall protection or had network trouble).

        Show
        Myrna van Lunteren added a comment - Tiago, maybe the upgrade tests, or not all of them, did get run? Did you maybe run with -DderbyTesting.oldReleasePath set to an old directory? Or perhaps you didn't run with it set and there was a problem getting to 'http://svn.apache.org/repos/asf/db/derby/jars'? (like, either that http server was down or you have some sort of firewall protection or had network trouble).
        Hide
        Ravinder Reddy added a comment -

        Sure , I will be back !

        Show
        Ravinder Reddy added a comment - Sure , I will be back !
        Hide
        Tiago R. Espinha added a comment -

        I'm attaching a fixed patch. I've also ran suites.All with no failures. However, I ran suites.All under Mac OS X and I was surprised to get a test count that was inferior to 10k tests. If I recall correctly, my Windows and Linux runs always have around 12k tests - is there a logical explanation for this? And is it concerning?

        Show
        Tiago R. Espinha added a comment - I'm attaching a fixed patch. I've also ran suites.All with no failures. However, I ran suites.All under Mac OS X and I was surprised to get a test count that was inferior to 10k tests. If I recall correctly, my Windows and Linux runs always have around 12k tests - is there a logical explanation for this? And is it concerning?
        Hide
        Kathey Marsden added a comment -

        Hi Tiago,
        Before you start your suites.All run. The test should use the J2EEDataSource class and run with client too.

        Also I think the arguments in the assertEquals are reversed.
        XAeForget.errorCode, XAException.XAER_PROTO

        The expected code should come first.

        Show
        Kathey Marsden added a comment - Hi Tiago, Before you start your suites.All run. The test should use the J2EEDataSource class and run with client too. Also I think the arguments in the assertEquals are reversed. XAeForget.errorCode, XAException.XAER_PROTO The expected code should come first.
        Hide
        Tiago R. Espinha added a comment -

        I'm attaching a second patch that adds a new fixture to XATransaction to test for the existence of this bug. With Ravinder's patch, the fixture passes correctly. I'll be running suites.All with both patches on.

        Show
        Tiago R. Espinha added a comment - I'm attaching a second patch that adds a new fixture to XATransaction to test for the existence of this bug. With Ravinder's patch, the fixture passes correctly. I'll be running suites.All with both patches on.
        Hide
        Kathey Marsden added a comment -

        Ravinder told me offline that he would not be able to work on this for a while. Unassigning so someone else can finish it off. I think the code change is ok. We just need a new test.

        Ravinder, I hope you will be able to come back in the near future and pick up another issue.

        Show
        Kathey Marsden added a comment - Ravinder told me offline that he would not be able to work on this for a while. Unassigning so someone else can finish it off. I think the code change is ok. We just need a new test. Ravinder, I hope you will be able to come back in the near future and pick up another issue.
        Hide
        Rick Hillegas added a comment -

        Triaged for 10.5.3: assigned normal urgency and noted that a repro is attached.

        Show
        Rick Hillegas added a comment - Triaged for 10.5.3: assigned normal urgency and noted that a repro is attached.
        Hide
        Ravinder Reddy added a comment -

        Hi Kathey,

        I think I can complete by then.

        thank you

        Show
        Ravinder Reddy added a comment - Hi Kathey, I think I can complete by then. thank you
        Hide
        Kathey Marsden added a comment -

        Hi Ravinder,

        I just wanted to check in on this issue. It would be great to get it committed by July 14,our target release candidate date. Do you think you will have time to work on it between now and then?

        Thanks

        Kathey

        Show
        Kathey Marsden added a comment - Hi Ravinder, I just wanted to check in on this issue. It would be great to get it committed by July 14,our target release candidate date. Do you think you will have time to work on it between now and then? Thanks Kathey
        Hide
        Kathey Marsden added a comment -

        Hi Ravinder,

        I think the code change looks ok. Please add a test to XATransactionTest. You should also run XATest (currently not in a suite DERBY-4155) to make sure there are no issues associated with the change and let us know the results of your Suites.All and derbyall runs.

        Thanks

        Kathey

        Show
        Kathey Marsden added a comment - Hi Ravinder, I think the code change looks ok. Please add a test to XATransactionTest. You should also run XATest (currently not in a suite DERBY-4155 ) to make sure there are no issues associated with the change and let us know the results of your Suites.All and derbyall runs. Thanks Kathey
        Hide
        Ravinder Reddy added a comment -

        As pointed out by Kathey , It seems that the error lies in the conditional evaluation of tranState.isPrepared in EmbedXAResource.forget().
        Attached is the first/quick/small patch to it , please review and give your comments.
        Thank you

        Show
        Ravinder Reddy added a comment - As pointed out by Kathey , It seems that the error lies in the conditional evaluation of tranState.isPrepared in EmbedXAResource.forget(). Attached is the first/quick/small patch to it , please review and give your comments. Thank you
        Hide
        Ravinder Reddy added a comment -

        Thanks Kathey

        Show
        Ravinder Reddy added a comment - Thanks Kathey
        Hide
        Kathey Marsden added a comment -

        My bad, I neglected to attach this file.

        Show
        Kathey Marsden added a comment - My bad, I neglected to attach this file.
        Hide
        Ravinder Reddy added a comment - - edited

        I am getting the following error when I try to reproduce the error for DERBY-1016

        ReproDerby1016.java:26: cannot find symbol
        symbol : class utilXid
        location: class ReproDerby1016
        Xid xid = new utilXid(1,93,18);
        ^
        1 error

        More Info about the Derby Setup:

        I have checked-out trunk source in windows machine and built it using Ant.
        I have created a lib folder in home directory and copied all jars (jars/sane/*.jar) to lib and set the DERBY_HOME , path , classpath accordingly.
        I have put classes folder also in classpath.

        what might be the Error.?

        Show
        Ravinder Reddy added a comment - - edited I am getting the following error when I try to reproduce the error for DERBY-1016 ReproDerby1016.java:26: cannot find symbol symbol : class utilXid location: class ReproDerby1016 Xid xid = new utilXid(1,93,18); ^ 1 error More Info about the Derby Setup: I have checked-out trunk source in windows machine and built it using Ant. I have created a lib folder in home directory and copied all jars (jars/sane/*.jar) to lib and set the DERBY_HOME , path , classpath accordingly. I have put classes folder also in classpath. what might be the Error.?
        Hide
        Kathey Marsden added a comment -

        This appears to be a problem for both embedded and network server. Attached is a reproduction. As is it runs with embedded but you can comment/'uncomment a few lines to run with Client.

        There is this highly suspect code in EmbedXAResource.forget() which may be relevant:

        throw new XAException(tranState.isPrepared
        ? XAException.XAER_NOTA
        : XAException.XAER_PROTO);

        Show
        Kathey Marsden added a comment - This appears to be a problem for both embedded and network server. Attached is a reproduction. As is it runs with embedded but you can comment/'uncomment a few lines to run with Client. There is this highly suspect code in EmbedXAResource.forget() which may be relevant: throw new XAException(tranState.isPrepared ? XAException.XAER_NOTA : XAException.XAER_PROTO);
        Hide
        Mike Matrigali added a comment -

        I took a quick look at the store implementation of forget() which is in
        opensource/java/engine/org/apache/derby/impl/store/raw/xact/XactXAResourceManager.java!forget().
        It turns out in this store implementation of XA there is never such a thing as a
        heuristically completed transaction, so the implementation just ALWAYS throws
        throw StandardException.newException(
        SQLState.STORE_XA_PROTOCOL_VIOLATION);

        I would have assumed that this would have been mapped to PROTO.

        I also seem to remember that in runtime the jdbc layer maintains the XA xid list, so it may be doing a
        lookup before calling store, to do the forget.

        The problem must be in the error mapping from store error to jdbc error. I am not sure but I believe this mapping
        may be different in network vs. embedded, so it ism portant for the bug reporter to note which system he is using. My
        guess would be embedded , but that is only a guess.

        Anyone know where the mapping for these errors is?

        Show
        Mike Matrigali added a comment - I took a quick look at the store implementation of forget() which is in opensource/java/engine/org/apache/derby/impl/store/raw/xact/XactXAResourceManager.java!forget(). It turns out in this store implementation of XA there is never such a thing as a heuristically completed transaction, so the implementation just ALWAYS throws throw StandardException.newException( SQLState.STORE_XA_PROTOCOL_VIOLATION); I would have assumed that this would have been mapped to PROTO. I also seem to remember that in runtime the jdbc layer maintains the XA xid list, so it may be doing a lookup before calling store, to do the forget. The problem must be in the error mapping from store error to jdbc error. I am not sure but I believe this mapping may be different in network vs. embedded, so it ism portant for the bug reporter to note which system he is using. My guess would be embedded , but that is only a guess. Anyone know where the mapping for these errors is?
        Hide
        Daniel John Debrunner added a comment -

        Reading the javadoc for XAResource I think I agree with you, XAER_PROTO is the correct error.

        XAResource.recover()

        "The transaction manager calls this method during recovery to obtain the list of transaction branches that are currently in prepared or heuristically completed states."

        XAResource.forget

        "Tells the resource manager to forget about a heuristically completed transaction branch."

        Thus a prepared transaction is not a heuristically completed transaction branch and therefore is not a valid state for forget.

        Show
        Daniel John Debrunner added a comment - Reading the javadoc for XAResource I think I agree with you, XAER_PROTO is the correct error. XAResource.recover() "The transaction manager calls this method during recovery to obtain the list of transaction branches that are currently in prepared or heuristically completed states." XAResource.forget "Tells the resource manager to forget about a heuristically completed transaction branch." Thus a prepared transaction is not a heuristically completed transaction branch and therefore is not a valid state for forget.

          People

          • Assignee:
            Jayaram Subramanian
            Reporter:
            Kathey Marsden
          • Votes:
            1 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development