From 53a744ebde8fcfb0f24577766e7557069512f416 Mon Sep 17 00:00:00 2001 From: Victor Alejandro Sanz Ararat Date: Tue, 23 Jun 2026 10:27:49 +0200 Subject: [PATCH 1/6] fix(tx): keep the connection on a recoverable BEGIN error 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 --- pgxpool/pool_test.go | 13 ++++++------- tx.go | 16 +++++++++++++--- tx_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 10 deletions(-) diff --git a/pgxpool/pool_test.go b/pgxpool/pool_test.go index 52c294a51..802cace7d 100644 --- a/pgxpool/pool_test.go +++ b/pgxpool/pool_test.go @@ -1066,14 +1066,13 @@ func TestConnReleaseWhenBeginFail(t *testing.T) { assert.NoError(t, err) } - for range 1000 { - if db.Stat().TotalConns() == 0 { - break - } - time.Sleep(time.Millisecond) - } + // The invalid BEGIN fails with a non-fatal error, so the connection stays + // usable and is released back to the pool rather than destroyed. + assert.EqualValues(t, 1, db.Stat().TotalConns()) - assert.EqualValues(t, 0, db.Stat().TotalConns()) + var n int + require.NoError(t, db.QueryRow(ctx, "select 1").Scan(&n)) + require.EqualValues(t, 1, n) } func TestTxBeginFuncNestedTransactionCommit(t *testing.T) { diff --git a/tx.go b/tx.go index 3f93a6f24..5d2f2789c 100644 --- a/tx.go +++ b/tx.go @@ -100,9 +100,11 @@ func (c *Conn) Begin(ctx context.Context) (Tx, error) { func (c *Conn) BeginTx(ctx context.Context, txOptions TxOptions) (Tx, error) { _, err := c.Exec(ctx, txOptions.beginSQL()) if err != nil { - // begin should never fail unless there is an underlying connection issue or - // a context timeout. In either case, the connection is possibly broken. - c.die() + // begin kills the connection upon receiving fatal or panic errors, but does not otherwise as the connection should be reusable + var pgErr *pgconn.PgError + if !errors.As(err, &pgErr) || isConnectionFatal(pgErr) { + c.die() + } return nil, err } @@ -112,6 +114,14 @@ func (c *Conn) BeginTx(ctx context.Context, txOptions TxOptions) (Tx, error) { }, nil } +func isConnectionFatal(pgErr *pgconn.PgError) bool { + severity := pgErr.SeverityUnlocalized + if severity == "" { + severity = pgErr.Severity + } + return severity == "FATAL" || severity == "PANIC" +} + // Tx represents a database transaction. // // Tx is an interface instead of a struct to enable connection pools to be implemented without relying on internal pgx diff --git a/tx_test.go b/tx_test.go index a035aba07..4775b4c25 100644 --- a/tx_test.go +++ b/tx_test.go @@ -643,3 +643,47 @@ func TestTxSendBatchClosed(t *testing.T) { _, err = br.Query() require.Error(t, err) } + +func TestBeginTxNonFatalErrorKeepsConnAlive(t *testing.T) { + t.Parallel() + + conn := mustConnectString(t, os.Getenv("PGX_TEST_DATABASE")) + defer closeConn(t, conn) + + pgxtest.SkipCockroachDB(t, conn, "Server returns a different severity/error for an invalid BEGIN") + + ctx := context.Background() + + _, err := conn.BeginTx(ctx, pgx.TxOptions{BeginQuery: "begin transaction isolation level nonsense"}) + require.Error(t, err) + + var pgErr *pgconn.PgError + require.True(t, errors.As(err, &pgErr)) + require.NotEqual(t, "FATAL", pgErr.SeverityUnlocalized) + + require.False(t, conn.IsClosed()) + + var n int + require.NoError(t, conn.QueryRow(ctx, "select 1").Scan(&n)) + require.Equal(t, 1, n) +} + +func TestBeginTxFatalErrorKillsConn(t *testing.T) { + t.Parallel() + + conn := mustConnectString(t, os.Getenv("PGX_TEST_DATABASE")) + defer closeConn(t, conn) + + pgxtest.SkipCockroachDB(t, conn, "Server does not support pg_terminate_backend()") + + ctx := context.Background() + + _, err := conn.BeginTx(ctx, pgx.TxOptions{BeginQuery: "select pg_terminate_backend(pg_backend_pid())"}) + require.Error(t, err) + + var pgErr *pgconn.PgError + require.True(t, errors.As(err, &pgErr)) + require.Equal(t, "FATAL", pgErr.SeverityUnlocalized) + + require.True(t, conn.IsClosed()) +} From 0ef4e7f41c7585cd4fe1d3939a73fdf07e4f90d2 Mon Sep 17 00:00:00 2001 From: Victor Alejandro Sanz Ararat Date: Tue, 23 Jun 2026 11:43:30 +0200 Subject: [PATCH 2/6] Update tx.go --- tx.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tx.go b/tx.go index 5d2f2789c..dcb0feb43 100644 --- a/tx.go +++ b/tx.go @@ -100,7 +100,8 @@ func (c *Conn) Begin(ctx context.Context) (Tx, error) { func (c *Conn) BeginTx(ctx context.Context, txOptions TxOptions) (Tx, error) { _, err := c.Exec(ctx, txOptions.beginSQL()) if err != nil { - // begin kills the connection upon receiving fatal or panic errors, but does not otherwise as the connection should be reusable + // begin kills the connection upon receiving fatal, panic or non PGError errors, + // but does not otherwise as the connection should be reusable. var pgErr *pgconn.PgError if !errors.As(err, &pgErr) || isConnectionFatal(pgErr) { c.die() From ad6e41a1402550fd65ac2272935d74330df6c16e Mon Sep 17 00:00:00 2001 From: Victor Alejandro Sanz Ararat Date: Tue, 23 Jun 2026 11:46:48 +0200 Subject: [PATCH 3/6] test(pgxpool): assert conn is destroyed when BEGIN fails fatally 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 --- pgxpool/pool_test.go | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/pgxpool/pool_test.go b/pgxpool/pool_test.go index 802cace7d..c3ff96273 100644 --- a/pgxpool/pool_test.go +++ b/pgxpool/pool_test.go @@ -1075,6 +1075,40 @@ func TestConnReleaseWhenBeginFail(t *testing.T) { require.EqualValues(t, 1, n) } +func TestConnDestroyedWhenBeginFailsFatally(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + defer cancel() + + controllerConn, err := pgx.Connect(ctx, os.Getenv("PGX_TEST_DATABASE")) + require.NoError(t, err) + defer controllerConn.Close(ctx) + pgxtest.SkipCockroachDB(t, controllerConn, "Server does not support pg_terminate_backend() (https://github.com/cockroachdb/cockroach/issues/35897)") + + db, err := pgxpool.New(ctx, os.Getenv("PGX_TEST_DATABASE")) + require.NoError(t, err) + defer db.Close() + + // The BEGIN terminates its own backend, so it fails with a fatal error and + // the connection is destroyed rather than released back to the pool. + tx, err := db.BeginTx(ctx, pgx.TxOptions{BeginQuery: "select pg_terminate_backend(pg_backend_pid())"}) + assert.Error(t, err) + if !assert.Zero(t, tx) { + err := tx.Rollback(ctx) + assert.NoError(t, err) + } + + for range 1000 { + if db.Stat().TotalConns() == 0 { + break + } + time.Sleep(time.Millisecond) + } + + assert.EqualValues(t, 0, db.Stat().TotalConns()) +} + func TestTxBeginFuncNestedTransactionCommit(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) defer cancel() From 09081aa2221fb8d740711655a594de7bfbcce5f4 Mon Sep 17 00:00:00 2001 From: Victor Alejandro Sanz Ararat Date: Tue, 23 Jun 2026 11:55:40 +0200 Subject: [PATCH 4/6] test: drop explanatory comments from begin-failure tests Co-Authored-By: Claude Opus 4.8 --- pgxpool/pool_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pgxpool/pool_test.go b/pgxpool/pool_test.go index c3ff96273..12e4036c3 100644 --- a/pgxpool/pool_test.go +++ b/pgxpool/pool_test.go @@ -1066,8 +1066,6 @@ func TestConnReleaseWhenBeginFail(t *testing.T) { assert.NoError(t, err) } - // The invalid BEGIN fails with a non-fatal error, so the connection stays - // usable and is released back to the pool rather than destroyed. assert.EqualValues(t, 1, db.Stat().TotalConns()) var n int @@ -1090,8 +1088,6 @@ func TestConnDestroyedWhenBeginFailsFatally(t *testing.T) { require.NoError(t, err) defer db.Close() - // The BEGIN terminates its own backend, so it fails with a fatal error and - // the connection is destroyed rather than released back to the pool. tx, err := db.BeginTx(ctx, pgx.TxOptions{BeginQuery: "select pg_terminate_backend(pg_backend_pid())"}) assert.Error(t, err) if !assert.Zero(t, tx) { From 7a3afc85f8797fde615496186c8e024813a78039 Mon Sep 17 00:00:00 2001 From: Victor Alejandro Sanz Ararat Date: Thu, 25 Jun 2026 16:57:35 +0200 Subject: [PATCH 5/6] test(pgxpool): use require for begin-failure assertions 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 --- pgxpool/pool_test.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/pgxpool/pool_test.go b/pgxpool/pool_test.go index 12e4036c3..90223a67f 100644 --- a/pgxpool/pool_test.go +++ b/pgxpool/pool_test.go @@ -1066,7 +1066,7 @@ func TestConnReleaseWhenBeginFail(t *testing.T) { assert.NoError(t, err) } - assert.EqualValues(t, 1, db.Stat().TotalConns()) + require.EqualValues(t, 1, db.Stat().TotalConns()) var n int require.NoError(t, db.QueryRow(ctx, "select 1").Scan(&n)) @@ -1089,11 +1089,8 @@ func TestConnDestroyedWhenBeginFailsFatally(t *testing.T) { defer db.Close() tx, err := db.BeginTx(ctx, pgx.TxOptions{BeginQuery: "select pg_terminate_backend(pg_backend_pid())"}) - assert.Error(t, err) - if !assert.Zero(t, tx) { - err := tx.Rollback(ctx) - assert.NoError(t, err) - } + require.Error(t, err) + require.Zero(t, tx) for range 1000 { if db.Stat().TotalConns() == 0 { @@ -1102,7 +1099,7 @@ func TestConnDestroyedWhenBeginFailsFatally(t *testing.T) { time.Sleep(time.Millisecond) } - assert.EqualValues(t, 0, db.Stat().TotalConns()) + require.EqualValues(t, 0, db.Stat().TotalConns()) } func TestTxBeginFuncNestedTransactionCommit(t *testing.T) { From eb929e31576bcec81b3bd5243f3381305592f5af Mon Sep 17 00:00:00 2001 From: Victor Alejandro Sanz Ararat Date: Thu, 25 Jun 2026 17:03:50 +0200 Subject: [PATCH 6/6] test(pgxpool): simplify TestConnReleaseWhenBeginFail to require 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 --- pgxpool/pool_test.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pgxpool/pool_test.go b/pgxpool/pool_test.go index 90223a67f..2bae82914 100644 --- a/pgxpool/pool_test.go +++ b/pgxpool/pool_test.go @@ -1060,11 +1060,8 @@ func TestConnReleaseWhenBeginFail(t *testing.T) { tx, err := db.BeginTx(ctx, pgx.TxOptions{ IsoLevel: pgx.TxIsoLevel("foo"), }) - assert.Error(t, err) - if !assert.Zero(t, tx) { - err := tx.Rollback(ctx) - assert.NoError(t, err) - } + require.Error(t, err) + require.Zero(t, tx) require.EqualValues(t, 1, db.Stat().TotalConns())