From de765e37c1d9f422a44317edc0f00e0fe2c335cb Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 24 Jul 2023 15:23:08 +0200 Subject: [PATCH] Never connect to the database in define_attribute_methods initializer Followup: https://github.com/rails/rails/pull/48743 After careful consideration, unless users have a schema cache dump loaded and `check_schema_cache_dump_version = false`, we have no choice but to arbitrate between resiliency and performance. If we define attribute methods during boot, we allow them to be shared between forked workers, and prevent the first requests to be slower. However, by doing so we'd trigger a connection to the datase, which if it's unresponsive could lead to workers not being able to restart triggering a cascading failure. Previously Rails used to be in some sort of middle-ground where it would define attribute methods during boot if for some reason it was already connected to the database at this point. But this is now deemed undesirable, as an application initializer inadvertantly establishing the database connection woudl lead to a readically different behavior. --- activerecord/lib/active_record/railtie.rb | 22 ++++++++--------- .../test/application/configuration_test.rb | 24 +++++++++---------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index 23caf7e4fc082..b75ecfdae77af 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -186,20 +186,18 @@ class Railtie < Rails::Railtie # :nodoc: ActiveSupport.on_load(:active_record) do # In development and test we shouldn't eagerly define attribute methods because # db:test:prepare will trigger later and might change the schema. - if app.config.eager_load && !Rails.env.local? + # + # Additionally if `check_schema_cache_dump_version` is enabled (which is the default), + # loading the schema cache dump trigger a database connection to compare the schema + # versions. + # This means the attribute methods will be lazily defined whent the model is accessed, + # likely as part of the first few requests or jobs. This isn't good for performance + # but we unfortunately have to arbitrate between resiliency and performance, and chose + # resiliency. + if !check_schema_cache_dump_version && app.config.eager_load && !Rails.env.local? begin descendants.each do |model| - # If the schema cache doesn't have the columns for this model, - # we avoid calling `define_attribute_methods` as it would trigger a query. - # - # However if we're already connected to the database, it's too late so we might - # as well eagerly define the attributes and hope the database timeout is strict enough. - # - # Additionally if `check_schema_cache_dump_version` is enabled, we have to connect to the - # database anyway to load the schema cache dump, so we might as well do it during boot to - # save memory in pre-forking setups and avoid slowness during the first requests post deploy. - schema_reflection = model.connection_pool.schema_reflection - if check_schema_cache_dump_version || schema_reflection.cached?(model.table_name) || model.connected? + if model.connection_pool.schema_reflection.cached?(model.table_name) model.define_attribute_methods end end diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index c252145bd7cbf..2aa340c419ca3 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -406,7 +406,7 @@ class Post < ActiveRecord::Base assert_not_includes Post.instance_methods, :title end - test "does not eager load attribute methods in production when the schema cache is empty and the database not connected" do + test "does not eager load attribute methods in production when the schema cache is empty and and check_schema_cache_dump_version=false" do app_file "app/models/post.rb", <<-RUBY class Post < ActiveRecord::Base end @@ -423,7 +423,7 @@ class Post < ActiveRecord::Base assert_not_includes (Post.instance_methods - ActiveRecord::Base.instance_methods), :title end - test "eager loads attribute methods in production when the schema cache is populated" do + test "eager loads attribute methods in production when the schema cache is populated and check_schema_cache_dump_version=false" do app_file "app/models/post.rb", <<-RUBY class Post < ActiveRecord::Base end @@ -442,18 +442,19 @@ class Post < ActiveRecord::Base add_to_config <<-RUBY config.enable_reloading = false config.eager_load = true + config.active_record.check_schema_cache_dump_version = false RUBY app_file "config/initializers/schema_cache.rb", <<-RUBY - ActiveRecord::Base.connection.schema_cache.add("posts") + ActiveRecord::Base.connection.schema_cache.add("posts") RUBY app "production" - assert_includes Post.instance_methods, :title + assert_includes (Post.instance_methods - ActiveRecord::Base.instance_methods), :title end - test "does not attempt to eager load attribute methods for models that aren't connected" do + test "does not eager loads attribute methods in production when the schema cache is populated and check_schema_cache_dump_version=true" do app_file "app/models/post.rb", <<-RUBY class Post < ActiveRecord::Base end @@ -472,17 +473,16 @@ class Post < ActiveRecord::Base add_to_config <<-RUBY config.enable_reloading = false config.eager_load = true + config.active_record.check_schema_cache_dump_version = true RUBY - app_file "app/models/comment.rb", <<-RUBY - class Comment < ActiveRecord::Base - establish_connection(adapter: "mysql2", database: "does_not_exist") - end + app_file "config/initializers/schema_cache.rb", <<-RUBY + ActiveRecord::Base.connection.schema_cache.add("posts") RUBY - assert_nothing_raised do - app "production" - end + app "production" + + assert_not_includes (Post.instance_methods - ActiveRecord::Base.instance_methods), :title end test "application is always added to eager_load namespaces" do