-
Notifications
You must be signed in to change notification settings - Fork 14
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
Improves transaction rollback handling #395
Conversation
orville-postgresql/src/Orville/PostgreSQL/Execution/Transaction.hs
Outdated
Show resolved
Hide resolved
orville-postgresql/src/Orville/PostgreSQL/Execution/Transaction.hs
Outdated
Show resolved
Hide resolved
orville-postgresql/src/Orville/PostgreSQL/Execution/Transaction.hs
Outdated
Show resolved
Hide resolved
orville-postgresql/src/Orville/PostgreSQL/Execution/Transaction.hs
Outdated
Show resolved
Hide resolved
orville-postgresql/src/Orville/PostgreSQL/Execution/Transaction.hs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favor of this change in general. I agree with @telser re: combining the concepts of Orville's transaction state and LibPQ's and handling each LibPQ state explicitly.
orville-postgresql/src/Orville/PostgreSQL/Execution/Transaction.hs
Outdated
Show resolved
Hide resolved
orville-postgresql/src/Orville/PostgreSQL/Execution/Transaction.hs
Outdated
Show resolved
Hide resolved
I added exception handling around the success event SQL execution in `finishTransaction` in `withTransaction`. If a `SqlExecutionError` occurs, it will run the rollback callback and rethrow the exception. I also added libpq transaction status checks to `beginTransaction` and `finishTransaction` in `withTransaction`. If the status is unexpected for a given transaction event, `withTransaction` will throw an `UnexpectedTransactionStatusError`.
a6c350e
to
e08919c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going to be a good backstop against connections being in unexpected states. Under usual circumstances that shouldn't happen, but great to have the handling.
I added exception handling around the success event SQL execution in
finishTransaction
inwithTransaction
. If aSqlExecutionError
occurs, it will run the rollback callback and rethrow the exception.I also added libpq transaction status checks to
beginTransaction
andfinishTransaction
inwithTransaction
. If the status is unexpected for a given transaction event,withTransaction
will throw anUnexpectedTransactionStatusError
.