Merge pull request rails#49415 from Shopify/schema-cache-server-version

Move the database version cache from schema cache to pool config

Author: byroot

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

@@ -22,6 +22,16 @@ def method_missing(*)
       end
       NULL_CONFIG = NullConfig.new # :nodoc:
 
+      def initialize
+        super()
+        @mutex = Mutex.new
+        @server_version = nil
+      end
+
+      def server_version(connection) # :nodoc:
+        @server_version || @mutex.synchronize { @server_version ||= connection.get_database_version }
+      end
+
       def schema_reflection
         SchemaReflection.new(nil)
       end
@@ -111,7 +121,7 @@ class ConnectionPool
       attr_accessor :automatic_reconnect, :checkout_timeout
       attr_reader :db_config, :size, :reaper, :pool_config, :async_executor, :role, :shard
 
-      delegate :schema_reflection, :schema_reflection=, to: :pool_config
+      delegate :schema_reflection, :schema_reflection=, :server_version, to: :pool_config
 
       # Creates a new ConnectionPool object. +pool_config+ is a PoolConfig
       # object which describes database connection information (e.g. adapter,

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -868,7 +868,7 @@ def get_database_version # :nodoc:
       end
 
       def database_version # :nodoc:
-        schema_cache.database_version
+        pool.server_version(self)
       end
 
       def check_version # :nodoc:

activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb

@@ -174,7 +174,7 @@ def configure_connection
         end
 
         def full_version
-          schema_cache.database_version.full_version_string
+          database_version.full_version_string
         end
 
         def get_full_version

activerecord/lib/active_record/connection_adapters/pool_config.rb

@@ -3,10 +3,10 @@
 module ActiveRecord
   module ConnectionAdapters
     class PoolConfig # :nodoc:
-      include Mutex_m
+      include MonitorMixin
 
       attr_reader :db_config, :role, :shard
-      attr_writer :schema_reflection
+      attr_writer :schema_reflection, :server_version
       attr_accessor :connection_class
 
       def schema_reflection
@@ -28,6 +28,7 @@ def disconnect_all!
 
       def initialize(connection_class, db_config, role, shard)
         super()
+        @server_version = nil
         @connection_class = connection_class
         @db_config = db_config
         @role = role
@@ -36,6 +37,10 @@ def initialize(connection_class, db_config, role, shard)
         INSTANCES[self] = self
       end
 
+      def server_version(connection)
+        @server_version || synchronize { @server_version ||= connection.get_database_version }
+      end
+
       def connection_name
         if connection_class.primary_class?
           "ActiveRecord::Base"

activerecord/lib/active_record/connection_adapters/schema_cache.rb

@@ -66,10 +66,6 @@ def indexes(connection, table_name)
         cache(connection).indexes(connection, table_name)
       end
 
-      def database_version(connection)
-        cache(connection).database_version(connection)
-      end
-
       def version(connection)
         cache(connection).version(connection)
       end
@@ -196,10 +192,6 @@ def indexes(table_name)
         @schema_reflection.indexes(@connection, table_name)
       end
 
-      def database_version
-        @schema_reflection.database_version(@connection)
-      end
-
       def version
         @schema_reflection.version(@connection)
       end
@@ -264,7 +256,6 @@ def initialize
         @primary_keys = {}
         @data_sources = {}
         @indexes      = {}
-        @database_version = nil
         @version = nil
       end
 
@@ -283,7 +274,6 @@ def encode_with(coder) # :nodoc:
         coder["data_sources"]     = @data_sources.sort.to_h
         coder["indexes"]          = @indexes.sort.to_h
         coder["version"]          = @version
-        coder["database_version"] = @database_version
       end
 
       def init_with(coder)
@@ -293,7 +283,6 @@ def init_with(coder)
         @data_sources     = coder["data_sources"]
         @indexes          = coder["indexes"] || {}
         @version          = coder["version"]
-        @database_version = coder["database_version"]
 
         unless coder["deduplicated"]
           derive_columns_hash_and_deduplicate_values
@@ -370,10 +359,6 @@ def indexes(connection, table_name)
         end
       end
 
-      def database_version(connection) # :nodoc:
-        @database_version ||= connection.get_database_version
-      end
-
       def version(connection)
         @version ||= connection.schema_version
       end
@@ -401,7 +386,6 @@ def add_all(connection) # :nodoc:
         end
 
         version(connection)
-        database_version(connection)
       end
 
       def dump_to(filename)
@@ -415,11 +399,11 @@ def dump_to(filename)
       end
 
       def marshal_dump # :nodoc:
-        [@version, @columns, {}, @primary_keys, @data_sources, @indexes, @database_version]
+        [@version, @columns, {}, @primary_keys, @data_sources, @indexes]
       end
 
       def marshal_load(array) # :nodoc:
