Omit BEGIN/COMMIT statements for empty transactions

If a transaction is opened and closed without any queries being run, we
can safely omit the `BEGIN` and `COMMIT` statements, as they only exist
to modify the connection's behaviour inside the transaction. This
removes the overhead of those statements when saving a record with no
changes, which makes workarounds like `save if changed?` unnecessary.

This implementation buffers transactions inside the transaction manager
and materializes them the next time the connection is used. For this to
work, the adapter needs to guard all connection use with a call to
`materialize_transactions`. Because of this, adapters must opt in to get
this new behaviour by implementing `supports_lazy_transactions?`.

If `raw_connection` is used to get a reference to the underlying
database connection, the behaviour is disabled and transactions are
opened eagerly, as we can't know how the connection will be used.
However when the connection is checked back into the pool, we can assume
that the application won't use the reference again and reenable lazy
transactions. This prevents a single `raw_connection` call from
disabling lazy transactions for the lifetime of the connection.

17 files changed, 185 insertions, 17 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
index 41553cfa83..1d36c3c8b1 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
@@ -259,7 +259,9 @@ module ActiveRecord
 
       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?
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
index b59df2fff7..564b226b39 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
@@ -91,12 +91,14 @@ module ActiveRecord
     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 @@ module ActiveRecord
         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 @@ module ActiveRecord
     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 @@ module ActiveRecord
     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 @@ module ActiveRecord
       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 @@ module ActiveRecord
                                        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
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
index a4748dbeda..359cc54cf8 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -80,6 +80,8 @@ module ActiveRecord
       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
@@ -338,6 +340,10 @@ module ActiveRecord
         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
@@ -449,6 +455,7 @@ module ActiveRecord
       # 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
 
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
index 9de8242a58..88fff83a9e 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -180,6 +180,8 @@ module ActiveRecord
 
       # 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)
diff --git a/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb b/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb
index d89eeb7f54..684c7042a7 100644
--- a/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb
@@ -29,6 +29,8 @@ module ActiveRecord
         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 @@ module ActiveRecord
         end
 
         def exec_delete(sql, name = nil, binds = [])
+          materialize_transactions
+
           if without_prepared_statement?(binds)
             execute_and_free(sql, name) { @connection.affected_rows }
           else
diff --git a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
index 544d720428..92f15de219 100644
--- a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
@@ -58,6 +58,10 @@ module ActiveRecord
         true
       end
 
+      def supports_lazy_transactions?
+        true
+      end
+
       # HELPER METHODS ===========================================
 
       def each_hash(result) # :nodoc:
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb
index 8db2a645af..6bd6b67165 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb
@@ -58,6 +58,8 @@ module ActiveRecord
 
         # 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 @@ module ActiveRecord
         # 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)
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index fdf6f75108..3ee344a249 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -326,6 +326,10 @@ module ActiveRecord
         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 @@ module ActiveRecord
         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 @@ module ActiveRecord
         end
 
         def exec_cache(sql, name, binds)
+          materialize_transactions
+
           stmt_key = prepare_statement(sql)
           type_casted_binds = type_casted_binds(binds)
 
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
index bee74dc33d..e0523de484 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
@@ -186,6 +186,10 @@ module ActiveRecord
         true
       end
 
+      def supports_lazy_transactions?
+        true
+      end
+
       # REFERENTIAL INTEGRITY ====================================
 
       def disable_referential_integrity # :nodoc:
@@ -212,6 +216,8 @@ module ActiveRecord
       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 @@ module ActiveRecord
       end
 
       def execute(sql, name = nil) #:nodoc:
+        materialize_transactions
+
         log(sql, name) do
           ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
             @connection.execute(sql)
diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb
index 59b99351d1..67734d24d7 100644
--- a/activerecord/test/cases/adapter_test.rb
+++ b/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
 
     ##
diff --git a/activerecord/test/cases/adapters/mysql2/connection_test.rb b/activerecord/test/cases/adapters/mysql2/connection_test.rb
index 726f58d58e..0c0e2a116e 100644
--- a/activerecord/test/cases/adapters/mysql2/connection_test.rb
+++ b/activerecord/test/cases/adapters/mysql2/connection_test.rb
@@ -170,6 +170,8 @@ class Mysql2ConnectionTest < ActiveRecord::Mysql2TestCase
   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
diff --git a/activerecord/test/cases/adapters/postgresql/active_schema_test.rb b/activerecord/test/cases/adapters/postgresql/active_schema_test.rb
index 308ad1d854..afd422881b 100644
--- a/activerecord/test/cases/adapters/postgresql/active_schema_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/active_schema_test.rb
@@ -4,6 +4,8 @@ require "cases/helper"
 
 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
diff --git a/activerecord/test/cases/adapters/postgresql/connection_test.rb b/activerecord/test/cases/adapters/postgresql/connection_test.rb
index 54b0dde7dc..70aa189893 100644
--- a/activerecord/test/cases/adapters/postgresql/connection_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/connection_test.rb
@@ -15,8 +15,9 @@ module ActiveRecord
     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
diff --git a/activerecord/test/cases/log_subscriber_test.rb b/activerecord/test/cases/log_subscriber_test.rb
index f0126fdb0d..ae2597adc8 100644
--- a/activerecord/test/cases/log_subscriber_test.rb
+++ b/activerecord/test/cases/log_subscriber_test.rb
@@ -44,6 +44,7 @@ class LogSubscriberTest < ActiveRecord::TestCase
   def setup
     @old_logger = ActiveRecord::Base.logger
     Developer.primary_key
+    ActiveRecord::Base.connection.materialize_transactions
     super
     ActiveRecord::LogSubscriber.attach_to(:active_record)
   end
diff --git a/activerecord/test/cases/test_case.rb b/activerecord/test/cases/test_case.rb
index 409b07e56c..40947767f3 100644
--- a/activerecord/test/cases/test_case.rb
+++ b/activerecord/test/cases/test_case.rb
@@ -31,6 +31,7 @@ module ActiveRecord
     end
 
     def capture_sql
+      ActiveRecord::Base.connection.materialize_transactions
       SQLCounter.clear_log
       yield
       SQLCounter.log_all.dup
@@ -48,6 +49,7 @@ module ActiveRecord
 
     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
diff --git a/activerecord/test/cases/transaction_isolation_test.rb b/activerecord/test/cases/transaction_isolation_test.rb
index eaafd13360..8bcf121f15 100644
--- a/activerecord/test/cases/transaction_isolation_test.rb
+++ b/activerecord/test/cases/transaction_isolation_test.rb
@@ -11,7 +11,7 @@ unless ActiveRecord::Base.connection.supports_transaction_isolation?
 
     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
diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb
index 46463ac414..b13cf88c00 100644
--- a/activerecord/test/cases/transactions_test.rb
+++ b/activerecord/test/cases/transactions_test.rb
@@ -575,7 +575,7 @@ class TransactionTest < ActiveRecord::TestCase
         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 @@ class TransactionTest < ActiveRecord::TestCase
     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|