Delay Adapter#check_version to #configure_connection #49378

casperisfine · 2023-09-25T13:47:30Z

Ideally we should be able to checkout an Adapter instance without triggering a connection to the server.

For some adapters like Trilogy that is the case today, but only if you have a schema cache loaded, otherwise check_version will trigger a query.

I think this check can be delayed to when we actually use the connection.

casperisfine · 2023-09-25T16:24:54Z

There's some intricate build failures to investigate. I'll have another look tomorrow.

composerinteralia

👍🏻 Making this change was on my list of things to do. GitHub has a lingering Trilogy adapter patch that does basically the same thing, and it'll be nice for us to get that cleaned up.

casperisfine · 2023-09-27T11:09:50Z

I only have one failure left in Action Cable, but I'm having trouble reproducing it locally :/

casperisfine · 2023-09-27T15:14:47Z

I haven't had the time to figure out the Action Cable issue today, but I don't expect it to need much more change.

@matthewd any opinions here?

casperisfine · 2023-09-28T08:28:47Z

I only have one failure left in Action Cable, but I'm having trouble reproducing it locally :/

Ok, so actually it seems they are currently broken on main. All the PostgresqlAdapter tests are skipped because we fail to connect to postgresql.

The PG server is started but I think we fail to create the database or something.

actioncable/lib/action_cable/subscription_adapter/postgresql.rb

matthewd · 2023-09-28T08:16:08Z

activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb

@@ -233,7 +233,7 @@ def with_connection

      # Returns true if a connection has already been opened.
      def connected?
-        synchronize { @connections.any? }
+        synchronize { @connections.any?(&:connected?) }


Gently hesitant here on whether callers are most likely to expect this to mean "are there physical sockets established" or "does this pool have conns in it"

Well, the doc says "connection has been opened".

before my change this was true as soon as there one one adapter instance in the pool, now that the connection is properly lazy, it's no longer true. I had to change this to fix a good number of tests.

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

matthewd · 2023-09-28T08:29:19Z

activerecord/lib/active_record/railtie.rb

                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!


This sounds reasonable but only semi-related... assuming it is separate (the instigating change?), might be better split out, it only because of how confusing it is that this is also about a different version check? 😅

Moved to #49415

casperisfine · 2023-09-28T12:57:58Z

I rebased on top of #49415, this solves the circular issue in PostgresAdapter.

Ref: rails#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.

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

Ideally we should be able to checkout an Adapter instance without triggering a connection to the server. For some adapters like Trilogy that is the case today, but only if you have a schema cache loaded, otherwise `check_version` will trigger a query. I think this check can be delayed to when we actually use the connection.

Close #236 Ref: rails/rails#49378

rails-bot bot added the activerecord label Sep 25, 2023

rafaelfranca approved these changes Sep 25, 2023

View reviewed changes

casperisfine marked this pull request as draft September 25, 2023 16:24

composerinteralia reviewed Sep 26, 2023

View reviewed changes

casperisfine force-pushed the ar-delay-version-check branch 5 times, most recently from 3e566f6 to 50e337f Compare September 27, 2023 10:56

rails-bot bot added the railties label Sep 27, 2023

casperisfine force-pushed the ar-delay-version-check branch 2 times, most recently from 90b60f8 to 55f5f4e Compare September 28, 2023 07:50

rails-bot bot added the actioncable label Sep 28, 2023

casperisfine force-pushed the ar-delay-version-check branch from 55f5f4e to f538068 Compare September 28, 2023 08:27

matthewd reviewed Sep 28, 2023

View reviewed changes

casperisfine force-pushed the ar-delay-version-check branch 2 times, most recently from 979373d to e03f24f Compare September 28, 2023 09:14

casperisfine marked this pull request as ready for review September 28, 2023 09:18

casperisfine force-pushed the ar-delay-version-check branch 2 times, most recently from e9fee38 to 2bca6fe Compare September 28, 2023 09:41

casperisfine mentioned this pull request Sep 28, 2023