-        @version, @columns, _columns_hash, @primary_keys, @data_sources, @indexes, @database_version = array
+        @version, @columns, _columns_hash, @primary_keys, @data_sources, @indexes, _database_version = array
         @indexes ||= {}
 
         derive_columns_hash_and_deduplicate_values

activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb

@@ -202,7 +202,7 @@ def reconnect
         end
 
         def full_version
-          schema_cache.database_version.full_version_string
+          database_version.full_version_string
         end
 
         def get_full_version

activerecord/lib/active_record/railtie.rb

@@ -162,10 +162,13 @@ class Railtie < Rails::Railtie # :nodoc:
                 warn "Failed to validate the schema cache because of #{error.class}: #{error.message}"
                 nil
               end
-              next if current_version.nil?
 
-              if cache.schema_version != current_version
+              if current_version.nil?
+                connection_pool.schema_reflection.clear!
+                next
+              elsif cache.schema_version != current_version
                 warn "Ignoring #{filename} because it has expired. The current schema version is #{current_version}, but the one in the schema cache file is #{cache.schema_version}."
+                connection_pool.schema_reflection.clear!
                 next
               end
             end

activerecord/test/cases/adapters/abstract_mysql_adapter/datetime_precision_quoting_test.rb

@@ -38,12 +38,10 @@ def assert_match_quoted_microsecond_datetime(match)
       assert_match match, @connection.quoted_date(Time.now.change(sec: 55, usec: 123456))
     end
 
-    def stub_version(full_version_string)
-      @connection.stub(:get_full_version, full_version_string) do
-        @connection.schema_cache.clear!
-        yield
-      end
+    def stub_version(full_version_string, &block)
+      @connection.pool.pool_config.server_version = nil
+      @connection.stub(:get_full_version, full_version_string, &block)
     ensure
-      @connection.schema_cache.clear!
+      @connection.pool.pool_config.server_version = nil
     end
 end

activerecord/test/cases/connection_adapters/schema_cache_test.rb

@@ -6,9 +6,8 @@ module ActiveRecord
   module ConnectionAdapters
     class SchemaCacheTest < ActiveRecord::TestCase
       def setup
-        @connection       = ARUnit2Model.connection
-        @cache            = new_bound_reflection
-        @database_version = @connection.get_database_version
+        @connection = ARUnit2Model.connection
+        @cache = new_bound_reflection
         @check_schema_cache_dump_version_was = SchemaReflection.check_schema_cache_dump_version
       end
 
@@ -67,7 +66,6 @@ def test_yaml_dump_and_load
           assert cache.data_source_exists?("courses")
           assert_equal "id", cache.primary_keys("courses")
           assert_equal 1, cache.indexes("courses").size
-          assert_equal @database_version.to_s, cache.database_version.to_s
         end
       ensure
         tempfile.unlink
@@ -104,7 +102,6 @@ def test_yaml_dump_and_load_with_gzip
           assert cache.data_source_exists?(@connection, "courses")
           assert_equal "id", cache.primary_keys(@connection, "courses")
           assert_equal 1, cache.indexes(@connection, "courses").size
-          assert_equal @database_version.to_s, cache.database_version(@connection).to_s
         end
 
         # Load the cache the usual way.
@@ -116,7 +113,6 @@ def test_yaml_dump_and_load_with_gzip
           assert cache.data_source_exists?("courses")
           assert_equal "id", cache.primary_keys("courses")
           assert_equal 1, cache.indexes("courses").size
-          assert_equal @database_version.to_s, cache.database_version.to_s
         end
       ensure
         tempfile.unlink
@@ -141,18 +137,6 @@ def test_yaml_loads_5_1_dump_without_indexes_still_queries_for_indexes
         end
       end
 
-      def test_yaml_loads_5_1_dump_without_database_version_still_queries_for_database_version
-        cache = load_bound_reflection(schema_dump_path)
-
-        # We can't verify queries get executed because the database version gets
-        # cached in both MySQL and PostgreSQL outside of the schema cache.
-
-        assert_not_nil reflection = @cache.instance_variable_get(:@schema_reflection)
-        assert_nil reflection.instance_variable_get(:@cache)
-
-        assert_equal @database_version.to_s, cache.database_version.to_s
-      end
-
       def test_primary_key_for_existent_table
         assert_equal "id", @cache.primary_keys("courses")
       end
@@ -189,18 +173,6 @@ def test_indexes_for_non_existent_table
         assert_equal [], @cache.indexes("omgponies")
       end
 
-      def test_caches_database_version
-        @cache.database_version # cache database_version
-
-        assert_no_queries do
-          assert_equal @database_version.to_s, @cache.database_version.to_s
-
-          if current_adapter?(:Mysql2Adapter, :TrilogyAdapter)
-            assert_not_nil @cache.database_version.full_version_string
-          end
-        end
-      end
-
       def test_clearing
         @cache.columns("courses")
         @cache.columns_hash("courses")
