Unnecessary BEGIN/COMMIT happens when model save is a no-op

delayed_begin_on_rails_2.2.2.patch

diff --git a/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/abstract/database_statements.rb b/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/abstract/database_statements.rb
index 97c6cd4..28f1318 100644
--- a/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/abstract/database_statements.rb
+++ b/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/abstract/database_statements.rb
@@ -36,9 +36,15 @@ module ActiveRecord
 
       # Executes the SQL statement in the context of this connection.
       def execute(sql, name = nil)
-        raise NotImplementedError, "execute is an abstract method"
+        check_delayed_open_transaction
+        execute_direct(sql, name)
       end
 
+      # Executes the SQL statement in the context of this connection.
+      def execute_direct(sql, name = nil)
+        raise NotImplementedError, "execute_direct is an abstract method"
+      end
+      
       # Returns the last auto-generated ID from the affected table.
       def insert(sql, name = nil, pk = nil, id_value = nil, sequence_name = nil)
         insert_sql(sql, name, pk, id_value, sequence_name)
@@ -60,7 +66,7 @@ module ActiveRecord
         begin
           if block_given?
             if start_db_transaction
-              begin_db_transaction
+              @delayed_open_transaction = true
               transaction_open = true
             end
             yield
@@ -68,19 +74,23 @@ module ActiveRecord
         rescue Exception => database_transaction_rollback
           if transaction_open
             transaction_open = false
-            rollback_db_transaction
+            rollback_db_transaction if !@delayed_open_transaction
+            @delayed_open_transaction = false
           end
           raise unless database_transaction_rollback.is_a? ActiveRecord::Rollback
         end
       ensure
-        if transaction_open
+        if transaction_open && !@delayed_open_transaction
           begin
             commit_db_transaction
           rescue Exception => database_transaction_rollback
             rollback_db_transaction
+            @delayed_open_transaction = false
             raise
           end
         end
+
+        @delayed_open_transaction = false
       end
 
       # Begins the transaction (and turns off auto-committing).
@@ -93,6 +103,20 @@ module ActiveRecord
       # done if the transaction block raises an exception or returns false.
       def rollback_db_transaction() end
 
+      # Checks to see if the begin of a transaction is currently delayed.
+      def delayed_open_transaction
+        @delayed_open_transaction
+      end
+
+      # Checks to see if the begin of a transaction is currently delayed, and,
+      # if so, actually begins it.
+      def check_delayed_open_transaction
+        if @delayed_open_transaction
+          begin_db_transaction
+          @delayed_open_transaction = false
+        end
+      end
+
       # Alias for <tt>add_limit_offset!</tt>.
       def add_limit!(sql, options)
         add_limit_offset!(sql, options) if options
diff --git a/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/mysql_adapter.rb b/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/mysql_adapter.rb
index 1d8dce2..ec9a5e3 100644
--- a/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/mysql_adapter.rb
+++ b/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/mysql_adapter.rb
@@ -306,7 +306,7 @@ module ActiveRecord
         rows
       end
 
-      def execute(sql, name = nil) #:nodoc:
+      def execute_direct(sql, name = nil) #:nodoc:
         log(sql, name) { @connection.query(sql) }
       rescue ActiveRecord::StatementInvalid => exception
         if exception.message.split(":").first =~ /Packets out of order/
@@ -327,7 +327,7 @@ module ActiveRecord
       end
 
       def begin_db_transaction #:nodoc:
-        execute "BEGIN"
+        execute_direct "BEGIN"
       rescue Exception
         # Transactions aren't supported
       end
diff --git a/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/postgresql_adapter.rb b/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/postgresql_adapter.rb
index c3adcaf..a08b08f 100644
--- a/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -503,30 +503,50 @@ module ActiveRecord
 
       # Queries the database and returns the results in an Array-like object
       def query(sql, name = nil) #:nodoc:
+        check_delayed_open_transaction
+        query_direct(sql, name)
+      end
+
+      # Queries the database and returns the results in an Array-like object
+      def query_direct(sql, name = nil) #:nodoc:
         log(sql, name) do
-          res = execute_direct_with_retry(sql)
+          res = raw_execute_with_retry(sql)
           return result_as_array(res)
         end
       end
 
+      alias :execute_with_retry :execute
+
+      def execute_without_retry(sql, name = nil)
+        check_delayed_open_transaction
+        execute_direct_without_retry(sql, name)
+      end
+
+      # Executes an SQL statement, returning a PGresult object on success
+      # or raising a PGError exception otherwise.
+      def execute_direct_with_retry(sql, name = nil) #:nodoc:
+        log(sql, name) { raw_execute_with_retry(sql) }
+      end
+
+      alias :execute_direct :execute_direct_with_retry
+
       # Executes an SQL statement, returning a PGresult object on success
       # or raising a PGError exception otherwise.
