Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[postgres] Issue rollback after migration error #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+171 −9
Open

[postgres] Issue rollback after migration error #1

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e2c6b1
Issue an explicit rollback if a migration statement returns any error
bclarkx2 Sep 20, 2023
bfdd2f0
Change wording of migration tx rollback error message to reduce noise
bclarkx2 Sep 29, 2023
4813cdb
Define migrationErr with non-nil value from the get-go
bclarkx2 Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions database/pgx/pgx.go
View file
Original file line number Diff line number Diff line change
@@ -285,7 +285,7 @@ func (p *Postgres) runStatement(statement []byte) error {
return nil
}
if _, err := p.conn.ExecContext(ctx, query); err != nil {

migrationErr := database.Error{OrigErr: err, Err: "migration failed", Query: statement}
if pgErr, ok := err.(*pgconn.PgError); ok {
var line uint
var col uint
@@ -298,9 +298,19 @@ func (p *Postgres) runStatement(statement []byte) error {
if pgErr.Detail != "" {
message = fmt.Sprintf("%s, %s", message, pgErr.Detail)
}
return database.Error{OrigErr: err, Err: message, Query: statement, Line: line}
migrationErr = database.Error{OrigErr: err, Err: message, Query: statement, Line: line}
}

// For safety, always issue a rollback on error. In multi-statement
// mode, this is necessary to make sure that the connection is not left
// in an aborted state. In single-statement mode, this will be a no-op
// outside of the implicit transaction block that was already rolled
// back.
if _, rollbackErr := p.conn.ExecContext(ctx, "ROLLBACK"); rollbackErr != nil {
rollbackErr = fmt.Errorf("rolling back migration tx: %w", rollbackErr)
return multierror.Append(migrationErr, rollbackErr)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a simple example of what this output might look like from a basic console logger in the case where both the migration statement and this explicit rollback generated an error:

2023/09/20 15:20:30 Database connection established
2023/09/20 15:20:30 Error running migrations: 2 errors occurred:
        * migration failed: division by zero in line 0:
SELECT 1/0; -- Divide by zero error!

 (details: pq: division by zero)
        * failed to rollback migration tx: pq: column "doesnt-exist" does not exist

2023/09/20 15:20:30 Closing database connection

I aimed to make this message clear that what failed wasn't a "rollback of the migrate schema version itself" , but just a rollback of the transaction being used in the up migration that was being attempted. Happy to take feedback on this error to make that clearer, though!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only nit on the wrapped error message is that failed to is kinda noise, as suggested by https://github.com/uber-go/guide/blob/master/style.md#error-wrapping, but it looks like this repo uses Failed to... already https://github.com/search?q=repo%3Anicheinc%2Fmigrate+Errorf+%25w&type=code

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me! In bfdd2f0 I changed the wording from "failed to rollback migration tx: " to "rolling back migration tx: ".

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do enough CFA-by-eye to know that migrationErr can't be nil here, but I wonder if we could protect against that my moving migrationErr = database.Error{OrigErr: err, Err: "migration failed", Query: statement} to the initialization of migrationErr - or if we'd even want to?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing of note is that hashicorp (in their infinite wisdom) has made Append work such that if migrationErr were nil here it would still at least generate a multierror with a single error inside of it: https://github.com/hashicorp/go-multierror/blob/v1.1.1/append.go#L36-L39

à la https://go.dev/play/p/HOmHw7-3Dbf

That being said, defining migrationErr from the get-go as the version derived from a non-pgconn error to start with and overwriting with a more specific database.Error when possible is fine by me as well!

Switched over accordingly in 4813cdb

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing of note is that hashicorp (in their infinite wisdom) has made Append work such that if migrationErr were nil here it would still at least generate a multierror with a single error inside of it: https://github.com/hashicorp/go-multierror/blob/v1.1.1/append.go#L36-L39

à la https://go.dev/play/p/HOmHw7-3Dbf

That being said, defining migrationErr from the get-go as the version derived from a non-pgconn error to start with and overwriting with a more specific database.Error when possible is fine by me as well!

Switched over accordingly in 4813cdb

}
return database.Error{OrigErr: err, Err: "migration failed", Query: statement}
return migrationErr
}
return nil
}
42 changes: 42 additions & 0 deletions database/pgx/pgx_test.go
View file
Original file line number Diff line number Diff line change
@@ -167,6 +167,48 @@ func TestMultipleStatements(t *testing.T) {
})
}

