Move schema cache to pool #36371

eileencodes · 2019-05-31T19:25:16Z

This PR proposes moving the schema cache from the connection to the pool
so the connection can ask the pool for the cache. The end goal, if this
PR is acceptable, is to have the pool read the cache from the yaml file
so that we can get rid of the active_record.check_schema_cache_dump
initializer.

Why do we want to get rid of the initializer you ask?

Well I was looking at #34449 and trying to make it work for our usecase
and it revealed A LOT of problems. There are a few issues that I will
fix in remaining PRs with SchemaMigration, but there's a big glaring
issue with this initializer.

When you have an application with multiple databases we'll need to loop
through all the configurations and set the schema cache on those
connections. The problem is on initialization we only have one
connection - the one for Ar::Base. This is fine in a single db
application but not fine in multi-db. If we follow the pattern in #34449
and establish a connection to those other dbs we will end up setting the
cache on the connection object rather than on all connections that
connect for that config.

So even though we looped through the configs and assigned the cache the
cache will not be set (or will be set wrong) once the app is booted.
After trying many different ways to set the schema cache @tenderlove
and I came to the conclusion that the initializer is problematic, as is
setting the schema cache twice.

This is part 1 to move the cache to the pool so the cache can read from
the schema cache yaml file instead of setting it when initializing the
app.

cc/ @jhawthorn @rafaelfranca @matthewd - @tenderlove and I would like your thoughts on this before proceeding and cleaning it up. The schema cache is kind of busted in Rails 6 for multi-db 😢

tenderlove · 2019-05-31T19:29:40Z

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -106,6 +106,20 @@ def self.build_read_query_regexp(*parts) # :nodoc:
        Regexp.union(*parts)
      end

+      class NullPool


Suggested change

class NullPool

class NullPool # :nodoc:

tenderlove · 2019-05-31T19:30:49Z

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

      def schema_cache=(cache)
        cache.connection = self
-        @schema_cache = cache
+        @pool.set_schema_cache(cache)


We should find places calling schema_cache= on the connection and remove them. Since the connection just asks the pool for the schema cache, there's no reason to set it on the connection AFAIK.

tenderlove · 2019-05-31T19:32:26Z

activerecord/test/cases/connection_pool_test.rb

@@ -507,7 +507,7 @@ def test_pool_sets_connection_schema_cache
        pool.schema_cache = schema_cache

        pool.with_connection do |conn|
-          assert_not_same pool.schema_cache, conn.schema_cache
+          assert_same pool.schema_cache, conn.schema_cache
          assert_equal pool.schema_cache.size, conn.schema_cache.size
          assert_same pool.schema_cache.columns(:posts), conn.schema_cache.columns(:posts)


I think we can remove these two assertions. The assert_same we added above should be sufficient.

This PR proposes moving the schema cache from the connection to the pool so the connection can ask the pool for the cache. In a future PR our goal is to be able to read the yaml file from the pool so we can get rid of the `active_record.check_schema_cache_dump` initializer. This will fix the issues surrounding dumping the schema cache and mulitple databases. Why do we want to get rid of the initializer you ask? Well I was looking at rails#34449 and trying to make it work for our usecase and it revealed A LOT of problems. There are a few issues that I will fix in remaining PRs with SchemaMigration, but there's a big glaring issue with this initializer. When you have an application with multiple databases we'll need to loop through all the configurations and set the schema cache on those connections. The problem is on initialization we only have one connection - the one for Ar::Base. This is fine in a single db application but not fine in multi-db. If we follow the pattern in rails#34449 and establish a connection to those other dbs we will end up setting the cache on the _connection object_ rather than on all connections that connect for that config. So even though we looped through the configs and assigned the cache the cache will not be set (or will be set wrong) once the app is booted because the connection objects after boot are _different_ than the connection objects we assigned the cache to. After trying many different ways to set the schema cache `@tenderlove` and I came to the conclusion that the initializer is problematic, as is setting the schema cache twice. This is part 1 to move the cache to the pool so the cache can read from the schema cache yaml file instead of setting it when initializing the app. To do this we have created a `NullPool` that initializes an empty cache. I put the `get_schema_cache` and `set_schema_cache` in an `AbstractPool` so we can share code between `ConnectionPool` and `NullPool` instead of duplicating code. Now we only need to set the schema_cache on the pool rather than the connection. In `discard!` we need to unset the connection from the schema_cache - we still want the cache just not the connection.

Move schema cache to pool

Follow up rails/rails#36371.

ActiveRecord now returns a NullPool when there is no pool since rails/rails#36371 landed in v6.1.0. Both `nil` and `NullPool` do not respond to `spec`, so this should make 6.1+ work in a backwards compatible way.

ActiveRecord connection pooling was re-jigged in rails/rails#36371 and landed in v6.1.0. Connection pools quack a little differently. They might be a NullPool instead of a nil. And they have a #db_config, not a \#spec.

ActiveRecord connection pooling was re-jigged in rails/rails#36371 and landed in v6.1.0. Connection pools quack a little differently. They might be a NullPool instead of a nil. And they have a #db_config, not a #spec.

eileencodes added this to the 6.0.0 milestone May 31, 2019

rails-bot bot added the activerecord label May 31, 2019

tenderlove reviewed May 31, 2019

View reviewed changes

eileencodes force-pushed the move-schema-cache-to-pool branch from cd74a3b to d007590 Compare June 5, 2019 15:29

eileencodes changed the title ~~WIP: Move schema cache to pool~~ Move schema cache to pool Jun 5, 2019

eileencodes force-pushed the move-schema-cache-to-pool branch from d007590 to 097403e Compare June 5, 2019 20:09

eileencodes force-pushed the move-schema-cache-to-pool branch from 097403e to 49b6b21 Compare June 5, 2019 20:37

eileencodes merged commit c4d8e05 into rails:master Jun 5, 2019

eileencodes deleted the move-schema-cache-to-pool branch June 5, 2019 21:02

eileencodes added a commit that referenced this pull request Jun 5, 2019

Merge pull request #36371 from eileencodes/move-schema-cache-to-pool

0e1625b

Move schema cache to pool

koic added a commit to koic/oracle-enhanced that referenced this pull request Jun 6, 2019

Move schema cache to pool

a7d6e24

Follow up rails/rails#36371.

koic mentioned this pull request Jun 6, 2019

Move schema cache to pool rsim/oracle-enhanced#1888

Merged

eileencodes mentioned this pull request Jun 14, 2019

Update multi-db docs #36488

Merged

zarqman mentioned this pull request Mar 5, 2023

Make SchemaCache#connection thread-safe #47582

Closed

4 tasks

denis-haskin-elemental mentioned this pull request May 7, 2024

Fix for rails 6.1 basecamp/marginalia#91

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move schema cache to pool #36371

Move schema cache to pool #36371

eileencodes commented May 31, 2019

tenderlove May 31, 2019

tenderlove May 31, 2019

tenderlove May 31, 2019

Move schema cache to pool #36371

Move schema cache to pool #36371

Conversation

eileencodes commented May 31, 2019

tenderlove May 31, 2019

Choose a reason for hiding this comment

tenderlove May 31, 2019

Choose a reason for hiding this comment

tenderlove May 31, 2019

Choose a reason for hiding this comment