Skip to content
  • eileencodes's avatar
    adb64db4
    Only remove connection for an existing pool if the config is different · adb64db4
    eileencodes authored
    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: #41855
    Fixes: #41876
    Fixes: #42873
    Fixes: #43004
    adb64db4
    Only remove connection for an existing pool if the config is different
    eileencodes authored
    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: #41855
    Fixes: #41876
    Fixes: #42873
    Fixes: #43004
Loading