Move the database version cache from schema cache to pool config #49415

Merged

casperisfine force-pushed the ar-delay-version-check branch from 2bca6fe to 1479702 Compare September 28, 2023 12:14

casperisfine force-pushed the ar-delay-version-check branch from 1479702 to e1612a1 Compare September 28, 2023 12:21

casperisfine force-pushed the ar-delay-version-check branch from e1612a1 to 0a688a7 Compare September 28, 2023 12:55

casperisfine force-pushed the ar-delay-version-check branch from 0a688a7 to 90e5d1c Compare September 28, 2023 13:04

casperisfine force-pushed the ar-delay-version-check branch 2 times, most recently from c3c3854 to 8f2f25d Compare September 28, 2023 15:17

matthewd reviewed Sep 28, 2023

View reviewed changes

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb Outdated Show resolved Hide resolved

casperisfine requested a review from matthewd September 28, 2023 17:22

casperisfine force-pushed the ar-delay-version-check branch from 8f2f25d to b67a9c7 Compare September 28, 2023 17:22

casperisfine force-pushed the ar-delay-version-check branch from b67a9c7 to 2da1852 Compare September 29, 2023 06:53

byroot merged commit 0d0e6bd into rails:main Sep 29, 2023
4 checks passed

yahonda mentioned this pull request Sep 30, 2023

PersistenceTest#test_persisted_returns_boolean fails at 7-1-stable branch #49439

Closed

lorint mentioned this pull request Oct 2, 2023

Incorporate Rails PR #49378 trilogy-libraries/activerecord-trilogy-adapter#66

Merged

tagliala mentioned this pull request Oct 5, 2023

Does not work with Rails edge ifad/chronomodel#236

Closed

tagliala added a commit to ifad/chronomodel that referenced this pull request Oct 5, 2023

Connect to the database before checking PG version

be36bec

Close #236 Ref: rails/rails#49378

tagliala mentioned this pull request Oct 5, 2023

Connect to the database before checking PG version ifad/chronomodel#237

Merged

tagliala added a commit to ifad/chronomodel that referenced this pull request Oct 5, 2023

Connect to the database before checking PG version

319a8f8

Close #236 Ref: rails/rails#49378

tagliala added a commit to ifad/chronomodel that referenced this pull request Oct 5, 2023

Connect to the database before checking PG version

6752d79

Close #236 Ref: rails/rails#49378

Earlopain mentioned this pull request Oct 17, 2023

ConnectionNotEstablished when accessing column data with check_schema_cache_dump_version = false on postgres #49672

Open

fatkodima mentioned this pull request Nov 13, 2023

db:create:all task fails for new app with mysql adapter #50038

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay Adapter#check_version to #configure_connection #49378

Delay Adapter#check_version to #configure_connection #49378

casperisfine commented Sep 25, 2023

casperisfine commented Sep 25, 2023

composerinteralia left a comment •

edited

casperisfine commented Sep 27, 2023

casperisfine commented Sep 27, 2023

casperisfine commented Sep 28, 2023

matthewd Sep 28, 2023

casperisfine Sep 28, 2023

matthewd Sep 28, 2023

casperisfine Sep 28, 2023

casperisfine commented Sep 28, 2023

		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!

Delay Adapter#check_version to #configure_connection #49378

Delay Adapter#check_version to #configure_connection #49378

Conversation

casperisfine commented Sep 25, 2023

casperisfine commented Sep 25, 2023

composerinteralia left a comment • edited

Choose a reason for hiding this comment

casperisfine commented Sep 27, 2023

casperisfine commented Sep 27, 2023

casperisfine commented Sep 28, 2023

matthewd Sep 28, 2023

Choose a reason for hiding this comment

casperisfine Sep 28, 2023

Choose a reason for hiding this comment

matthewd Sep 28, 2023

Choose a reason for hiding this comment

casperisfine Sep 28, 2023

Choose a reason for hiding this comment

casperisfine commented Sep 28, 2023

composerinteralia left a comment •

edited