Repository navigation
fix(postgres,pgx): roll back aborted explicit transaction on migration error - #1446
Open
cipherprofessor wants to merge 1 commit into
Open
cipherprofessor wants to merge 1 commit into
cipherprofessor wants to merge 1 commit into
Conversation
…n error A migration script using an explicit BEGIN/COMMIT block (which this project's own TUTORIAL.md recommends) is not automatically rolled back by Postgres when a statement inside it errors -- unlike an implicit transaction, which Postgres's simple query protocol rolls back automatically on error. Left un-rolled-back, the connection stays in an aborted-transaction state, and every subsequent statement on it fails, including the advisory-unlock call migrate issues after a failed run -- surfacing a confusing second error on top of the real one, exactly as described in golang-migrate#581. Fixed identically across all three Postgres drivers (postgres, pgx, pgx/v5): runStatement now issues a ROLLBACK after a failed ExecContext, before building the returned error. Verified empirically (not assumed) that ROLLBACK with no explicit transaction open is a safe no-op: Postgres reports only a WARNING, not an error, and the connection remains fully usable afterward -- checked directly via the same driver code path used here, not just psql. Review before committing caught two real issues with the first draft, both fixed: - The original draft discarded the statement's own rich pgErr diagnostic (message, line/column, detail) entirely if the recovery ROLLBACK itself failed, replacing it with a bare rollback-failure error. Now both are preserved: the original error message/line/ column is still built from the statement's own error, a rollback failure is appended as additional context rather than replacing it, and OrigErr joins both errors (errors.Join), matching this package's own existing convention for combining a statement error with a rollback error (see SetVersion). - The recovery ROLLBACK was issued on an unbounded context.Background(), ignoring the driver's own configured StatementTimeout. It now gets its own fresh timeout window sized to that same config value, rather than being unbounded or reusing the original ctx (which may already be expired if a timeout is what caused the statement to fail). Testing: this project's own dktest-based Docker test suite can't run in this environment -- the pinned dhui/dktest@v0.4.6 dependency hardcodes Docker API version 1.41 (client.WithVersion("1.41")) even with DOCKER_API_VERSION set, which this Docker daemon (minimum supported API 1.44) rejects. Confirmed this is a pre-existing, diff-independent environment limitation, not something introduced here: an unrelated, untouched existing test (testMultipleStatements) fails identically on unmodified code. Verified the fix instead by running a real Postgres 16 container directly and exercising the actual unmodified driver code (Open/Lock/Run/Unlock) against it via a standalone program, both before the fix (red: Unlock fails with "current transaction is aborted") and after (green: Unlock succeeds) -- for both the postgres and pgx/v5 drivers. Also confirmed the fix doesn't change the exact error message an existing test (testErrorParsing) asserts on for a plain (non-transactional) syntax error, by re-running that exact scenario against real Postgres before and after. Added the project's own standard dktest-based regression test (testExplicitTransactionRolledBackOnError / TestExplicitTransactionRolledBackOnError) to all three driver test files, matching each file's existing convention, so CI (which has a working Docker setup) exercises this directly going forward even though it couldn't be run locally here. go build, go vet, and gofmt are all clean. Fixes golang-migrate#581
This branch has not been deployed
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
1 participant
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.
Fixes #581.
Problem
A migration script using an explicit
BEGIN/COMMITblock (which this project's ownTUTORIAL.mdrecommends) is not automatically rolled back by Postgres when a statement inside it errors — unlike an implicit transaction, which Postgres's simple query protocol rolls back automatically on error. Left un-rolled-back, the connection stays in an aborted-transaction state, and every subsequent statement on it fails, including the advisory-unlock callmigrateissues after a failed run — surfacing a confusing second error on top of the real one, exactly as described in #581.Fix
Identical across all three Postgres drivers (
postgres,pgx,pgx/v5):runStatementnow issues aROLLBACKafter a failedExecContext, before building the returned error.ROLLBACKwith no explicit transaction open is a safe no-op — Postgres reports only aWARNING, not an error, and the connection remains fully usable afterward. Checked directly through the actual driver code path, not justpsql.pgErr— message, line/column, detail), that's preserved even if the recoveryROLLBACKalso fails: the rollback failure is appended as additional context and joined viaerrors.Join(matching this package's own existing convention inSetVersion), rather than discarding the original diagnostic.ROLLBACKgets its own fresh timeout window sized to the driver's configuredStatementTimeout, rather than being unbounded or reusing the original (possibly already-expired) context.Testing
This project's own
dktest-based Docker test suite couldn't run in my environment — the pinneddhui/dktest@v0.4.6dependency hardcodes Docker API version 1.41 (client.WithVersion("1.41")) even withDOCKER_API_VERSIONset, which my Docker daemon (minimum supported API 1.44) rejects. I confirmed this is a pre-existing, diff-independent environment limitation, not something this change introduces: an unrelated, untouched existing test (testMultipleStatements) fails identically against unmodifiedmaster.To still verify with real rigor, I ran a real Postgres 16 container directly and exercised the actual driver code (
Open/Lock/Run/Unlock) against it via a standalone program — both before the fix (red:Unlockfails withcurrent transaction is aborted) and after (green:Unlocksucceeds) — for both thepostgresandpgx/v5drivers. I also confirmed the fix doesn't change the exact error messagetestErrorParsingasserts on for a plain (non-transactional) syntax error, by re-running that exact scenario against real Postgres before and after.I've added this project's own standard
dktest-based regression test (testExplicitTransactionRolledBackOnError/TestExplicitTransactionRolledBackOnError) to all three driver test files, matching each file's existing convention, so CI (which has a working Docker setup) exercises this directly even though I couldn't run it locally.go build,go vet, andgofmtare all clean.Related
There's an existing, never-upstreamed fix for this same issue sitting in a fork (referenced in a comment on #581) — I read it for context but implemented this independently with my own tests, not copied from it.