Previously Rails would always remove the connection if it found a matching class in the pool manager. Therefore if `ActiveRecord::Base.establish_connection` was called with the same config, each time it was called it would be clobbered, even though the config hasn't changed and the existing connection is prefectly fine. As far as I can tell from conversations and reading the history this functionality was added for ActiveRecord tests to be able to clobber the connection and use a new config, then re-establish the old connection. Essentially outside Rake tasks and AR tests, this functionality doesn't have a ton of value. On top of not adding a ton of value, this has resulted in a few bugs. In Rails 6.0 I made it so that if you established a connection on `ApplicationRecord` Rails would treat that connection the same as `ActiveRecord::Base.` The reason for this is that the Railtie establishes a connection on boot to the first database, but then if you're using multiple databases you're calling `connects_to` in your `ApplicationRecord` or primary abstract class which essentially doubles your connections to the same database. To avoid opening 2 connections to the same database, Rails treats them the same. However, because we have this code that removes existing connections, when an application boots, `ApplicationRecord` will clobber the connection that the Railtie established even though the connection configs are the same. This removal of the connection caused bugs in migrations that load up a model connected to `ApplicationRecord` (ex `Post.first`) and then calls `execute("SELECT 1")` (obviously a simplified example). When `execute` runs the connection is different from the one opened to run the migration and essentially it is lost when the `remove_connection` code is called. To fix this I've updated the code to only remove the connection if the database config is different. Ultimately I'd like to remove this code altogether but to do that we first need to stop using `Base.establish_connection` in the rake tasks and tests. This will fix the major bugs until I come up with a solution for the areas that currently need to call `establish_connection` on Base. The added benefit of this change is that if your app is calling `establish_connection` multiple times with the same config, it is now 3x faster than the previous implementation because we can return the found pool instead of setting it up again. To benchmark this I duplicated the `establish_connection` method to use the new behavior with a new name. Benchmark script: ```ruby require "active_record" require "logger" require "benchmark/ips" config_hash = { "development" => { "primary" => { "adapter" => "mysql2", "username" => "rails", "database" => "activerecord_unittest"}}} ActiveRecord::Base.configurations = config_hash db_config = ActiveRecord::Base.configurations.configs_for(env_name: "development", name: "primary") p "Same model same config" ActiveRecord::Base.connected_to(role: :writing, prevent_writes: true) do Benchmark.ips do |x| x.report "establish_connection with remove" do ActiveRecord::Base.establish_connection(db_config) end x.report "establish_connection without remove" do ActiveRecord::Base.establish_connection_no_remove(db_config) end x.compare! end end ``` Benchmark results: ``` Warming up -------------------------------------- establish_connection with remove 4.677k i/100ms establish_connection without remove 19.501k i/100ms Calculating ------------------------------------- establish_connection with remove 41.252k (±11.3%) i/s - 205.788k in 5.075525s establish_connection without remove 179.205k (± 6.9%) i/s - 897.046k in 5.029742s Comparison: establish_connection without remove: 179205.1 i/s establish_connection with remove: 41252.3 i/s - 4.34x (± 0.00) slower ``` Other changes: 1) sqlite3 now disconnects and reconnects the connection when `purge` is called. This is necessary now that a new connection isn't created everyt time `establish_connection` is called. Without this change to purge the new database is left in an inaccessible state causing a readonly error from the sqlite3 client. This wasn't happening in mysql or postgres because they were already reconnecting the db connection. 2) I added `remove_connection` to tests that use `ApplicationRecord`. This is required because `ApplicationRecord` or any class that is a `primary_abstract_class` will be treated the same as `ActiveRecord::Base`. This is fine in applications because they are shared connections, but in the AR test environment, we don't want those connnections to stick around (we want AR::Base back). 3) In the async tests I removed 2 calls to `establish_connection`. These were causing sqlite3 tests to leak the state of async_executor because it's stored on the connection. I'm not sure why these were calling `establish_connection` but it's not necessary and was leaking state when now that we are no longer removing the connection. Fixes: rails#41855 Fixes: rails#41876 Fixes: rails#42873 Fixes: rails#43004

Uh oh!

Horizontal sharding & migration with data changing script (symlink) #43004

Description

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions