Merge pull request #32647 from eugeneius/lazy_transactions

Omit BEGIN/COMMIT statements for empty transactions

activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb

@@ -259,7 +259,9 @@ def transaction(requires_new: nil, isolation: nil, joinable: true)
 
       attr_reader :transaction_manager #:nodoc:
 
-      delegate :within_new_transaction, :open_transactions, :current_transaction, :begin_transaction, :commit_transaction, :rollback_transaction, to: :transaction_manager
+      delegate :within_new_transaction, :open_transactions, :current_transaction, :begin_transaction,
+               :commit_transaction, :rollback_transaction, :materialize_transactions,
+               :disable_lazy_transactions!, :enable_lazy_transactions!, to: :transaction_manager
 
       def transaction_open?
         current_transaction.open?

activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

@@ -91,12 +91,14 @@ def add_record(record); end
     end
 
     class Transaction #:nodoc:
-      attr_reader :connection, :state, :records, :savepoint_name
+      attr_reader :connection, :state, :records, :savepoint_name, :isolation_level
 
       def initialize(connection, options, run_commit_callbacks: false)
         @connection = connection
         @state = TransactionState.new
         @records = []
+        @isolation_level = options[:isolation]
+        @materialized = false
         @joinable = options.fetch(:joinable, true)
         @run_commit_callbacks = run_commit_callbacks
       end
@@ -105,6 +107,14 @@ def add_record(record)
         records << record
       end
 
+      def materialize!
+        @materialized = true
+      end
+
+      def materialized?
+        @materialized
+      end
+
       def rollback_records
         ite = records.uniq
         while record = ite.shift
@@ -141,24 +151,30 @@ def open?; !closed?; end
     end
 
     class SavepointTransaction < Transaction
-      def initialize(connection, savepoint_name, parent_transaction, options, *args)
-        super(connection, options, *args)
+      def initialize(connection, savepoint_name, parent_transaction, *args)
+        super(connection, *args)
 
         parent_transaction.state.add_child(@state)
 
-        if options[:isolation]
+        if isolation_level
           raise ActiveRecord::TransactionIsolationError, "cannot set transaction isolation in a nested transaction"
         end
-        connection.create_savepoint(@savepoint_name = savepoint_name)
+
+        @savepoint_name = savepoint_name
+      end
+
+      def materialize!
+        connection.create_savepoint(savepoint_name)
+        super
       end
 
       def rollback
-        connection.rollback_to_savepoint(savepoint_name)
+        connection.rollback_to_savepoint(savepoint_name) if materialized?
         @state.rollback!
       end
 
       def commit
-        connection.release_savepoint(savepoint_name)
+        connection.release_savepoint(savepoint_name) if materialized?
         @state.commit!
       end
 
@@ -166,22 +182,23 @@ def full_rollback?; false; end
     end
 
     class RealTransaction < Transaction
-      def initialize(connection, options, *args)
-        super
-        if options[:isolation]
-          connection.begin_isolated_db_transaction(options[:isolation])
+      def materialize!
+        if isolation_level
+          connection.begin_isolated_db_transaction(isolation_level)
         else
           connection.begin_db_transaction
         end
+
+        super
       end
 
       def rollback
-        connection.rollback_db_transaction
+        connection.rollback_db_transaction if materialized?
         @state.full_rollback!
       end
 
       def commit
-        connection.commit_db_transaction
+        connection.commit_db_transaction if materialized?
         @state.full_commit!
       end
     end
@@ -190,6 +207,9 @@ class TransactionManager #:nodoc:
       def initialize(connection)
         @stack = []
         @connection = connection
+        @has_unmaterialized_transactions = false
+        @materializing_transactions = false
+        @lazy_transactions_enabled = true
       end
 
       def begin_transaction(options = {})
@@ -203,11 +223,41 @@ def begin_transaction(options = {})
                                        run_commit_callbacks: run_commit_callbacks)
             end
 
+          transaction.materialize! unless @connection.supports_lazy_transactions? && lazy_transactions_enabled?
           @stack.push(transaction)
+          @has_unmaterialized_transactions = true if @connection.supports_lazy_transactions?
           transaction
         end
       end
 
+      def disable_lazy_transactions!
+        materialize_transactions
+        @lazy_transactions_enabled = false
+      end
+
+      def enable_lazy_transactions!
+        @lazy_transactions_enabled = true
+      end
+
+      def lazy_transactions_enabled?
+        @lazy_transactions_enabled
+      end
+
+      def materialize_transactions
+        return if @materializing_transactions
+        return unless @has_unmaterialized_transactions
+
+        @connection.lock.synchronize do
+          begin
+            @materializing_transactions = true
+            @stack.each { |t| t.materialize! unless t.materialized? }
+          ensure
+            @materializing_transactions = false
+          end
+          @has_unmaterialized_transactions = false
+        end
+      end
+
       def commit_transaction
         @connection.lock.synchronize do
           transaction = @stack.last

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -80,6 +80,8 @@ class AbstractAdapter
       attr_reader :schema_cache, :owner, :logger, :prepared_statements, :lock
       alias :in_use? :owner
 
