TEST_CODEOWNERS/rails
3
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Move the database version cache from schema cache to pool config

Browse Source 
Ref: https://github.com/rails/rails/pull/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.
This commit is contained in:
Jean Boussier 2023-09-28 13:26:36 +02:00
parent 6c9967f85a
commit a1c0173ee3
9 changed files with 39 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
View File

@ -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,

2
activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
View File

@ -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:

2
activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
View File

@ -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

9
activerecord/lib/active_record/connection_adapters/pool_config.rb
View File

@ -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"

20
activerecord/lib/active_record/connection_adapters/schema_cache.rb
View File

@ -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

2
activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb
View File

@ -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

7
activerecord/lib/active_record/railtie.rb
View File

@ -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

10
activerecord/test/cases/adapters/abstract_mysql_adapter/datetime_precision_quoting_test.rb
View File

@ -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

54
activerecord/test/cases/connection_adapters/schema_cache_test.rb
View File

@ -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,20 +350,20 @@ 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
@ -410,7 +371,7 @@ def test_when_lazily_load_schema_cache_is_set_cache_is_lazily_populated_when_est
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