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.

Sign up for GitHub

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

Jump to bottom

fix: map integrity constraint violation to XA_RBINTEGRITY instead of XAER_RMFAIL #1175

Merged
merged 6 commits into from
Jul 14, 2018
Merged

fix: map integrity constraint violation to XA_RBINTEGRITY instead of XAER_RMFAIL #1175

merged 6 commits into from
Jul 14, 2018

Conversation

janvdbergh
Copy link
Contributor

Fix for issue #1171.

@codecov-io
Copy link

codecov-io commented Apr 23, 2018 •
edited
Loading

Codecov Report

Merging #1175 into master will increase coverage by 0.01%.
The diff coverage is 75%.

@@             Coverage Diff              @@
##             master    #1175      +/-   ##
============================================
+ Coverage     68.71%   68.73%   +0.01%     
- Complexity     3850     3858       +8     
============================================
  Files           174      174              
  Lines         16035    16048      +13     
  Branches       2614     2617       +3     
============================================
+ Hits          11019    11030      +11     
- Misses         3780     3781       +1     
- Partials       1236     1237       +1

@davecramer
Copy link
Member

I'm wondering if there is a decent way to test this ?

@janvdbergh
Copy link
Contributor Author

Do you mean manually or automated?
I manually tested the changes by deliberately adding a deferrable foreign key constraint to my database schema, causing it fo fail during commit. After the change the distributed transaction manager (Atomikos) could determine the outcome of the transaction and properly respond.
To test this automatically you would need a similar schema in your database and run a distributed transaction against it. The outcome should be an XAException indicating that the transaction was rolled back. However it might take a lot of work to set this up.

@janvdbergh
Copy link
Contributor Author

@davecramer Do you still see issues before this pull request can be merged? Let me know if anything has to be adapted.

@davecramer
Copy link
Member

@janvdbergh no I see no reason not to merge it.
@vlsi ?

vlsi
vlsi requested changes May 7, 2018
View reviewed changes
Copy link
Member

@vlsi vlsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still wonder if you could replicate the issue with something like

pgjdbc/pgjdbc/src/test/java/org/postgresql/test/xa/XADataSourceTest.java

Line 50 in e5aab1c

public XADataSourceTest() {

At least constraint violation should not be that hard to replicate.

pgjdbc/src/main/java/org/postgresql/xa/PGXAConnection.java Outdated
}

private boolean isPostgreSQLIntegrityConstraintViolation(SQLException sqlException) {
return sqlException instanceof PSQLException && sqlException.getSQLState().matches("23\\d{3}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While regexp looks clever, it is actually specified to mean that? What if the code clashes somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Replaced it with a non-regex version.

pgjdbc/src/main/java/org/postgresql/xa/PGXAConnection.java
@@ -584,6 +584,18 @@ public boolean setTransactionTimeout(int seconds) {
return false;
}

private int mapSQLStateToXAErrorCode(SQLException sqlException) {
if (isPostgreSQLIntegrityConstraintViolation(sqlException)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why integrity constraint violation exceptions only are treated special?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These exceptions can be mapped to the correct XA Error Code. When Postgres reported an integrity constraint violation that means the change has been rolled back and can be reported as such. We can not make the same claim for other exceptions.

@janvdbergh
Copy link
Contributor Author

@vlsi I made the requested change and added a test to validate the error code mapping. Any other comments?

@janvdbergh
Copy link
Contributor Author

@vlsi Can this branch be merged or is there something else to do?
Or should I just close and reopen the pull request?

vlsi
vlsi reviewed Jul 14, 2018
View reviewed changes
pgjdbc/src/main/java/org/postgresql/xa/PGXAConnection.java Outdated
@@ -584,6 +584,20 @@ public boolean setTransactionTimeout(int seconds) {
return false;
}

private int mapSQLStateToXAErrorCode(SQLException sqlException) {
if (isPostgreSQLIntegrityConstraintViolation(sqlException)) {
return XAException.XA_RBROLLBACK;
Copy link
Member

@vlsi vlsi Jul 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janvdbergh , what do you think of using XA_RBINTEGRITY instead?

A condition that violates the integrity of the resource was detected

https://docs.oracle.com/javaee/6/api/javax/transaction/xa/XAException.html#XA_RBINTEGRITY

@vlsi vlsi added this to the 42.2.4 milestone Jul 14, 2018
@vlsi vlsi changed the title Correctly map integrity violations to XA error codes. fix: map integrity constraint violation to XA_RBINTEGRITY instead of XAER_RMFAIL Jul 14, 2018
@vlsi vlsi merged commit f2d1352 into pgjdbc:master Jul 14, 2018
@davecramer davecramer added this to In progress in 42.2.4 Release Jul 14, 2018
@davecramer davecramer moved this from In progress to Done in 42.2.4 Release Jul 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vlsi vlsi vlsi approved these changes

@davecramer davecramer davecramer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
42.2.4 Release
  
Done
Milestone
42.2.4
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@janvdbergh @codecov-io @davecramer @vlsi