+      set_callback :checkin, :after, :enable_lazy_transactions!
+
       def self.type_cast_config_to_integer(config)
         if config.is_a?(Integer)
           config
@@ -342,6 +344,10 @@ def supports_foreign_tables?
         false
       end
 
+      def supports_lazy_transactions?
+        false
+      end
+
       # This is meant to be implemented by the adapters that support extensions
       def disable_extension(name)
       end
@@ -453,6 +459,7 @@ def verify!
       # This is useful for when you need to call a proprietary method such as
       # PostgreSQL's lo_* methods.
       def raw_connection
+        disable_lazy_transactions!
         @connection
       end
 

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

@@ -180,6 +180,8 @@ def explain(arel, binds = [])
 
       # Executes the SQL statement in the context of this connection.
       def execute(sql, name = nil)
+        materialize_transactions
+
         log(sql, name) do
           ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
             @connection.query(sql)

activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb

@@ -29,6 +29,8 @@ def execute(sql, name = nil)
         end
 
         def exec_query(sql, name = "SQL", binds = [], prepare: false)
+          materialize_transactions
+
           if without_prepared_statement?(binds)
             execute_and_free(sql, name) do |result|
               ActiveRecord::Result.new(result.fields, result.to_a) if result
@@ -41,6 +43,8 @@ def exec_query(sql, name = "SQL", binds = [], prepare: false)
         end
 
         def exec_delete(sql, name = nil, binds = [])
+          materialize_transactions
+
           if without_prepared_statement?(binds)
             execute_and_free(sql, name) { @connection.affected_rows }
           else

activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb

@@ -58,6 +58,10 @@ def supports_savepoints?
         true
       end
 
+      def supports_lazy_transactions?
+        true
+      end
+
       # HELPER METHODS ===========================================
 
       def each_hash(result) # :nodoc:

activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb

@@ -58,6 +58,8 @@ def result_as_array(res) #:nodoc:
 
         # Queries the database and returns the results in an Array-like object
         def query(sql, name = nil) #:nodoc:
+          materialize_transactions
+
           log(sql, name) do
             ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
               result_as_array @connection.async_exec(sql)
@@ -70,6 +72,8 @@ def query(sql, name = nil) #:nodoc:
         # Note: the PG::Result object is manually memory managed; if you don't
         # need it specifically, you may want consider the <tt>exec_query</tt> wrapper.
         def execute(sql, name = nil)
+          materialize_transactions
+
           log(sql, name) do
             ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
               @connection.async_exec(sql)

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

@@ -326,6 +326,10 @@ def supports_pgcrypto_uuid?
         postgresql_version >= 90400
       end
 
+      def supports_lazy_transactions?
+        true
+      end
+
       def get_advisory_lock(lock_id) # :nodoc:
         unless lock_id.is_a?(Integer) && lock_id.bit_length <= 63
           raise(ArgumentError, "PostgreSQL requires advisory lock ids to be a signed 64 bit integer")
@@ -597,6 +601,8 @@ def execute_and_clear(sql, name, binds, prepare: false)
         end
 
         def exec_no_cache(sql, name, binds)
+          materialize_transactions
+
           type_casted_binds = type_casted_binds(binds)
           log(sql, name, binds, type_casted_binds) do
             ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
@@ -606,6 +612,8 @@ def exec_no_cache(sql, name, binds)
         end
 
         def exec_cache(sql, name, binds)
+          materialize_transactions
+
           stmt_key = prepare_statement(sql)
           type_casted_binds = type_casted_binds(binds)
 

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -186,6 +186,10 @@ def supports_explain?
         true
       end
 
+      def supports_lazy_transactions?
+        true
+      end
+
       # REFERENTIAL INTEGRITY ====================================
 
       def disable_referential_integrity # :nodoc:
@@ -212,6 +216,8 @@ def explain(arel, binds = [])
       end
 
       def exec_query(sql, name = nil, binds = [], prepare: false)
+        materialize_transactions
+
         type_casted_binds = type_casted_binds(binds)
 
         log(sql, name, binds, type_casted_binds) do
@@ -252,6 +258,8 @@ def last_inserted_id(result)
       end
 
       def execute(sql, name = nil) #:nodoc:
+        materialize_transactions
+
         log(sql, name) do
           ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
             @connection.execute(sql)

activerecord/test/cases/adapter_test.rb

@@ -10,6 +10,7 @@ module ActiveRecord
   class AdapterTest < ActiveRecord::TestCase
     def setup
       @connection = ActiveRecord::Base.connection
+      @connection.materialize_transactions
     end
 
     ##

activerecord/test/cases/adapters/mysql2/connection_test.rb

@@ -170,6 +170,8 @@ def test_mysql_set_session_variable_to_default
   end
 
   def test_logs_name_show_variable
+    ActiveRecord::Base.connection.materialize_transactions
+    @subscriber.logged.clear
     @connection.show_variable "foo"
     assert_equal "SCHEMA", @subscriber.logged[0][1]
   end