func TestMultipleStatementsError(t *testing.T) {
dktesting.ParallelTest(t, specs, func(t *testing.T, c dktest.ContainerInfo) {
ip, port, err := c.FirstPort()
if err != nil {
t.Fatal(err)
}

addr := pgConnectionString(ip, port)
p := &Postgres{}
d, err := p.Open(addr)
if err != nil {
t.Fatal(err)
}
defer func() {
if err := d.Close(); err != nil {
t.Error(err)
}
}()

// Run a migration with explicit transaction that we expect to fail
err = d.Run(strings.NewReader("BEGIN; SELECT 1/0; COMMIT;"))

// Migration should return expected error
var e database.Error
if !errors.As(err, &e) || err == nil {
t.Fatalf("Unexpected error, want migration error. Got: %#v", err)
}
if !strings.Contains(e.OrigErr.Error(), "division by zero") {
t.Fatalf("Migration error missing expected message. Got: %s", err)
}

// Connection should still be usable after failed migration
var result int
if err := d.(*Postgres).conn.QueryRowContext(context.Background(), "SELECT 123").Scan(&result); err != nil {
t.Fatalf("Unexpected error, want connection to be usable. Got: %s", err)
}
if result != 123 {
t.Fatalf("Unexpected result, want 123. Got: %d", result)
}
})
}

