Don't use after commit/rollback database hooks if the model instance …

…methods are not overridden

This is faster than the previous code, at least when the defaults
are used.  In benchmarking with a mock database, it's about 10%
faster for creates and 25% faster for updates.  Note there is a
2-3% performance decrease for users who set
use_after_commit_rollback = false manually, but I think this is
a better default.

The main advantage of doing this is that it automatically handles
cases that were handled poorly by the previous default of always
using after commit/rollback hooks.  The previous default did
not work well when using prepared transactions
(which doesn't work with after commit/rollback hooks).  The
previous default also caused performance issues for transactions
that saved many objects, as it created multiple closures for
every object saved, keeping all closures and objects in memory
until the transaction was committed/rolled back.

In Sequel 5, after commit/rollback model hooks will be removed
by default, and you'll have to use a plugin to enable them.
It's recommended that any users using the model hooks switch to
using the database level hooks.

Note that this breaks backward compatibility slightly, in that
use_after_commit_rollback now returns nil instead of true by
default, and setting the value to nil will keep the default
of checking for overridden methods, instead of treating it
like a false value.

CHANGELOG

@@ -1,5 +1,7 @@
 === HEAD
 
+* Don't use after commit/rollback database hooks if the model instance methods are not overridden (jeremyevans)
+
 * Add SQL::NumericMethods#coerce, allowing code such as Sequel.expr{1 - x} (jeremyevans)
 
 * Support ** operator for exponentiation on expressions, similar to +, -, *, and / (jeremyevans)

lib/sequel/model.rb

@@ -117,7 +117,7 @@ class Model
     @strict_param_setting = true
     @typecast_empty_string_to_nil = true
     @typecast_on_assignment = true
-    @use_after_commit_rollback = true
+    @use_after_commit_rollback = nil
     @use_transactions = true
 
     Sequel.require %w"default_inflections inflections plugins dataset_module base exceptions errors", "model"

lib/sequel/model/base.rb

@@ -1946,7 +1946,8 @@ def _delete_without_checking
       # allow running inside a transaction
       def _destroy(opts)
         sh = {:server=>this_server}
-        db.after_rollback(sh){after_destroy_rollback} if uacr = use_after_commit_rollback
+        uacr = use_after_commit_rollback
+        db.after_rollback(sh){after_destroy_rollback} if uacr.nil? ? (method(:after_destroy_rollback).owner != InstanceMethods) : uacr
         called = false
         around_destroy do
           called = true
@@ -1956,7 +1957,7 @@ def _destroy(opts)
           true
         end
         raise_hook_failure(:around_destroy) unless called
-        db.after_commit(sh){after_destroy_commit} if uacr
+        db.after_commit(sh){after_destroy_commit} if uacr.nil? ? (method(:after_destroy_commit).owner != InstanceMethods) : uacr
         self
       end
 
@@ -2025,7 +2026,8 @@ def _refresh_set_values(h)
       # it's own transaction.
       def _save(opts)
         sh = {:server=>this_server}
-        db.after_rollback(sh){after_rollback} if uacr = use_after_commit_rollback
+        uacr = use_after_commit_rollback
+        db.after_rollback(sh){after_rollback} if uacr.nil? ? (method(:after_rollback).owner != InstanceMethods) : uacr
         pk = nil
         called_save = false
         called_cu = false
@@ -2071,7 +2073,7 @@ def _save(opts)
         end
         raise_hook_failure(:around_save) unless called_save
         _after_save(pk)
-        db.after_commit(sh){after_commit} if uacr
+        db.after_commit(sh){after_commit} if uacr.nil? ? (method(:after_commit).owner != InstanceMethods) : uacr
         self
       end
 

spec/model/hooks_spec.rb

@@ -601,4 +601,44 @@ def after_destroy_rollback
     o.save
     @db.sqls.must_equal ['BEGIN', 'as', 'COMMIT']
   end
+
+  it "should handle use_after_commit_rollback when subclassing after loading" do
+    @m = Class.new(Sequel::Model(@db[:items]))
+    @m.use_transactions = true
+    o = @m.load({})
+    @db.sqls
+    o.save
+    @db.sqls.must_equal ['BEGIN', 'COMMIT']
+
+    c = Class.new(@m)
+    o = c.load({})
+    def o.after_commit; db.execute 'ac' end
+    @db.sqls
+    o.save
+    @db.sqls.must_equal ['BEGIN', 'COMMIT', 'ac']
+
+    o = c.load({})
+    @db.sqls
+    o.save
+    @db.sqls.must_equal ['BEGIN', 'COMMIT']
+
+    c.send(:define_method, :after_commit){db.execute 'ac'}
+    o = c.load({})
+    @db.sqls
+    o.save
+    @db.sqls.must_equal ['BEGIN', 'COMMIT', 'ac']
+
+    c = Class.new(@m)
+    c.send(:include, Module.new{def after_commit; db.execute 'ac'; end})
+    o = c.load({})
+    @db.sqls
+    o.save
+    @db.sqls.must_equal ['BEGIN', 'COMMIT', 'ac']
+
+    c.use_after_commit_rollback = false
+    o = c.load({})
+    @db.sqls
+    o.save
+    @db.sqls.must_equal ['BEGIN', 'COMMIT']
+  end
 end

spec/model/record_spec.rb

@@ -1304,8 +1304,9 @@ def set_column_value(c, v)
   end
 
   it "#set_all should set not set restricted fields" do
-    @o1.set_all(:x => 1, :use_after_commit_rollback => false)
-    @o1.use_after_commit_rollback.must_equal true
+    @o1.use_after_commit_rollback.must_equal nil
+    @o1.set_all(:x => 1, :use_after_commit_rollback => true)
+    @o1.use_after_commit_rollback.must_equal nil
     @o1.values.must_equal(:x => 1)
   end