Config.TransactionsEnabled to control whether postgres/pgx drivers wraps each migration in a transaction

[sfllaw]

Is your feature request related to a problem? Please describe.

#196 is a suggested enhancement that wraps each transaction in a migration. This is practical for PostgreSQL, since it supports DDL transactions: Transactional DDL in PostgreSQL: A Competitive Analysis.

The practical upshot of automatically wrapping each migration in a transaction is that rolling back will almost always leave the database in a clean state. The only exceptions are adding and dropping databases, tablespaces, and clusters; which is unlikely to happen in a series of migrations.

Since a failed migration will not be commited, the migration runner does not need to mark the migration as dirty. This makes operations far easier, since the failing migration can be fixed in code and tried again. There is no need for human intervention on the database itself.

Describe the solution you'd like

1. In database/postgres/postgres.go and database/pgx/pgx.go, add a Config.TransactionsEnabled field:

   type Config struct {
   	// ...
   	TransactionsEnabled bool
   }

2. In addition, add an x-transactions-enabled parameter to the URL parsed by Postgres.Open.

3. Modify Postgres.SetVersion so that it leaves its transaction open when dirty is set and commits it when the version is clean:

   type Postgres struct {
   	// ...
   
   	mu sync.Mutex
   	// Tx is the shared transaction when config.TransactionsEnabled.
   	tx *sql.Tx
   }
   
   func (p *Postgres) SetVersion(version int, dirty bool) error {
   	p.mu.Lock()
   	defer p.mu.Unlock()
   	tx := p.tx
   	if tx == nil {
   		var err error
   		if tx, err = p.conn.BeginTx(context.Background(), &sql.TxOptions{}); err != nil {
   			return &database.Error{OrigErr: err, Err: "transaction start failed"}
   		}
   	}
   	if p.config.TransactionsEnabled && dirty {
   		// Leave the transaction open until migrations have completed.
   		p.tx = tx
   	}
   
   	// ...
   
   	if p.tx == nil {
   		// Migrations are not wrapped in transactions, so the version is always committed.
   		if err := tx.Commit(); err != nil {
   			return &database.Error{OrigErr: err, Err: "transaction commit failed"}
   		}
   	} else if !dirty {
   		// Migrations ran cleanly, so we can safely commit them and the new version.
   		p.tx = nil
   		if err := tx.Commit(); err != nil {
   			return &database.Error{OrigErr: err, Err: "transaction commit failed"}
   		}
   	}
   	return nil
   }

4. Modify Postgres.Run so that it wraps each migration in a transaction:

   type ExecerContext interface {
   	func ExecContext(ctx context.Context, query string, args ...interface{}) (sql.Result, error)
   }
   
   func (p *Postgres) Run(migration io.Reader) error {
   	p.mu.Lock()
   	defer p.mu.Unlock()
   	var conn ExecerContext = p.conn
   	if p.tx != nil {
   		// p.tx begins in p.SetVersion(n, true) and is committed in p.SetVersion(n, false)
   		conn = p.tx
   	}
   	if p.config.MultiStatementEnabled {
   		var err error
   		if e := multistmt.Parse(migration, multiStmtDelimiter, p.config.MultiStatementMaxSize, func(m []byte) bool {
   			if err = p.runStatement(conn, m); err != nil {
   				return false
   			}
   			return true
   		}); e != nil {
   			return e
   		}
   		if err != nil {
   			if errRollback := p.txRollback(); errRollback != nil {
   				err = multierror.Append(err, errRollback)
   			}
   			return err
   		}
   	} else {
   		migr, err := ioutil.ReadAll(migration)
   		if err != nil {
   			return err
   		}
   		if err := p.runStatement(conn, migr); err != nil {
   			if errRollback := p.txRollback(); errRollback != nil {
   				err = multierror.Append(err, errRollback)
   			}
   			return err
   		}
   	}
   	return nil
   }
   
   func (p *Postgres) runStatement(conn ExecerContext, statement []byte) error {
   	// ...
   	if _, err := conn.ExecContext(ctx, query); err != nil {
   		// ...
   	}
   	return nil
   }
   
   func (p *Postgres) txRollback() error {
   	p.mu.Lock()
   	defer p.mu.Unlock()
   	if p.tx == nil {
   		return nil
   	}
   	tx := p.tx
   	p.tx = nil
   	return tx.Rollback()
   }

Describe alternatives you've considered

Wrap each migration with transactions by default:

Instead, wrap the transactions by default with a Config.TransactionsDisabled.

One concern is that the postgres and pgx drivers would behave differently to all the other drivers, since most other databases do not support DDL transactions. This would violate the principle of least surprise.

Although this would be a backwards-compatible change, Postgres will issue a scary warning that would probably trigger support tickets:

postgres=# BEGIN;
BEGIN
postgres=*# BEGIN;
WARNING:  there is already a transaction in progress
BEGIN

Annotate each migration with its transaction safety:

Support a hybrid approach transactions by somehow determining whether the migration should be wrapped with a transaction or not.

This could be done by using a particular filename, leaving a magic comment in the migration file, or inspecting the file to see if rolling it back is possible.

The added complexity of this solution does not seem to be beneficial. Since the contents of each migration is an opaque blob, developers can fix up their legacy migrations so that they are compatible with Config.TransactionsEnabled or live with scary warnings.

Additional context

When Config.TransactionsEnabled is set, transactions will be controlled by Postgres.SetVersion, which will not commit until Migrate.runMigrations has declared that version to be clean. As such, the bug reported in #624 will not occur, assuming that Postgres.Lock is called early enough.

It is unfortunate that Postgres.Run has to rely on a side-effect in Postgres.SetVersion to begin the transaction, but changing the API to make this better would require a major version bump. Alternatively, we can move transaction handling to Postgres.Lock and Unlock, which might be more intuitive to use but might have unintended side-effects since it is more general.