@@ -211,9 +183,6 @@ def test_clearing
         @cache.clear!
 
         assert_equal 0, @cache.size
-        reflection = @cache.instance_variable_get(:@schema_reflection)
-        schema_cache = reflection.instance_variable_get(:@cache)
-        assert_nil schema_cache.instance_variable_get(:@database_version)
       end
 
       def test_marshal_dump_and_load
@@ -223,10 +192,6 @@ def test_marshal_dump_and_load
         # Populate it.
         cache.add("courses")
 
-        # We're going to manually dump, so we also need to force
-        # database_version to be stored.
-        cache.database_version
-
         # Create a new cache by marshal dumping / loading.
         cache = Marshal.load(Marshal.dump(cache.instance_variable_get(:@schema_reflection).instance_variable_get(:@cache)))
 
@@ -236,7 +201,6 @@ def test_marshal_dump_and_load
           assert cache.data_source_exists?(@connection, "courses")
           assert_equal "id", cache.primary_keys(@connection, "courses")
           assert_equal 1, cache.indexes(@connection, "courses").size
-          assert_equal @database_version.to_s, cache.database_version(@connection).to_s
         end
       end
 
@@ -257,7 +221,6 @@ def test_marshal_dump_and_load_via_disk
           assert cache.data_source_exists?("courses")
           assert_equal "id", cache.primary_keys("courses")
           assert_equal 1, cache.indexes("courses").size
-          assert_equal @database_version.to_s, cache.database_version.to_s
         end
       ensure
         tempfile.unlink
@@ -316,7 +279,6 @@ def test_marshal_dump_and_load_with_gzip
           assert cache.data_source_exists?(@connection, "courses")
           assert_equal "id", cache.primary_keys(@connection, "courses")
           assert_equal 1, cache.indexes(@connection, "courses").size
-          assert_equal @database_version.to_s, cache.database_version(@connection).to_s
         end
 
         # Load a new cache.
@@ -328,7 +290,6 @@ def test_marshal_dump_and_load_with_gzip
           assert cache.data_source_exists?("courses")
           assert_equal "id", cache.primary_keys("courses")
           assert_equal 1, cache.indexes("courses").size
-          assert_equal @database_version.to_s, cache.database_version.to_s
         end
       ensure
         tempfile.unlink
@@ -389,28 +350,28 @@ def test_when_lazily_load_schema_cache_is_set_cache_is_lazily_populated_when_est
           ActiveRecord::Base.establish_connection(new_config)
 
           # cache starts empty
-          assert_equal 0, ActiveRecord::Base.connection.pool.schema_reflection.instance_variable_get(:@cache).size
+          assert_nil ActiveRecord::Base.connection.pool.schema_reflection.instance_variable_get(:@cache)
 
           # now we access the cache, causing it to load
-          assert ActiveRecord::Base.connection.schema_cache.version
+          assert_not_nil ActiveRecord::Base.connection.schema_cache.version
 
           assert File.exist?(tempfile)
-          assert ActiveRecord::Base.connection.pool.schema_reflection.instance_variable_get(:@cache)
+          assert_not_nil ActiveRecord::Base.connection.pool.schema_reflection.instance_variable_get(:@cache)
 
           # assert cache is still empty on new connection (precondition for the
           # following to show it is loading because of the config change)
           ActiveRecord::Base.establish_connection(new_config)
 
           assert File.exist?(tempfile)
-          assert_equal 0, ActiveRecord::Base.connection.pool.schema_reflection.instance_variable_get(:@cache).size
+          assert_nil ActiveRecord::Base.connection.pool.schema_reflection.instance_variable_get(:@cache)
 
           # cache is loaded upon connection when lazily loading is on
           old_config = ActiveRecord.lazily_load_schema_cache
           ActiveRecord.lazily_load_schema_cache = true
           ActiveRecord::Base.establish_connection(new_config)
 
           assert File.exist?(tempfile)
-          assert ActiveRecord::Base.connection.pool.schema_reflection.instance_variable_get(:@cache)
+          assert_not_nil ActiveRecord::Base.connection.pool.schema_reflection.instance_variable_get(:@cache)
         ensure
           ActiveRecord.lazily_load_schema_cache = old_config
           ActiveRecord::Base.establish_connection(:arunit)
@@ -449,7 +410,6 @@ def test_when_lazily_load_schema_cache_is_set_cache_is_lazily_populated_when_est
         assert_equal expected, coder["data_sources"]
         assert_equal expected, coder["indexes"]
         assert coder.key?("version")
-        assert coder.key?("database_version")
       end
 
       private