Move the database version cache from schema cache to pool config

Ref: rails#49378

As discussed with Matthew Draper, we have a bit of a chicken and egg
problem with the schema cache and the database version.

The database version is stored in the cache to avoid a query,
but the schema cache need to query the schema version in the database
to be revalidated.

So `check_version` depends on `schema_cache`, which depends on
`Migrator.current_version`, which depends on `configure_connection`
which depends on `check_version`.

But ultimately, we think storing the server version in the cache
is incorrect, because upgrading a DB server is orthogonal from
regenerating the schema cache.

So not persisting the version in cache is better. Instead we store
it in the pool config, so that we only check it once per process
and per database.

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