-      def execute(sql, name = nil)
-        log(sql, name) { execute_direct_without_retry(sql) }
+      def execute_direct_without_retry(sql, name = nil)
+        log(sql, name) { raw_execute_without_retry(sql) }
       end
 
       # Executes an SQL statement, returning a PGresult object on success
       # or raising a PGError exception otherwise.
-      def execute_with_retry(sql, name = nil) #:nodoc:
-        log(sql, name) { execute_direct_with_retry(sql) }
+      def raw_execute_with_retry(sql)
+        with_auto_reconnect { raw_execute_without_retry(sql) }
       end
 
-      # Keep the original execute method so we can run it without retry logic
-      alias_method_chain :execute, :retry
+      alias :raw_execute :raw_execute_with_retry
 
       # Executes an SQL statement, returning a PGresult object on success
       # or raising a PGError exception otherwise.
-      def execute_direct(sql)
+      def raw_execute_without_retry(sql)
         if @async
           @connection.async_exec(sql)
         else
@@ -534,15 +554,6 @@ module ActiveRecord
         end
       end
 
-      # Executes an SQL statement, returning a PGresult object on success
-      # or raising a PGError exception otherwise.
-      def execute_direct_with_retry(sql)
-        with_auto_reconnect { execute_direct_without_retry(sql) }
-      end
-
-      # Keep the original execute_direct method so we can run it without retry logic
-      alias_method_chain :execute_direct, :retry
-
       # Executes an UPDATE query and returns the number of affected tuples.
       def update_sql(sql, name = nil)
         super.cmd_tuples
@@ -550,17 +561,17 @@ module ActiveRecord
 
       # Begins a transaction.
       def begin_db_transaction
-        execute "BEGIN"
+        execute_direct_with_retry "BEGIN"
       end
 
       # Commits a transaction.
       def commit_db_transaction
-        execute_without_retry "COMMIT"
+        execute_without_retry "COMMIT" if transaction_active?
       end
 
       # Aborts a transaction.
       def rollback_db_transaction
-        execute_without_retry "ROLLBACK"
+        execute_without_retry "ROLLBACK" if transaction_active?
       end
 
       # ruby-pg defines Ruby constants for transaction status,
@@ -572,36 +583,6 @@ module ActiveRecord
         @connection.transaction_status != PQTRANS_IDLE
       end
 
-      # Wrap a block in a transaction.  Returns result of block.
-      def transaction(start_db_transaction = true)
-        transaction_open = false
-        begin
-          if block_given?
-            if start_db_transaction
-              begin_db_transaction
-              transaction_open = true
-            end
-            yield
-          end
-        rescue Exception => database_transaction_rollback
-          if transaction_open && transaction_active?
-            transaction_open = false
-            rollback_db_transaction
-          end
-          raise unless database_transaction_rollback.is_a? ActiveRecord::Rollback
-        end
-      ensure
-        if transaction_open && transaction_active?
-          begin
-            commit_db_transaction
-          rescue Exception => database_transaction_rollback
-            rollback_db_transaction
-            raise
-          end
-        end
-      end
-
-
       # SCHEMA STATEMENTS ========================================
 
       def recreate_database(name) #:nodoc:
@@ -1100,8 +1081,9 @@ module ActiveRecord
 
       def select_raw(sql, name = nil)
         res = results = nil
+        check_delayed_open_transaction
         log(sql, name) do
-          res = execute_direct_with_retry(sql)
+          res = raw_execute_with_retry(sql)
           results = result_as_array(res)
         end
         fields = []
diff --git a/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/sqlite_adapter.rb b/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/sqlite_adapter.rb
index 84f8c02..372da8b 100644
--- a/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/sqlite_adapter.rb
+++ b/vmdb/vendor/gems/activerecord-2.2.2/lib/active_record/connection_adapters/sqlite_adapter.rb
@@ -128,7 +128,7 @@ module ActiveRecord
 
       # DATABASE STATEMENTS ======================================
 
-      def execute(sql, name = nil) #:nodoc:
+      def execute_direct(sql, name = nil) #:nodoc:
         catch_schema_changes { log(sql, name) { @connection.execute(sql) } }
       end
 
diff --git a/vmdb/vendor/gems/activerecord-2.2.2/test/cases/miq_cases/miq_migration_test.rb b/vmdb/vendor/gems/activerecord-2.2.2/test/cases/miq_cases/miq_migration_test.rb
index 50cbed5..86ffcb1 100644
--- a/vmdb/vendor/gems/activerecord-2.2.2/test/cases/miq_cases/miq_migration_test.rb
+++ b/vmdb/vendor/gems/activerecord-2.2.2/test/cases/miq_cases/miq_migration_test.rb
@@ -31,12 +31,14 @@ module MiqMigrationTest
     Person.connection.drop_table :testings rescue nil
   end
 
