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

Configure query caching (per thread) on the connection pool #26978

Merged
merged 2 commits into from Nov 10, 2016

Conversation

Projects
None yet
4 participants
@matthewd
Member

matthewd commented Nov 6, 2016

Fixes #25573

cc @sgrif

On reflection, I believe this makes #26909 backportable (which isn't too surprising: it almost reverses that PR's changes to the querycache middleware).

@eugeneius

This comment has been minimized.

Member

eugeneius commented Nov 8, 2016

This is awesome 😍

Viewing the diff combined with #26909 makes the overall intent pretty clear: 2b2c096...3c785bd

My only concern with backporting this is that it moves the responsibility for checking out the thread-cached connection from the executor to the point where it's first used. Right now I can be sure that my application code will never block or raise trying to get a connection from the pool, but with this change that might start to happen.

Would it be insane to backport this but add a reference to ActiveRecord::Base.connection to the executor run hook, to preserve the previous behaviour on 5-0-stable?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Nov 8, 2016

My only concern with backporting this is that it moves the responsibility for checking out the thread-cached connection from the executor to the point where it's first used. Right now I can be sure that my application code will never block or raise trying to get a connection from the pool, but with this change that might start to happen.

This is exactly what we want to archive. The application should lazily connect to the database to be resilient to database failures.

@eugeneius

This comment has been minimized.

Member

eugeneius commented Nov 8, 2016

Fair enough - I wasn't sure if that was intentional or a side-effect, since there are other benefits to toggling the query cache on checkin/checkout. But if the whole point is to make the connection lazily loaded, then obviously what I suggested makes no sense 😬

@matthewd

This comment has been minimized.

Member

matthewd commented Nov 8, 2016

Yeah, it's a goal particularly because we're encouraging more places to use the executor (sidekiq, Active Job in general, Action Cable channels, thread-using user code in general)... but I think it's really surprising-bordering-on-bug that previous versions have eagerly allocated connections.

Anyone wanting to preserve the previous behaviour could call connection in a before_action (to get the 4.2-era "controllers only" version), or just add it as a to_run block.

@sgrif

sgrif approved these changes Nov 8, 2016

Makes sense to me. 👍

@matthewd matthewd merged commit cac3be6 into rails:master Nov 10, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

matthewd added a commit that referenced this pull request Nov 10, 2016

Merge pull request #26978 from matthewd/query-cache-pool
Configure query caching (per thread) on the connection pool
@matthewd

This comment has been minimized.

Member

matthewd commented Nov 10, 2016

Backported in 2b5d139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment