Make the migrators only use transactions by default if the database s…

…upports transactional DDL

Using transactions by default doesn't harm things in most cases,
but it can in some cases.  For example, on SQLite, when emulating
alter_table on a table referenced by foreign keys, you need to
disabled foreign key use outside of a transaction.  If the
migrator runs the migration inside a transaction, it messes up
the foreign key reference to point to the backup table.

Seems like PostgreSQL, MSSQL, and Derby support transactional
schema, so this doesn't change things for those databases.

This adds the transaction method to the migration DSL, so you
can force transactions on a per migration basis for databases
that don't support transactional schema.

CHANGELOG

@@ -1,5 +1,9 @@
 === HEAD
 
+* Make the migrators only use transactions by default if the database supports transactional DDL (jeremyevans)
+
+* Add Database#supports_transactional_ddl? for checking if DDL statements can be rolled back in transactions (jeremyevans)
+
 * Don't use auto parameterization when using cursors in the pg_auto_parameterize extension (jeremyevans) (#463)
 
 * No longer escape backslashes in strings by default, fixes doubled backslashes on some adapters (jeremyevans)

doc/migration.rdoc

@@ -173,19 +173,29 @@ which just executes the code on the underlying database.
 
 == Errors when running migrations
 
-Sequel attempts to run migrations inside of a transaction.  Some databases do not support
-schema modifications made in transactions, and if the migration raises an error, it will
-not rollback the previous schema changes made by the migration.  In that case, you will
+Sequel attempts to run migrations inside of a transaction if the database supports
+transactional DDL statements.  On the databases that don't support transactional DDL
+statements, if there is an error while running a migration, it will not rollback the
+previous schema changes made by the migration.  In that case, you will
 need to update the database by hand.
 
 It's recommended to always run migrations on a test database and ensure they work
 before running them on any production database.
 
 == Transactions
 
-As mentioned above, Sequel attempts to run migrations inside of a transaction by default.
-However, you can disable this on a per migration basis by calling +no_transaction+ inside
-of the Sequel.migration block:
+You can manually specify to use transactions on a per migration basis.  For example,
+if you want to force transaction use for a particular migration, call the transaction
+method in the Sequel.migration block:
+
+  Sequel.migration do
+    transaction
+    change do
+      # ...
+    end
+  end
+
+Likewise, you can disable transaction use via no_transaction:
 
   Sequel.migration do
     no_transaction
@@ -195,7 +205,8 @@ of the Sequel.migration block:
   end
 
 This is necessary in some cases, such as when attempting to use CREATE INDEX CONCURRENTLY
-on PostgreSQL.
+on PostgreSQL (which supports transactional schema, but not that statement inside a
+transaction).
 
 You can also override the transactions setting at the migrator level, either by forcing
 transactions even if no_transaction is set, or by disabling transactions all together:

lib/sequel/adapters/jdbc/derby.rb

@@ -37,6 +37,11 @@ def svn_version
           end
         end
 
+        # Derby supports transaction DDL statements.
+        def supports_transactional_ddl?
+          true
+        end
+
         private
 
         # Derby optimizes away Sequel's default check of SELECT NULL FROM table,

lib/sequel/adapters/shared/mssql.rb

@@ -72,6 +72,11 @@ def supports_transaction_isolation_levels?
         true
       end
 
+      # MSSQL supports transaction DDL statements.
+      def supports_transactional_ddl?
+        true
+      end
+
       # Microsoft SQL Server supports using the INFORMATION_SCHEMA to get
       # information on tables.
       def tables(opts={})

lib/sequel/adapters/shared/postgres.rb

@@ -383,13 +383,6 @@ def server_version(server=nil)
         @server_version
       end
 
-      # PostgreSQL supports prepared transactions (two-phase commit) if
-      # max_prepared_transactions is greater than 0.
-      def supports_prepared_transactions?
-        return @supports_prepared_transactions if defined?(@supports_prepared_transactions)
-        @supports_prepared_transactions = self['SHOW max_prepared_transactions'].get.to_i > 0
-      end
-
       # PostgreSQL supports CREATE TABLE IF NOT EXISTS on 9.1+
       def supports_create_table_if_not_exists?
         server_version >= 90100
@@ -400,6 +393,13 @@ def supports_drop_table_if_exists?
         true
       end
 
+      # PostgreSQL supports prepared transactions (two-phase commit) if
+      # max_prepared_transactions is greater than 0.
+      def supports_prepared_transactions?
+        return @supports_prepared_transactions if defined?(@supports_prepared_transactions)
+        @supports_prepared_transactions = self['SHOW max_prepared_transactions'].get.to_i > 0
+      end
+
       # PostgreSQL supports savepoints
       def supports_savepoints?
         true
@@ -410,6 +410,11 @@ def supports_transaction_isolation_levels?
         true
       end
 
+      # PostgreSQL supports transaction DDL statements.
+      def supports_transactional_ddl?
+        true
+      end
+
       # Array of symbols specifying table names in the current database.
       # The dataset used is yielded to the block if one is provided,
       # otherwise, an array of symbols of table names is returned.

lib/sequel/database/misc.rb

@@ -180,6 +180,11 @@ def supports_transaction_isolation_levels?
       false
     end
 
+    # Whether DDL statements work correctly in transactions, false by default.
+    def supports_transactional_ddl?
+      false
+    end
+
     # The timezone to use for this database, defaulting to <tt>Sequel.database_timezone</tt>.
     def timezone
       @timezone || Sequel.database_timezone

lib/sequel/extensions/migration.rb

@@ -44,9 +44,9 @@ def self.inherited(base)
       descendants << base
     end
 
-    # Always use transactions for old-style migrations.
+    # Don't allow transaction overriding in old migrations.
     def self.use_transactions
-      true
+      nil
     end
 
     # The default down action does nothing
@@ -74,12 +74,13 @@ class SimpleMigration
     # Proc used for the up action
     attr_accessor :up
 
-    # Whether to use transactions for this migration, true by default.
+    # Whether to use transactions for this migration, default depends on the
+    # database.
     attr_accessor :use_transactions
 
-    # Use transactions by default
+    # Don't set transaction use by default.
     def initialize
-      @use_transactions = true
+      @use_transactions = nil
     end
 
     # Apply the appropriate block on the +Database+
@@ -118,6 +119,11 @@ def no_transaction
       migration.use_transactions = false
     end
 
+    # Enable the use of transactions for the related migration
+    def transaction
+      migration.use_transactions = true
+    end
+
     # Defines the migration's up action.
     def up(&block)
       migration.up = block
@@ -414,9 +420,17 @@ def initialize(db, directory, opts={})
     # If transactions should be used for the migration, yield to the block
     # inside a transaction.  Otherwise, just yield to the block.
     def checked_transaction(migration, &block)
-      if @use_transactions == false
-        yield
-      elsif migration.use_transactions || @use_transactions
+      use_trans = if @use_transactions.nil?
+        if migration.use_transactions.nil?
+          @db.supports_transactional_ddl?
+        else
+          migration.use_transactions
+        end
+      else
+        @use_transactions
+      end
+
+      if use_trans
         db.transaction(&block)
       else
         yield

spec/core/database_spec.rb

@@ -1951,6 +1951,12 @@ def @db.dc; @dc end
   end
 end
 
+describe "Database#supports_transactional_ddl?" do
+  specify "should be false by default" do
+    Sequel::Database.new.supports_transactional_ddl?.should == false
+  end
+end
+
 describe "Database#supports_savepoints?" do
   specify "should be false by default" do
     Sequel::Database.new.supports_savepoints?.should == false

spec/extensions/migration_spec.rb

@@ -320,23 +320,30 @@ def table_exists?(name)
     proc{Sequel::IntegerMigrator.apply(@db, "spec/files/timestamped_migrations")}.should raise_error(Sequel::Migrator::Error)
   end
 
-  specify "should use transactions by default" do
-    Sequel::Migrator.apply(@db, "spec/files/transaction_migrations")
+  specify "should not use transactions by default" do
+    Sequel::Migrator.apply(@db, "spec/files/transaction_unspecified_migrations")
+    @db.sqls.should == ["CREATE TABLE schema_info (version integer DEFAULT 0 NOT NULL)", "SELECT 1 AS one FROM schema_info LIMIT 1", "INSERT INTO schema_info (version) VALUES (0)", "SELECT version FROM schema_info LIMIT 1", "CREATE TABLE sm11111 (smc1 integer)", "UPDATE schema_info SET version = 1", "CREATE TABLE sm (smc1 integer)", "UPDATE schema_info SET version = 2"]
+  end
+
+  specify "should use transactions by default if the database supports transactional ddl" do
+    @db.meta_def(:supports_transactional_ddl?){true}
+    Sequel::Migrator.apply(@db, "spec/files/transaction_unspecified_migrations")
     @db.sqls.should == ["CREATE TABLE schema_info (version integer DEFAULT 0 NOT NULL)", "SELECT 1 AS one FROM schema_info LIMIT 1", "INSERT INTO schema_info (version) VALUES (0)", "SELECT version FROM schema_info LIMIT 1", "BEGIN", "CREATE TABLE sm11111 (smc1 integer)", "UPDATE schema_info SET version = 1", "COMMIT", "BEGIN", "CREATE TABLE sm (smc1 integer)", "UPDATE schema_info SET version = 2", "COMMIT"]
   end
 
-  specify "should not use transactions for migrations that disable it" do
-    Sequel::Migrator.apply(@db, "spec/files/transactionless_migrations")
-    @db.sqls.should == ["CREATE TABLE schema_info (version integer DEFAULT 0 NOT NULL)", "SELECT 1 AS one FROM schema_info LIMIT 1", "INSERT INTO schema_info (version) VALUES (0)", "SELECT version FROM schema_info LIMIT 1", "CREATE TABLE sm11111 (smc1 integer)", "UPDATE schema_info SET version = 1", "CREATE TABLE sm (smc1 integer)", "UPDATE schema_info SET version = 2"]
+  specify "should respect transaction use on a per migration basis" do
+    @db.meta_def(:supports_transactional_ddl?){true}
+    Sequel::Migrator.apply(@db, "spec/files/transaction_specified_migrations")
+    @db.sqls.should == ["CREATE TABLE schema_info (version integer DEFAULT 0 NOT NULL)", "SELECT 1 AS one FROM schema_info LIMIT 1", "INSERT INTO schema_info (version) VALUES (0)", "SELECT version FROM schema_info LIMIT 1", "BEGIN", "CREATE TABLE sm11111 (smc1 integer)", "UPDATE schema_info SET version = 1", "COMMIT", "CREATE TABLE sm (smc1 integer)", "UPDATE schema_info SET version = 2"]
   end
 
   specify "should force transactions if enabled in the migrator" do
-    Sequel::Migrator.run(@db, "spec/files/transactionless_migrations", :use_transactions=>true)
+    Sequel::Migrator.run(@db, "spec/files/transaction_specified_migrations", :use_transactions=>true)
     @db.sqls.should == ["CREATE TABLE schema_info (version integer DEFAULT 0 NOT NULL)", "SELECT 1 AS one FROM schema_info LIMIT 1", "INSERT INTO schema_info (version) VALUES (0)", "SELECT version FROM schema_info LIMIT 1", "BEGIN", "CREATE TABLE sm11111 (smc1 integer)", "UPDATE schema_info SET version = 1", "COMMIT", "BEGIN", "CREATE TABLE sm (smc1 integer)", "UPDATE schema_info SET version = 2", "COMMIT"]
   end
 
   specify "should not use transactions if disabled in the migrator" do
-    Sequel::Migrator.run(@db, "spec/files/transaction_migrations", :use_transactions=>false)
+    Sequel::Migrator.run(@db, "spec/files/transaction_unspecified_migrations", :use_transactions=>false)
     @db.sqls.should == ["CREATE TABLE schema_info (version integer DEFAULT 0 NOT NULL)", "SELECT 1 AS one FROM schema_info LIMIT 1", "INSERT INTO schema_info (version) VALUES (0)", "SELECT version FROM schema_info LIMIT 1", "CREATE TABLE sm11111 (smc1 integer)", "UPDATE schema_info SET version = 1", "CREATE TABLE sm (smc1 integer)", "UPDATE schema_info SET version = 2"]
   end
 end
@@ -598,26 +605,32 @@ def create_table(name, *args, &block)
 
   specify "should use TimestampMigrator if TimestampMigrator.apply is called even for integer migrations directory" do
     Sequel::TimestampMigrator.apply(@db, "spec/files/integer_migrations")
-    @db.sqls.should == ["SELECT NULL FROM schema_migrations LIMIT 1", "CREATE TABLE schema_migrations (filename varchar(255) PRIMARY KEY)", "SELECT NULL FROM schema_info LIMIT 1", "SELECT filename FROM schema_migrations ORDER BY filename", "BEGIN", "CREATE TABLE sm1111 (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('001_create_sessions.rb')", "COMMIT", "BEGIN", "CREATE TABLE sm2222 (smc2 integer)", "INSERT INTO schema_migrations (filename) VALUES ('002_create_nodes.rb')", "COMMIT", "BEGIN", "CREATE TABLE sm3333 (smc3 integer)", "INSERT INTO schema_migrations (filename) VALUES ('003_3_create_users.rb')", "COMMIT"]
+    @db.sqls.should == ["SELECT NULL FROM schema_migrations LIMIT 1", "CREATE TABLE schema_migrations (filename varchar(255) PRIMARY KEY)", "SELECT NULL FROM schema_info LIMIT 1", "SELECT filename FROM schema_migrations ORDER BY filename", "CREATE TABLE sm1111 (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('001_create_sessions.rb')", "CREATE TABLE sm2222 (smc2 integer)", "INSERT INTO schema_migrations (filename) VALUES ('002_create_nodes.rb')", "CREATE TABLE sm3333 (smc3 integer)", "INSERT INTO schema_migrations (filename) VALUES ('003_3_create_users.rb')"]
   end
 
-  specify "should use transactions by default" do
-    Sequel::TimestampMigrator.apply(@db, "spec/files/transaction_migrations")
+  specify "should not use transactions by default" do
+    Sequel::TimestampMigrator.apply(@db, "spec/files/transaction_unspecified_migrations")
+    @db.sqls.should == ["SELECT NULL FROM schema_migrations LIMIT 1", "CREATE TABLE schema_migrations (filename varchar(255) PRIMARY KEY)", "SELECT NULL FROM schema_info LIMIT 1", "SELECT filename FROM schema_migrations ORDER BY filename", "CREATE TABLE sm11111 (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('001_create_alt_basic.rb')", "CREATE TABLE sm (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('002_create_basic.rb')"]
+  end
+
+  specify "should use transactions by default if database supports transactional ddl" do
+    @db.meta_def(:supports_transactional_ddl?){true}
+    Sequel::TimestampMigrator.apply(@db, "spec/files/transaction_unspecified_migrations")
     @db.sqls.should == ["SELECT NULL FROM schema_migrations LIMIT 1", "CREATE TABLE schema_migrations (filename varchar(255) PRIMARY KEY)", "SELECT NULL FROM schema_info LIMIT 1", "SELECT filename FROM schema_migrations ORDER BY filename", "BEGIN", "CREATE TABLE sm11111 (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('001_create_alt_basic.rb')", "COMMIT", "BEGIN", "CREATE TABLE sm (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('002_create_basic.rb')", "COMMIT"]
   end
 
-  specify "should not use transactions for migrations that disable it" do
-    Sequel::TimestampMigrator.apply(@db, "spec/files/transactionless_migrations")
-    @db.sqls.should == ["SELECT NULL FROM schema_migrations LIMIT 1", "CREATE TABLE schema_migrations (filename varchar(255) PRIMARY KEY)", "SELECT NULL FROM schema_info LIMIT 1", "SELECT filename FROM schema_migrations ORDER BY filename", "CREATE TABLE sm11111 (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('001_create_alt_basic.rb')", "CREATE TABLE sm (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('002_create_basic.rb')"]
+  specify "should support transaction use on a per migration basis" do
+    Sequel::TimestampMigrator.apply(@db, "spec/files/transaction_specified_migrations")
+    @db.sqls.should == ["SELECT NULL FROM schema_migrations LIMIT 1", "CREATE TABLE schema_migrations (filename varchar(255) PRIMARY KEY)", "SELECT NULL FROM schema_info LIMIT 1", "SELECT filename FROM schema_migrations ORDER BY filename", "BEGIN", "CREATE TABLE sm11111 (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('001_create_alt_basic.rb')", "COMMIT", "CREATE TABLE sm (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('002_create_basic.rb')"]
   end
 
   specify "should force transactions if enabled by the migrator" do
-    Sequel::TimestampMigrator.run(@db, "spec/files/transactionless_migrations", :use_transactions=>true)
+    Sequel::TimestampMigrator.run(@db, "spec/files/transaction_specified_migrations", :use_transactions=>true)
     @db.sqls.should == ["SELECT NULL FROM schema_migrations LIMIT 1", "CREATE TABLE schema_migrations (filename varchar(255) PRIMARY KEY)", "SELECT NULL FROM schema_info LIMIT 1", "SELECT filename FROM schema_migrations ORDER BY filename", "BEGIN", "CREATE TABLE sm11111 (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('001_create_alt_basic.rb')", "COMMIT", "BEGIN", "CREATE TABLE sm (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('002_create_basic.rb')", "COMMIT"]
   end
 
   specify "should not use transactions if disabled in the migrator" do
-    Sequel::TimestampMigrator.run(@db, "spec/files/transaction_migrations", :use_transactions=>false)
+    Sequel::TimestampMigrator.run(@db, "spec/files/transaction_unspecified_migrations", :use_transactions=>false)
     @db.sqls.should == ["SELECT NULL FROM schema_migrations LIMIT 1", "CREATE TABLE schema_migrations (filename varchar(255) PRIMARY KEY)", "SELECT NULL FROM schema_info LIMIT 1", "SELECT filename FROM schema_migrations ORDER BY filename", "CREATE TABLE sm11111 (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('001_create_alt_basic.rb')", "CREATE TABLE sm (smc1 integer)", "INSERT INTO schema_migrations (filename) VALUES ('002_create_basic.rb')"]
   end
 end

spec/files/transactionless_migrations/001_create_alt_basic.rb → spec/files/transaction_specified_migrations/001_create_alt_basic.rb

@@ -1,4 +1,4 @@
 Sequel.migration do
-  no_transaction
+  transaction
   change{create_table(:sm11111){Integer :smc1}}
 end

spec/integration/schema_test.rb

@@ -250,6 +250,12 @@
     @ds.columns!.should == [:number]
   end
 
+  specify "should handle create table in a rolled back transaction" do
+    @db.drop_table?(:items)
+    @db.transaction(:rollback=>:always){@db.create_table(:items){Integer :number}}
+    @db.table_exists?(:items).should be_false
+  end if INTEGRATION_DB.supports_transactional_ddl?
+
   describe "join tables" do
     after do
       @db.drop_join_table(:cat_id=>:cats, :dog_id=>:dogs) if @db.table_exists?(:cats_dogs)