func TestMultipleStatementsInMultiStatementMode(t *testing.T) {
dktesting.ParallelTest(t, specs, func(t *testing.T, c dktest.ContainerInfo) {
ip, port, err := c.FirstPort()
16 changes: 13 additions & 3 deletions database/pgx/v5/pgx.go
View file
Original file line number Diff line number Diff line change
@@ -283,7 +283,7 @@ func (p *Postgres) runStatement(statement []byte) error {
return nil
}
if _, err := p.conn.ExecContext(ctx, query); err != nil {

migrationErr := database.Error{OrigErr: err, Err: "migration failed", Query: statement}
if pgErr, ok := err.(*pgconn.PgError); ok {
var line uint
var col uint
@@ -296,9 +296,19 @@ func (p *Postgres) runStatement(statement []byte) error {
if pgErr.Detail != "" {
message = fmt.Sprintf("%s, %s", message, pgErr.Detail)
}
return database.Error{OrigErr: err, Err: message, Query: statement, Line: line}
migrationErr = database.Error{OrigErr: err, Err: message, Query: statement, Line: line}
}

// For safety, always issue a rollback on error. In multi-statement
// mode, this is necessary to make sure that the connection is not left
// in an aborted state. In single-statement mode, this will be a no-op
// outside of the implicit transaction block that was already rolled
// back.
if _, rollbackErr := p.conn.ExecContext(ctx, "ROLLBACK"); rollbackErr != nil {
rollbackErr = fmt.Errorf("rolling back migration tx: %w", rollbackErr)
return multierror.Append(migrationErr, rollbackErr)
}
return database.Error{OrigErr: err, Err: "migration failed", Query: statement}
return migrationErr
}
return nil
}
42 changes: 42 additions & 0 deletions database/pgx/v5/pgx_test.go
View file
Original file line number Diff line number Diff line change
@@ -168,6 +168,48 @@ func TestMultipleStatements(t *testing.T) {
})
}

func TestMultipleStatementsError(t *testing.T) {
dktesting.ParallelTest(t, specs, func(t *testing.T, c dktest.ContainerInfo) {
ip, port, err := c.FirstPort()
if err != nil {
t.Fatal(err)
}

addr := pgConnectionString(ip, port)
p := &Postgres{}
d, err := p.Open(addr)
if err != nil {
t.Fatal(err)
}
defer func() {
if err := d.Close(); err != nil {
t.Error(err)
}
}()

// Run a migration with explicit transaction that we expect to fail
err = d.Run(strings.NewReader("BEGIN; SELECT 1/0; COMMIT;"))

// Migration should return expected error
var e database.Error
if !errors.As(err, &e) || err == nil {
t.Fatalf("Unexpected error, want migration error. Got: %s", err)
}
if !strings.Contains(e.OrigErr.Error(), "division by zero") {
t.Fatalf("Migration error missing expected message. Got: %s", err)
}

// Connection should still be usable after failed migration
var result int
if err := d.(*Postgres).conn.QueryRowContext(context.Background(), "SELECT 123").Scan(&result); err != nil {
t.Fatalf("Unexpected error, want connection to be usable. Got: %s", err)
}
if result != 123 {
t.Fatalf("Unexpected result, want 123. Got: %d", result)
}
})
}

func TestMultipleStatementsInMultiStatementMode(t *testing.T) {
dktesting.ParallelTest(t, specs, func(t *testing.T, c dktest.ContainerInfo) {
ip, port, err := c.FirstPort()
7 changes: 6 additions & 1 deletion database/postgres/TUTORIAL.md
View file
Original file line number Diff line number Diff line change
@@ -68,6 +68,12 @@ Make sure to check if your database changed as expected in this case as well.

## Database transactions

By default, all the statements in the migration file will be run inside one
implicit transaction. However, you can also use `BEGIN` and `COMMIT` statements
to explicitly control what transactions your migration uses (for example, if you
want to break your migration into multiple transactions to avoid holding onto a
lock for too long).

To show database transactions usage, let's create another set of migrations by running:
```
migrate create -ext sql -dir db/migrations -seq add_mood_to_users
@@ -76,7 +82,6 @@ Again, it should create for us two migrations files:
- 000002_add_mood_to_users.down.sql
- 000002_add_mood_to_users.up.sql

In Postgres, when we want our queries to be done in a transaction, we need to wrap it with `BEGIN` and `COMMIT` commands.
In our example, we are going to add a column to our database that can only accept enumerable values or NULL.
Migration up:
```
15 changes: 13 additions & 2 deletions database/postgres/postgres.go
View file
Original file line number Diff line number Diff line change
@@ -296,6 +296,7 @@ func (p *Postgres) runStatement(statement []byte) error {
return nil
}
if _, err := p.conn.ExecContext(ctx, query); err != nil {
migrationErr := database.Error{OrigErr: err, Err: "migration failed", Query: statement}
if pgErr, ok := err.(*pq.Error); ok {
var line uint
var col uint
@@ -312,9 +313,19 @@ func (p *Postgres) runStatement(statement []byte) error {
if pgErr.Detail != "" {
message = fmt.Sprintf("%s, %s", message, pgErr.Detail)
}
return database.Error{OrigErr: err, Err: message, Query: statement, Line: line}
migrationErr = database.Error{OrigErr: err, Err: message, Query: statement, Line: line}
}
return database.Error{OrigErr: err, Err: "migration failed", Query: statement}

// For safety, always issue a rollback on error. In multi-statement
// mode, this is necessary to make sure that the connection is not left
// in an aborted state. In single-statement mode, this will be a no-op
// outside of the implicit transaction block that was already rolled
// back.
if _, rollbackErr := p.conn.ExecContext(ctx, "ROLLBACK"); rollbackErr != nil {
rollbackErr = fmt.Errorf("rolling back migration tx: %w", rollbackErr)
return multierror.Append(migrationErr, rollbackErr)
}
return migrationErr
}
return nil
}
42 changes: 42 additions & 0 deletions database/postgres/postgres_test.go
View file
Original file line number Diff line number Diff line change
@@ -165,6 +165,48 @@ func TestMultipleStatements(t *testing.T) {
})
}

func TestMultipleStatementsError(t *testing.T) {
dktesting.ParallelTest(t, specs, func(t *testing.T, c dktest.ContainerInfo) {
ip, port, err := c.FirstPort()
if err != nil {
t.Fatal(err)
}

addr := pgConnectionString(ip, port)
p := &Postgres{}
d, err := p.Open(addr)
if err != nil {
t.Fatal(err)
}
defer func() {
if err := d.Close(); err != nil {
t.Error(err)
}
}()

// Run a migration with explicit transaction that we expect to fail
err = d.Run(strings.NewReader("BEGIN; SELECT 1/0; COMMIT;"))

// Migration should return expected error
var e database.Error
if !errors.As(err, &e) || err == nil {
t.Fatalf("Unexpected error, want migration error. Got: %s", err)
}
if !strings.Contains(e.OrigErr.Error(), "division by zero") {
t.Fatalf("Migration error missing expected message. Got: %s", err)
}

// Connection should still be usable after failed migration
var result int
if err := d.(*Postgres).conn.QueryRowContext(context.Background(), "SELECT 123").Scan(&result); err != nil {
t.Fatalf("Unexpected error, want connection to be usable. Got: %s", err)
}
if result != 123 {
t.Fatalf("Unexpected result, want 123. Got: %d", result)
}
})
}

func TestMultipleStatementsInMultiStatementMode(t *testing.T) {
dktesting.ParallelTest(t, specs, func(t *testing.T, c dktest.ContainerInfo) {
ip, port, err := c.FirstPort()