activerecord/test/cases/adapters/postgresql/active_schema_test.rb

@@ -4,6 +4,8 @@
 
 class PostgresqlActiveSchemaTest < ActiveRecord::PostgreSQLTestCase
   def setup
+    ActiveRecord::Base.connection.materialize_transactions
+
     ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.class_eval do
       def execute(sql, name = nil) sql end
     end

activerecord/test/cases/adapters/postgresql/connection_test.rb

@@ -15,8 +15,9 @@ class NonExistentTable < ActiveRecord::Base
     def setup
       super
       @subscriber = SQLSubscriber.new
-      @subscription = ActiveSupport::Notifications.subscribe("sql.active_record", @subscriber)
       @connection = ActiveRecord::Base.connection
+      @connection.materialize_transactions
+      @subscription = ActiveSupport::Notifications.subscribe("sql.active_record", @subscriber)
     end
 
     def teardown

activerecord/test/cases/log_subscriber_test.rb

@@ -44,6 +44,7 @@ def debug(progname = nil, &block)
   def setup
     @old_logger = ActiveRecord::Base.logger
     Developer.primary_key
+    ActiveRecord::Base.connection.materialize_transactions
     super
     ActiveRecord::LogSubscriber.attach_to(:active_record)
   end

activerecord/test/cases/test_case.rb

@@ -31,6 +31,7 @@ def teardown
     end
 
     def capture_sql
+      ActiveRecord::Base.connection.materialize_transactions
       SQLCounter.clear_log
       yield
       SQLCounter.log_all.dup
@@ -48,6 +49,7 @@ def assert_sql(*patterns_to_match)
 
     def assert_queries(num = 1, options = {})
       ignore_none = options.fetch(:ignore_none) { num == :any }
+      ActiveRecord::Base.connection.materialize_transactions
       SQLCounter.clear_log
       x = yield
       the_log = ignore_none ? SQLCounter.log_all : SQLCounter.log

activerecord/test/cases/transaction_isolation_test.rb

@@ -11,7 +11,7 @@ class Tag < ActiveRecord::Base
 
     test "setting the isolation level raises an error" do
       assert_raises(ActiveRecord::TransactionIsolationError) do
-        Tag.transaction(isolation: :serializable) {}
+        Tag.transaction(isolation: :serializable) { Topic.connection.materialize_transactions }
       end
     end
   end

activerecord/test/cases/transactions_test.rb

@@ -575,7 +575,7 @@ def test_rollback_when_commit_raises
         assert_called(Topic.connection, :rollback_db_transaction) do
           e = assert_raise RuntimeError do
             Topic.transaction do
-              # do nothing
+              Topic.connection.materialize_transactions
             end
           end
           assert_equal "OH NOES", e.message
@@ -943,6 +943,76 @@ def test_transaction_rollback_with_primarykeyless_tables
     connection.drop_table "transaction_without_primary_keys", if_exists: true
   end
 
+  def test_empty_transaction_is_not_materialized
+    assert_no_queries do
+      Topic.transaction {}
+    end
+  end
+
+  def test_unprepared_statement_materializes_transaction
+    assert_sql(/BEGIN/i, /COMMIT/i) do
+      Topic.transaction { Topic.where("1=1").first }
+    end
+  end
+
+  if ActiveRecord::Base.connection.prepared_statements
+    def test_prepared_statement_materializes_transaction
+      Topic.first
+
+      assert_sql(/BEGIN/i, /COMMIT/i) do
+        Topic.transaction { Topic.first }
+      end
+    end
+  end
+
+  def test_savepoint_does_not_materialize_transaction
+    assert_no_queries do
+      Topic.transaction do
+        Topic.transaction(requires_new: true) {}
+      end
+    end
+  end
+
+  def test_raising_does_not_materialize_transaction
+    assert_raise(RuntimeError) do
+      assert_no_queries do
+        Topic.transaction { raise }
+      end
+    end
+  end
+
+  def test_accessing_raw_connection_materializes_transaction
+    assert_sql(/BEGIN/i, /COMMIT/i) do
+      Topic.transaction { Topic.connection.raw_connection }
+    end
+  end
+
+  def test_accessing_raw_connection_disables_lazy_transactions
+    Topic.connection.raw_connection
+
+    assert_sql(/BEGIN/i, /COMMIT/i) do
+      Topic.transaction {}
+    end
+  end
+
+  def test_checking_in_connection_reenables_lazy_transactions
+    connection = Topic.connection_pool.checkout
+    connection.raw_connection
+    Topic.connection_pool.checkin connection
+
+    assert_no_queries do
+      connection.transaction {}
+    end
+  end
+
+  def test_transactions_can_be_manually_materialized
+    assert_sql(/BEGIN/i, /COMMIT/i) do
+      Topic.transaction do
+        Topic.connection.materialize_transactions
+      end
+    end
+  end
+
   private
 
     %w(validation save destroy).each do |filter|