Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust XAException return codes for better compatibility with XA specification #782

Merged
merged 7 commits into from Mar 11, 2018

Conversation

@ochaloup
Copy link
Contributor

@ochaloup ochaloup commented Mar 15, 2017

This commit fixes error codes which are thrown from PostgreSQL
XAResource jdbc driver to comply better with xa specification.

There are three main adjustments

  • XAException.XAER_RMFAIL is used when some connection error happens
    and it is expected that reconnection of RM could occur
  • XAException.XAER_NOTA is used when RM does not know anything about the
    provided Xid
  • XAException.XAER_PROTO is used when some wrong sequence of method
    calls is invoked

This addresses issues
#236
#510
#683

@codecov-io
Copy link

@codecov-io codecov-io commented Mar 15, 2017

Codecov Report

Merging #782 into master will increase coverage by 0.08%.
The diff coverage is 43.07%.

@@             Coverage Diff              @@
##             master     #782      +/-   ##
============================================
+ Coverage     67.38%   67.47%   +0.08%     
- Complexity     3687     3710      +23     
============================================
  Files           170      170              
  Lines         15638    15681      +43     
  Branches       2531     2552      +21     
============================================
+ Hits          10538    10581      +43     
+ Misses         3917     3914       -3     
- Partials       1183     1186       +3
@ochaloup ochaloup force-pushed the ochaloup:incorrect-return-codes branch from f6d333e to b9fa629 Mar 21, 2017
@ochaloup
Copy link
Contributor Author

@ochaloup ochaloup commented Mar 21, 2017

fixed forgotten checkstyle failures

@davecramer
Copy link
Member

@davecramer davecramer commented Mar 22, 2017

So short of any way to really test this I'm inclined to +1

@ochaloup
Copy link
Contributor Author

@ochaloup ochaloup commented Mar 28, 2017

@davecramer thanks for looking at this!
I will try to run our tests with these changes this week. I will keep you updated.

@ochaloup ochaloup force-pushed the ochaloup:incorrect-return-codes branch from b9fa629 to dccbe28 Jan 6, 2018
@ochaloup
Copy link
Contributor Author

@ochaloup ochaloup commented Jan 6, 2018

hi @davecramer , I've got back to this. I re-based the changes and the CI tests seem to pass. Then I built the driver with this changes and run WildFly Narayana integration testsuite with the driver and PostgreSQL 9.3 and 9.4 and all passed.

Would that be ok to merge this PR or is there something more to do/check?

@ochaloup
Copy link
Contributor Author

@ochaloup ochaloup commented Jan 8, 2018

cc to @vlsi and @whitingjr as we were discussing the issue #510 which this PR covers

@davecramer
Copy link
Member

@davecramer davecramer commented Jan 8, 2018

@ochaloup long week :)

If nobody else has objections I'm fine with it

@whitingjr
Copy link
Contributor

@whitingjr whitingjr commented Jan 16, 2018

@davecramer I've looked over the changes. They look ok.

@vlsi vlsi force-pushed the pgjdbc:master branch from 4a6d1d1 to 5819f31 Jan 17, 2018
@vlsi vlsi added this to the 42.2.1 milestone Jan 18, 2018
if (PSQLState.isConnectionError(ex.getSQLState())) {
errorCode = XAException.XAER_RMFAIL;
}
throw new PGXAException(GT.tr("Error rolling back prepared transaction"), ex, errorCode);

This comment has been minimized.

@vlsi

vlsi Jan 23, 2018
Member

@ochaloup , I might be a bit late to the party, however what do you think of printing things like xid, preparedXid, committedOrRolledBack in the error message?

I'm sure, that would be crucial information if somebody receives ""Error rolling back prepared transaction".

This comment has been minimized.

@ochaloup

ochaloup Jan 25, 2018
Author Contributor

@vlsi : I agree. Just I was not sure if I can change the format as those information was not part of the messages previously. I'm going to add the proposed info to the messages, push it to the PR and please check that then again.

