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

Only define attribute methods from schema cache #33985

Merged
merged 1 commit into from Jan 3, 2019

Conversation

@eugeneius
Copy link
Member

@eugeneius eugeneius commented Sep 26, 2018

Followup to #33959.

To define the attribute methods for a model, Active Record needs to know the schema of the underlying table, which is usually achieved by making a request to the database. This is undesirable behaviour while the app is booting, for two reasons: it makes the boot process dependent on the availability of the database, and it means every new process will make one query for each table, which can cause issues for large applications.

However, if the application is using the schema cache dump feature, then the schema cache already contains the necessary information, and we can define the attribute methods without causing any extra database queries.

r? @rafaelfranca

descendants.each do |model|
# These internal models override `table_exists?` to bypass the schema cache,
# so don't call `define_attribute_methods` on them to avoid the extra query.
next if model == ActiveRecord::SchemaMigration || model == ActiveRecord::InternalMetadata

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 28, 2018
Member

Don't know if it is worth, but we could add a internal? or _internal? if we don't want to pollute the ActiveRecord::SchemaMigration that returns false by default but return true for those two models.

This comment has been minimized.

@eugeneius

eugeneius Sep 28, 2018
Author Member

I just noticed that these models aren't eager loaded, so I do think it's worthwhile to avoid mentioning them by name. I'll add an _internal? method.

# so don't call `define_attribute_methods` on them to avoid the extra query.
next if model == ActiveRecord::SchemaMigration || model == ActiveRecord::InternalMetadata

if model.connected? && model.connection.schema_cache.columns_hash?(model.table_name)

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 28, 2018
Member

Do we need to check for connected? here? I think we can use the connection_pool if the connection method connects to database.

This comment has been minimized.

@eugeneius

eugeneius Sep 28, 2018
Author Member

We could indeed call columns_hash? on the connection pool's schema cache instead, but then later this line would check out a connection, and we'd be back to running queries during boot.

To define the attribute methods for a model, Active Record needs to know
the schema of the underlying table, which is usually achieved by making
a request to the database. This is undesirable behaviour while the app
is booting, for two reasons: it makes the boot process dependent on the
availability of the database, and it means every new process will make
one query for each table, which can cause issues for large applications.

However, if the application is using the schema cache dump feature, then
the schema cache already contains the necessary information, and we can
define the attribute methods without causing any extra database queries.
@eugeneius eugeneius force-pushed the eugeneius:attribute_methods_schema_cache branch from 42bb4e3 to 0007501 Sep 28, 2018
@rails-bot rails-bot added the railties label Sep 28, 2018
@oniofchaos
Copy link
Contributor

@oniofchaos oniofchaos commented Oct 13, 2018

👍

@eugeneius
Copy link
Member Author

@eugeneius eugeneius commented Jan 2, 2019

@rafaelfranca would you mind taking another look here?

@kaspth kaspth merged commit 6f0cda8 into rails:master Jan 3, 2019
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth
Copy link
Member

@kaspth kaspth commented Jan 3, 2019

LGTM! Perhaps we should start promoting the schema cache more as a deploy step akin to assets:precompile?

@eugeneius
Copy link
Member Author

@eugeneius eugeneius commented Jan 15, 2019

Yeah, @matthewd had the same suggestion: #33959 (comment)

Maybe the Heroku Ruby buildpack should generate a schema cache by default? (👋 @schneems 😊)

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

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