-  def test_add_integer_column_with_out_of_range_limit
-    Person.connection.create_table :testings do |t|
+  if current_adapter?(:PostgreSQLAdapter)
+    def test_add_integer_column_with_out_of_range_limit
+      Person.connection.create_table :testings do |t|
+      end
+      assert_nothing_raised { Person.connection.add_column :testings, :bar, :integer, :limit => 20 }
+    ensure
+      Person.connection.drop_table :testings rescue nil
     end
-    assert_nothing_raised { Person.connection.add_column :testings, :bar, :integer, :limit => 20 }
-  ensure
-    Person.connection.drop_table :testings rescue nil
   end
 end
 
diff --git a/vmdb/vendor/gems/activerecord-2.2.2/test/cases/miq_cases/miq_transactions_test.rb b/vmdb/vendor/gems/activerecord-2.2.2/test/cases/miq_cases/miq_transactions_test.rb
new file mode 100644
index 0000000..584e1ea
--- /dev/null
+++ b/vmdb/vendor/gems/activerecord-2.2.2/test/cases/miq_cases/miq_transactions_test.rb
@@ -0,0 +1,73 @@
+# 
+#  Copyright (c) 2010 ManageIQ, Inc. All rights reserved.
+#
+
+module MiqTransactionsTest
+  #TODO: implement delay for SQL Server
+  unless current_adapter?(:SQLServerAdapter)
+    def self.included(other)
+      other.class_eval do
+        undef test_rollback_when_commit_raises
+      end
+    end
+
+    def test_delayed_open_transaction
+      assert !Topic.connection.delayed_open_transaction, "Before start, transaction should not be delayed"
+      #    assert !Topic.connection.transaction_active?, "Before start, transaction should not be active" if current_adapter?(:PostgreSQLAdapter)
+
+      Topic.transaction do
+        assert Topic.connection.delayed_open_transaction, "Before first execute, transaction should be delayed"
+        #      assert !Topic.connection.transaction_active?, "Before first execute, transaction should not be active" if current_adapter?(:PostgreSQLAdapter)
+
+        @first.approved = true
+        @first.save
+
+        assert !Topic.connection.delayed_open_transaction, "After execute, transaction should no longer be delayed"
+        #      assert Topic.connection.transaction_active?, "After execute, transaction should be active" if current_adapter?(:PostgreSQLAdapter)
+      end
+
+      assert !Topic.connection.delayed_open_transaction, "After complete, transaction should not be delayed"
+      #    assert !Topic.connection.transaction_active?, "After complete, transaction should not be active" if current_adapter?(:PostgreSQLAdapter)
+    end
+
+    def test_delayed_open_transaction_closed_on_error
+      begin
+        Topic.transaction do
+          raise RuntimeError
+        end
+      rescue
+        # caught it
+      end
+
+      assert !Topic.connection.delayed_open_transaction, "Error should not leave delayed open transaction still open"
+      #    assert !Topic.connection.transaction_active?, "Error should not leave transaction active" if current_adapter?(:PostgreSQLAdapter)
+    end
+
+    uses_mocha 'mocking connection.commit_db_transaction' do
+      def test_rollback_when_commit_raises
+        Topic.connection.expects(:begin_db_transaction)
+        Topic.connection.expects(:commit_db_transaction).raises('OH NOES')
+        Topic.connection.expects(:rollback_db_transaction)
+
+        assert_raise RuntimeError do
+          Topic.transaction do
+            # Force the connection to not delay the begin db transaction
+            Topic.connection.check_delayed_open_transaction
+          end
+        end
+      end
+
+      def test_delayed_open_transaction_should_not_begin_on_no_op
+        d = Developer.first
+
+        Developer.connection.expects(:begin_db_transaction).never
+        Developer.connection.expects(:commit_db_transaction).never
+        Developer.connection.expects(:rollback_db_transaction).never
+        Developer.connection.expects(:execute_direct).never
+        Developer.connection.expects(:execute_direct_with_retry).never if current_adapter?(:PostgreSQLAdapter)
+
+        d.save
+      end
+    end
+  end
+end
\ No newline at end of file
diff --git a/vmdb/vendor/gems/activerecord-2.2.2/test/cases/transactions_test.rb b/vmdb/vendor/gems/activerecord-2.2.2/test/cases/transactions_test.rb
index b12ec36..0ef1100 100644
--- a/vmdb/vendor/gems/activerecord-2.2.2/test/cases/transactions_test.rb
+++ b/vmdb/vendor/gems/activerecord-2.2.2/test/cases/transactions_test.rb
@@ -254,6 +254,9 @@ class TransactionTest < ActiveRecord::TestCase
     end
   end
 
+  require 'cases/miq_cases/miq_transactions_test'
+  include MiqTransactionsTest
+
   private
     def add_exception_raising_after_save_callback_to_topic
       Topic.class_eval { def after_save() raise "Make the transaction rollback" end }