@@ -442,18 +464,22 @@ private void commitOnePhase(Xid xid) throws XAException {
"Not implemented: one-phase commit must be issued using the same connection that was used to start it"),
XAException.XAER_RMERR);
}
if (!xid.equals(currentXid) || committedOrRolledBack) {
throw new PGXAException(GT.tr("One-phase commit with unknown xid "), XAException.XAER_NOTA);

This comment has been minimized.

@vlsi

vlsi Jan 23, 2018
Member

space at the end of string

@vlsi
Copy link
Member

@vlsi vlsi commented Jan 23, 2018

@ochaloup , the changes look ok, however, I think it makes sense to add xids (and other relevant state information) to the error messages. Otherwise the error messages are cryptic and it would be hard to analyze.

@ochaloup ochaloup force-pushed the ochaloup:incorrect-return-codes branch from dccbe28 to 677ffb0 Jan 25, 2018
@ochaloup
Copy link
Contributor Author

@ochaloup ochaloup commented Jan 25, 2018

@vlsi can you please review the current form. I changed some of the error messages, added few debug information printing. Would be fine this way? Does have no harm for translation?

@vlsi vlsi modified the milestones: 42.2.1, 42.2.2 Jan 25, 2018
@ochaloup ochaloup force-pushed the ochaloup:incorrect-return-codes branch from 677ffb0 to 8c5ec76 Jan 27, 2018
@ochaloup
Copy link
Contributor Author

@ochaloup ochaloup commented Jan 27, 2018

I haven't recognized that I need to run manually mvn checkstyle:check. Seems the issue is in the forgotten import. I hope that will be fine this time.

// In fact, we don't know if xid is bogus, or if it just wasn't associated with this
// connection.
if (xid.equals(preparedXid)) {
throw new PGXAException(GT.tr("One-phase commit called for xid {0} but connection was prepared with xid {1}",

This comment has been minimized.

@vlsi

vlsi Jan 27, 2018
Member

@ochaloup , Did you really mean xid.equals(preparedXid) above?
I think the negation is missing. Can you please clarify the logic here?

@@ -195,12 +197,12 @@ public void start(Xid xid, int flags) throws XAException {
// It's ok to join an ended transaction. WebLogic does that.
if (flags == TMJOIN) {
if (state != State.ENDED) {
throw new PGXAException(GT.tr("Transaction interleaving not implemented"),
throw new PGXAException(GT.tr("Transaction interleaving not implemented. xid={0}, currentXid={1}", xid, currentXid),

This comment has been minimized.

@vlsi

vlsi Jan 28, 2018
Member

Just a note: exception message does not reflect the cause. Something like "invalid state" should be used.

This comment has been minimized.

@ochaloup

ochaloup Feb 2, 2018
Author Contributor

@vlsi thank you for your fixes. I haven't managed to understand how the call should be compound correctly (eating humble pie).

In the case of the message - do you expect me to change the message? I can only guess what is the purpose. Do you want to change the message to something like Start called with transaction join flag while transaction is still active. Transaction interleaving not implemented..., or...?

This comment has been minimized.

@whitingjr

whitingjr Feb 19, 2018
Contributor

@vlsi Does the exception message still need changing to "invalid state" ? Or something similar ?

This comment has been minimized.

@whitingjr

whitingjr Feb 19, 2018
Contributor

"Invalid protocol state requested. Attempted Transaction interleaving is not supported. xid={0}, currentXid={1}"

This comment has been minimized.

@ochaloup

ochaloup Feb 19, 2018
Author Contributor

@whitingjr thank you. I changed the PR to the text you suggested. I only wonder if that's right to say that the protocol state is invalid.

@ochaloup ochaloup force-pushed the ochaloup:incorrect-return-codes branch 2 times, most recently from c06f31e to c369a64 Feb 19, 2018
ochaloup and others added 3 commits Mar 21, 2017
This commit fixes error codes which are thrown from PostgreSQL
XAResource jdbc driver to comply better with xa specification.

There are three main adjustments
* XAException.XAER_RMFAIL is used when some connection error happens
  and it is expected that reconnection of RM could occur
* XAException.XAER_NOTA is used when RM does not know anything about the
  provided Xid
* XAException.XAER_PROTO is used when some wrong sequence of method
  calls is invoked

This addresses issues
#236
#510
#683
ochaloup and others added 3 commits Feb 19, 2018
@vlsi vlsi changed the title Adjusting XAException return codes for PG XAResource Adjust XAException return codes for better compatibility with XA specification Mar 11, 2018
@vlsi vlsi force-pushed the ochaloup:incorrect-return-codes branch from df2cb3e to 1ca93b4 Mar 11, 2018
@vlsi vlsi merged commit e5aab1c into pgjdbc:master Mar 11, 2018
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018
…ification (pgjdbc#782)

There are three main adjustments
* XAException.XAER_RMFAIL is used when some connection error happens
  and it is expected that reconnection of RM could occur
* XAException.XAER_NOTA is used when RM does not know anything about the
  provided Xid
* XAException.XAER_PROTO is used when some wrong sequence of method
  calls is invoked

Fixes pgjdbc#236, pgjdbc#510, pgjdbc#683
rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018
…ification (pgjdbc#782)

There are three main adjustments
* XAException.XAER_RMFAIL is used when some connection error happens
  and it is expected that reconnection of RM could occur
* XAException.XAER_NOTA is used when RM does not know anything about the
  provided Xid
* XAException.XAER_PROTO is used when some wrong sequence of method
  calls is invoked

Fixes pgjdbc#236, pgjdbc#510, pgjdbc#683
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.