Conversation
`Floating::return_to_pool` validates a connection on release with `Connection::ping`, and the Postgres impl was a bare `wait_until_ready`: it drains the `ReadyForQuery` but never looks at its transaction-status byte. A session sitting inside a transaction block is therefore reported healthy and handed to the next borrower, whose statements run inside it and hold its locks. Two shapes reach that point with a client-side `transaction_depth` of zero, so neither drop guard has queued a `ROLLBACK`: - A future cancelled while `BEGIN`'s round trip is in flight (transact-rs#4393). transact-rs#4394 fixed this one at the source by claiming the depth before the await. - A statement that fails inside a block opened by a multi-statement `raw_sql`, where the block is opened and aborted within a single simple-query message and the depth is never raised at all. This one does not self-heal: the next borrower's `BEGIN` fails because the block is already aborted, leaving the depth at zero again, so every subsequent checkout fails with 25P02 until `max_lifetime` recycles the connection. Check the server-reported status in `ping` and roll back when it disagrees. The status byte is already decoded into `PgConnection::transaction_status` on every `ReadyForQuery`, so the check itself is free, and the `ROLLBACK` is only sent on a connection that is actually dirty. The check is gated on `transaction_depth == 0`. `ping` is public API and a caller may be holding a transaction deliberately; a non-zero depth means the client knows about the block and owns its lifetime, so it is left alone. Only a block the client has no record of is ended here -- which also means there is never a savepoint to restore to, hence a plain `ROLLBACK`. Also exposes `PgConnection::transaction_status`, so pool users can make the same distinction themselves. `Connection::is_in_transaction` reports the client-side depth, which is precisely the value that is wrong in both shapes above, and the server's view was not reachable from outside the crate. Tests: `it_does_not_return_a_connection_inside_a_transaction_to_the_pool` covers both shapes plus the leaked `BEGIN READ ONLY` case, where the damage surfaces as a failing write rather than 25P02, and `it_does_not_roll_back_a_transaction_the_caller_owns` pins the depth guard. The first fails on main with `left: Error, right: Idle`.
This file contains hidden or 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
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.
Follow-up to #4393 / #4394, covering the half of that bug those did not reach.
The problem
Floating::return_to_poolvalidates a connection on release withConnection::ping, and the Postgres impl is a barewait_until_ready:That drains the
ReadyForQuerybut never looks at its transaction-status byte. A session sitting inside a transaction block is therefore reported healthy and handed to the next borrower, whose statements silently run inside it and hold its locks.Two shapes reach that point with a client-side
transaction_depthof zero, so neither drop guard has queued aROLLBACK:BEGIN's round trip is in flight — Postgres: cancelling Pool::begin leaves the connection in a transaction and poisons it for the pool #4393, fixed at the source by fix(postgres): roll back a transaction cancelled during BEGIN #4394.raw_sql, e.g.BEGIN; SELECT 1/0;. The block is opened and aborted within a single simple-query message, so the depth is never raised at all and fix(postgres): roll back a transaction cancelled during BEGIN #4394 does not apply.The second shape does not self-heal. The next borrower's
BEGINfails because the block is already aborted, which leaves the depth at zero again, so nothing queues a rollback on that release either, and the ping waves it through once more. Every checkout of that connection fails with25P02untilmax_lifetimerecycles it.Both shapes also occur with
BEGIN READ ONLY, where the damage surfaces not as25P02but as a write failing withcannot execute … in a read-only transactionat some unrelated call site that merely drew the poisoned connection.The change
Check the server-reported status in
pingand roll back when it disagrees with the client's view.This costs no extra round trip.
wait_until_readyhas just refreshedtransaction_statusfrom the server's ownReadyForQuery, so the check is a field read, and theROLLBACKis only sent on a connection that is actually dirty — a clean release is byte-for-byte what it is today, with noWARNING: there is no transaction in progressin the server log.Fixing it in
pingrather than inreturn_to_poolkeeps it in the driver, where the status byte lives, and covers every caller ofpingrather than the pool alone.The
transaction_depth == 0gate is load-bearing.pingis public API and a caller may be holding a transaction deliberately across one; a non-zero depth means the client knows about the block and owns its lifetime, so it is left strictly alone. Only a block the client has no record of is ended here. That also means there is never a savepoint to restore to, hence a plainROLLBACKrather thanrollback_ansi_transaction_sql.Also:
PgConnection::transaction_statusExposed as public API, with
Clone/Copy/PartialEq/EqonTransactionStatus.Connection::is_in_transactionreportsget_transaction_depth(self) != 0— the client-side depth, which is precisely the value that is wrong in both shapes above. The server's view was not reachable from outside the crate at all:in_transaction()ispub(crate), the field is private, and whilemessage::ready_for_querydoespub use TransactionStatus,mod messageitself is private inlib.rs, so the type could not even be named. This lets pool users make the same distinction themselves in anafter_releasehook at no round-trip cost.Note
in_transaction()is deliberately left as-is: it treatsErroras not in a transaction, which is correct for its caller inbeginbut wrong for a release check. The new accessor returns the raw status so callers choose their own predicate.Tests
it_does_not_return_a_connection_inside_a_transaction_to_the_pool— on a one-connection pool, so the same session is guaranteed back. Covers shape 2 (deterministic, no timing), asserting the precondition that the client-side depth stays zero, then the leakedBEGIN READ ONLYcase, ending with the write that fails in production. It fails onmainwith:it_does_not_roll_back_a_transaction_the_caller_owns— pins the depth gate, so the guard cannot be "simplified" into rolling back a live transaction underneath its owner.Verified against a real server (Postgres 17, the
tests/postgres/setup.sqlfixtures). All 9 Postgres test targets pass, 270 tests, includingit_rolls_back_a_transaction_cancelled_during_beginfrom #4394 and theit_can_work_with_failed_transactions/it_can_work_with_nested_transactions/it_can_fail_and_recoverfamily.cargo clippy -p sqlx-postgresandcargo fmt --all --checkare clean, and the 150sqlx-postgresunit tests pass.Note on scope
This makes #4394 belt-and-braces rather than redundant: #4394 stops the cancelled-
BEGINcase from ever leaking, and this stops any connection from returning to the pool dirty, whatever the cause. I would suggest keeping both.Happy to split the
transaction_statusaccessor into its own PR if you would rather review the fix alone.