Return early from allow list when possible

Gemfile

@@ -7,6 +7,7 @@ gem "rake-compiler"
 
 group :test do
   gem "benchmark-memory"
+  gem "benchmark-ips"
   gem "memory_profiler"
   gem "minitest"
   gem "mocha"

lib/semian/activerecord_trilogy_adapter.rb

@@ -48,7 +48,7 @@ def initialize(*options)
     end
 
     def raw_execute(sql, *)
-      if query_allowlisted?(sql)
+      if self.query_allowlisted?(sql)
         super
       else
         acquire_semian_resource(adapter: :trilogy_adapter, scope: :query) do
@@ -90,11 +90,34 @@ def resource_exceptions
       ]
     end
 
-    # TODO: share this with Mysql2
     QUERY_ALLOWLIST = %r{\A(?:/\*.*?\*/)?\s*(ROLLBACK|COMMIT|RELEASE\s+SAVEPOINT)}i
 
-    def query_allowlisted?(sql, *)
-      QUERY_ALLOWLIST.match?(sql)
+    def self.query_allowlisted?(sql, *)
+      # The common case here NOT to have transcation management statements,
+      # we are exploiting the fact that ActiveRecord will use COMMIT/ROLLBACK and
+      # name savepoints by level of nesting as `active_record_1` ... n.
+      #
+      # Since looking at the last character in a sring is a LOT cheaper than
+      # running a regex we are returning earlier if the last characters of 
+      # the SQL statements are NOT the last characters of the known transaction 
+      # control patterns
+      #
+      # test/alllow_list_bench.rb as of now (Dec 2023 using ruby 3.1) shows 
+      # this method to be 34x faster in the common case
+      #
+      # But what about achoring the regex? 
+      #
+      # This regex is 1.1x faster in benchmark
+      # %r{\A(?:/\*.*?\*/)?\s*(ROLLBACK|COMMIT|RELEASE\s+SAVEPOINT)\Z}i.match?(sql)
+      #
+      # but then we might miss cases and is not nearly as fast as looking at the last 
+      # characters of the sql
+      if !sql.end_with?('T') || !sql.end_with('K') || !sql.end_with?('_1') || !sql.end_with?('_2')
+        false
+      else
+        QUERY_ALLOWLIST.match?(sql)
+      end
+
     rescue ArgumentError
       return false unless sql.valid_encoding?
 

test/adapters/activerecord_trilogy_adapter_test.rb

@@ -332,23 +332,22 @@ def test_query_allowlisted_returns_false_for_binary_sql
         refute(@adapter.send(:query_allowlisted?, binary_query))
       end
 
-      def test_semian_allows_rollback_to_safepoint
+      def test_semian_allows_set_savepoint
         @adapter.execute("START TRANSACTION;")
-        @adapter.execute("SAVEPOINT foobar;")
 
         Semian[:mysql_testing].acquire do
-          @adapter.execute("ROLLBACK TO foobar;")
+        @adapter.execute("SAVEPOINT active_record_99;")
         end
 
         @adapter.execute("ROLLBACK;")
       end
 
       def test_semian_allows_release_savepoint
         @adapter.execute("START TRANSACTION;")
-        @adapter.execute("SAVEPOINT foobar;")
+        @adapter.execute("SAVEPOINT active_record_99;")
 
         Semian[:mysql_testing].acquire do
-          @adapter.execute("RELEASE SAVEPOINT foobar;")
+          @adapter.execute("RELEASE SAVEPOINT active_record_99;")
         end
 
         @adapter.execute("ROLLBACK;")

test/allow_list_bench.rb

@@ -0,0 +1,20 @@
+require 'semian'
+require 'active_record'
+require 'semian/activerecord_trilogy_adapter'
+require 'benchmark/ips'
+
+sql_commit = "/*key:value,key:morevalue,morekey:morevalue,id:IDIDIDIDI,anotherkey:anothervalue,k:v,k:v*/ COMMIT"
+
+# Common case
+sql_not_commit = "/*key:value,key:morevalue,morekey:morevalue,id:IDIDIDIDID,anotherkey:anothervalue,k:v,k:v*/ SELECT /*+ 1111111111111111111111111*/ `line_items`.`id`, `line_items`.`shop_id`, `line_items`.`title`, `line_items`.`sku`, `line_items`.`vendor`, `line_items`.`variant_id`, `line_items`.`variant_title`, `line_items`.`order_id`, `line_items`.`currency`, `line_items`.`presentment_price`, `line_items`.`price`, `line_items`.`gift_card` FROM `line_items` WHERE `line_items`.`id` = ASKJAKJSDASDKJ"
+
+
+Benchmark.ips do |x|
+  x.report("end-with-case?") do
+    Semian::ActiveRecordTrilogyAdapter.query_allowlisted?(sql_not_commit)
+  end
+
+  x.report("regex") { Semian::ActiveRecordTrilogyAdapter::QUERY_ALLOWLIST.match?(sql_not_commit) }
+  x.compare!
+end
+