Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a fallback database config when loading schema cache #38383

Merged
merged 2 commits into from Feb 5, 2020

Conversation

@kytrinyx
Copy link
Contributor

kytrinyx commented Feb 4, 2020

Summary

The schema cache defaults to loading the 'primary' database config.
However, if an app doesn't have a db config with a spec name of
'primary' the filename lookup will blow up.

This adds a fallback for this case.

Other Information

The filename for the schema cache used to be hard-coded, but Rails allows people to override the default path. In #38348 I made a change to take overrides into account, but failed to account for apps without a database config containing a spec named primary.

It turns out, at GitHub we don't have a database named primary, so even though we don't rely on this active record initializer, it blows up for us.

The schema cache defaults to loading the 'primary' database config.
However, if an app doesn't have a db config with a spec name of
'primary' the filename lookup will blow up.

This adds a fallback for this case.
@rails-bot rails-bot bot added the activerecord label Feb 4, 2020
@tenderlove

This comment has been minimized.

Copy link
Member

tenderlove commented Feb 4, 2020

I thought the database configuration loading code guaranteed that one of the db configs would have a spec name of "primary". Is that not the case? Do you have a test app that can reproduce the issue?

@kytrinyx

This comment has been minimized.

Copy link
Contributor Author

kytrinyx commented Feb 4, 2020

Do you have a test app that can reproduce the issue?

Not yet; let me make one

@kytrinyx

This comment has been minimized.

Copy link
Contributor Author

kytrinyx commented Feb 4, 2020

I created a test app: https://github.com/kytrinyx/rails-test-app-pumpkin

The database config uses the three tier config style that we use at GitHub:
https://github.com/kytrinyx/rails-test-app-pumpkin/blob/master/config/database.yml

In the Rails console I did this:

ActiveRecord::Base.configurations.configs_for(env_name: Rails.env, spec_name: "primary")
=> nil
@tenderlove

This comment has been minimized.

Copy link
Member

tenderlove commented Feb 4, 2020

Got it.

Instead of loading the cache just for the "primary" connection, would it make sense to loop through each configuration, looking for schema caches, then loading those caches? Then we don't depend on a "primary" connection.

@kytrinyx

This comment has been minimized.

Copy link
Contributor Author

kytrinyx commented Feb 4, 2020

Instead of loading the cache just for the "primary" connection, would it make sense to loop through each configuration, looking for schema caches, then loading those caches? Then we don't depend on a "primary" connection.

I think that's what this PR does, which didn't get merged: #34449

I know the multi db stuff is broken, but the change in that PR might actually be "more correct" than the current state.

Thoughts?

@tenderlove

This comment has been minimized.

Copy link
Member

tenderlove commented Feb 4, 2020

@kytrinyx ya, that PR does look like what we should be doing. Can you test it out, and try fixing if it's broken?

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Feb 4, 2020

@casperisfine do you have opinion about this one? I think we have this problem solved in our codebase.

@kytrinyx kytrinyx force-pushed the kytrinyx:schema-spec-fallback-config branch 3 times, most recently from 9e54fb4 to 464d997 Feb 4, 2020
@kytrinyx

This comment has been minimized.

Copy link
Contributor Author

kytrinyx commented Feb 4, 2020

I tested looping through all the configs and a bunch of tests broke, so I backed that out.

@casperisfine

This comment has been minimized.

Copy link

casperisfine commented Feb 5, 2020

@casperisfine do you have opinion about this one? I think we have this problem solved in our codebase.

We simply don't use that code at all, and added support for multidb schema cache. Meaning each connection go grab it's schema cache in a different path (it's more complicated because many connection have similar schemas, so they share their cache as well, but it doesn't matter here).

As for Rails users in general, my worry about this PR is that by falling back to the first connection, we might load a cache that doesn't match the connection. But note that I haven't double checked wether it's actually possible or not.

IMHO It would be preferable to actually implement multi-schema cache support, e.g. one cache dump per spec_name.

IIRC I tried to implement that a few months back, but it was very hard. It might be easier now that some refactorings have been done.

@kytrinyx

This comment has been minimized.

Copy link
Contributor Author

kytrinyx commented Feb 5, 2020

As for Rails users in general, my worry about this PR is that by falling back to the first connection, we might load a cache that doesn't match the connection.

Agreed. I pushed up a change that will simply not load a cache if we don't find a primary connection, which I think is safer.

I will start working on a new PR to see if I can work out how to load all the specs for all the connections. Last time I tried that a bunch of tests broke, so that is probably not going to be a quick fix.

@tenderlove tenderlove merged commit 25c3807 into rails:master Feb 5, 2020
2 checks passed
2 checks passed
build
Details
buildkite/rails Build #66836 passed (19 minutes, 10 seconds)
Details
@kytrinyx kytrinyx deleted the kytrinyx:schema-spec-fallback-config branch Feb 5, 2020
kytrinyx added a commit to kytrinyx/rails that referenced this pull request Feb 11, 2020
Hopefully third time's a charm.

Originally, the schema cache initializer railtie was loading
the 'schema_cache.yml' file by hard-coding the path.

Since it's possible to override the schema cache path either by
providing an ENV variable, or by adding a configuration entry to
the database.yml config file, this means that we could potentially
end up not loading a cache that is configured to use a non-default
file.

The first attempt at fixing this (rails#38348) assumed that the
db config must contain a spec named 'primary'. This is not the case.
A second attempt at fixing this (rails#38383) bailed if no db config
was found.

This however, means that for an app without a db config named
'primary', we would not be attempting to load a schema cache
at all, even if they were using the default schema cache path.

Instead of bailing, here we go get the filename, passing a 'nil'
for the schema_cache_path, which will fall back to the default
path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.