Prevent returning connections to pool with a positive transaction depth #1283
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mark transaction as closed *only* after commit/rollback succeeds. Previously, `open` on the transaction would be set to `false` prior to attempting to commit or rollback the transaction. When the operation failed, for example, due to a serialization failure with a serializable isolation level, this would leave the transaction in an inconsistent state, where it thought it was closed but really it was still open. The connection would then be returned to the connection pool with a transaction depth of 1, causing a savepoint to be erroneously created the next time a transaction was created for the connection. By waiting to set `open` to `false` until the commit/rollback succeeds, a failure to do either will result in us correctly rolling back the transaction when dropping it, ensuring that the connection is returned to the pool with a transaction depth of 0. Note that this is consistent with how `sqlx` handles transactions. We attempted to write a test, but had a very difficult time forcing postgres to fail to commit a transaction. We found that it would block our requests instead when creating conflicting updates, and we couldn't find any information about when it blocks vs when transaction commits fail. Co-Authored-By: Nathan Sobo <nathan@zed.dev>
billy1624
approved these changes
Dec 16, 2022
There was a problem hiding this comment.
Hey @as-cii @nathansobo sorry for the delay. Thanks for the investigation!!
I'll fix the clippy warning :)
|
Sorry for the late reply... |
|
Awesome! Thank you everyone. |
billy1624
pushed a commit
that referenced
this pull request
Dec 23, 2022
…th (#1283) Mark transaction as closed *only* after commit/rollback succeeds. Previously, `open` on the transaction would be set to `false` prior to attempting to commit or rollback the transaction. When the operation failed, for example, due to a serialization failure with a serializable isolation level, this would leave the transaction in an inconsistent state, where it thought it was closed but really it was still open. The connection would then be returned to the connection pool with a transaction depth of 1, causing a savepoint to be erroneously created the next time a transaction was created for the connection. By waiting to set `open` to `false` until the commit/rollback succeeds, a failure to do either will result in us correctly rolling back the transaction when dropping it, ensuring that the connection is returned to the pool with a transaction depth of 0. Note that this is consistent with how `sqlx` handles transactions. We attempted to write a test, but had a very difficult time forcing postgres to fail to commit a transaction. We found that it would block our requests instead when creating conflicting updates, and we couldn't find any information about when it blocks vs when transaction commits fail. Co-Authored-By: Nathan Sobo <nathan@zed.dev> Co-authored-by: Nathan Sobo <nathan@zed.dev>
|
Appreciate your merge and hard work on this project. It's been very helpful to have the abstraction so we can talk to Postgres in production and SQLite in tests. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mark transaction as closed only after commit/rollback succeeds.
Previously,
openon the transaction would be set tofalseprior to attempting to commit or rollback the transaction. When the operation failed, for example, due to a serialization failure with a serializable isolation level, this would leave the transaction in an inconsistent state, where it thought it was closed but really it was still open. The connection would then be returned to the connection pool with a transaction depth of 1, causing a savepoint to be erroneously created the next time a transaction was created for the connection.By waiting to set
opentofalseuntil the commit/rollback succeeds, a failure to do either will result in us correctly rolling back the transaction when dropping it, ensuring that the connection is returned to the pool with a transaction depth of 0. Note that this is consistent with howsqlxhandles transactions.We attempted to write a test, but had a very difficult time forcing postgres to fail to commit a transaction. We found that it would block our requests instead when creating conflicting updates, and we couldn't find any information about when it blocks vs when transaction commits fail.