Keep connections alive after BEGIN statement for non-FATAL errors#2589
Open
VashDD wants to merge 6 commits into
Open
Keep connections alive after BEGIN statement for non-FATAL errors#2589VashDD wants to merge 6 commits into
VashDD wants to merge 6 commits into
Conversation
Conn.BeginTx called c.die() on any BEGIN error, unconditionally destroying
the connection. That is wrong when the server returns a normal (non-FATAL)
ErrorResponse followed by ReadyForQuery, leaving the connection reusable:
- a pooler/proxy shedding load on BEGIN with SQLSTATE 53000, which keeps the
connection alive; destroying it amplifies load under retry-heavy clients.
- a malformed custom BeginQuery / invalid isolation level, which fails with a
plain syntax error and leaves the session intact.
Only die() when the connection is actually compromised: a non-PgError
(transport error / context cancellation) or a FATAL/PANIC server error.
Otherwise return the error and keep the connection.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
VashDD
changed the title
VashDD
changed the title
Complements TestConnReleaseWhenBeginFail (recoverable error -> conn kept) with the fatal case: a BEGIN that terminates its own backend fails fatally, so the pool destroys the connection (TotalConns -> 0) instead of reusing it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sean-
reviewed
Jun 24, 2026
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| assert.EqualValues(t, 1, db.Stat().TotalConns()) |
| defer db.Close() | ||
|
|
||
| tx, err := db.BeginTx(ctx, pgx.TxOptions{BeginQuery: "select pg_terminate_backend(pg_backend_pid())"}) | ||
| assert.Error(t, err) |
| assert.Error(t, err) | ||
| if !assert.Zero(t, tx) { | ||
| err := tx.Rollback(ctx) | ||
| assert.NoError(t, err) |
| if severity == "" { | ||
| severity = pgErr.Severity | ||
| } | ||
| return severity == "FATAL" || severity == "PANIC" |
Contributor
There was a problem hiding this comment.
Probably unrelated to this PR, but I wish these magic strings were constants that could be traced through the source.
Author
There was a problem hiding this comment.
Agree, I don't mind doing a follow up for that, but should take part in a different PR.
Address review: switch the begin-failure preconditions to require, and simplify the fatal pool test to require.Error + require.Zero (FailNow still runs deferred db.Close, so no cleanup is leaked). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
|
Thanks for the review, addressed comments, ready for another pass. |
sean-
approved these changes
Jun 25, 2026
Mirror TestConnDestroyedWhenBeginFailsFatally: require.Error + require.Zero instead of the assert-guarded defensive rollback (FailNow still runs the deferred db.Close, so nothing leaks). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
2 participants
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.
Problem
For more context, see: #2583
Conn.BeginTxcallsc.die()on any error fromBEGIN, unconditionally destroying the connection:The assumption ("begin should never fail unless the connection is broken") is true for connecting directly to postgres, but with a connection pooler in between, there are errors that should not kill the connection, as it is reusable, for instance errors due to load shedding.
Change
Only
die()when the connection is actually compromised due to:PgError(transport error / context cancellation)FATAL/PANICPgError(server tearing the session down)Tests
TestBeginTxNonFatalErrorKeepsConnAlive— a BEGIN failing with a non-FATAL error leavesIsClosed() == falseand the connection still answersselect 1.TestBeginTxFatalErrorKillsConn— a BEGIN that terminates its own backend (pg_terminate_backend(pg_backend_pid())) returns aFATALerror and the connection is closed.pgxpool.TestConnReleaseWhenBeginFail— updated: a recoverable BEGIN failure now releases the connection back to the pool alive (TotalConns stays 1, reusable) instead of destroying it.pgxpool.TestConnDestroyedWhenBeginFailsFatally— a BEGIN that terminates its own backend fails fatally, so the pool destroys the connection (TotalConns drops to 0) instead of reusing it.