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

Fix Relation#exists? queries with query cache #29454

Merged
merged 2 commits into from Jun 19, 2017

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Jun 15, 2017

If a connection adapter overrides select_* methods, query caching will
doesn't work. This patch changes select_value to select_one in
Relation#exists? to ensure query caching.

Fixes #29449.

If a connection adapter overrides `select_*` methods, query caching will
doesn't work. This patch changes `select_value` to `select_one` in
`Relation#exists?` to ensure query caching.

Fixes rails#29449.
@matthewd
Copy link
Member

I wonder whether other callers are expecting select_value to bypass the query cache.

@vipulnsward
Copy link
Member

👍 for the second change. Does it not have an overhead?

@kamipo
Copy link
Member Author

kamipo commented Jun 15, 2017

select_{value,values,rows} in mysql2 and postgresql adapters are for bypassing ActiveRecord::Result instanciation.
These are mostly used in schema statements (select_all in Relation is almost all, select_one and select_value are once) and it is not a performance hotspot in AR internal (actually select_one in mysql2 adapter was removed in #25507).
I think that removing the bypassing is acceptable.

@tjschuck
Copy link
Contributor

It seems like this can be cleanly and safely backported to 5-0-stable and 5-1-stable as well with no change in behavior, so I'd like to preemptively request that here!

@zmoazeni
Copy link

Thank you @kamipo for jumping on this. I for one really appreciate all the hard work all of you put into these performance updates. I've tried following along on several of the PRs (like @matthewd's awesome connection pool updates), but it is definitely a different league.

One thing I was going to check was to see if other typical Rails queries are relying on select_one but it looks like you have tests for those cases, and that's awesome.

As for:

I wonder whether other callers are expecting select_value to bypass the query cache.

People can currently opt out of the query cache right now yeah?

> Project.cache { Project.uncached { 3.times { Project.find(1) } }}
  Project Load (0.3ms)  SELECT  `projects`.* FROM `projects` WHERE `projects`.`id` = 1 LIMIT 1
  Project Load (0.3ms)  SELECT  `projects`.* FROM `projects` WHERE `projects`.`id` = 1 LIMIT 1
  Project Load (0.2ms)  SELECT  `projects`.* FROM `projects` WHERE `projects`.`id` = 1 LIMIT 1

My vote would be that the programmer should be explicit by wrapping their queries in uncached { ... } blocks if they want to avoid the cache.

There could also exist other scenarios that I'm not aware of, but that feels appropriate with the bits that I do.

@rafaelfranca rafaelfranca merged commit 91450fa into rails:master Jun 19, 2017
rafaelfranca added a commit that referenced this pull request Jun 19, 2017
Fix `Relation#exists?` queries with query cache
rafaelfranca added a commit that referenced this pull request Jun 19, 2017
Fix `Relation#exists?` queries with query cache
rafaelfranca added a commit that referenced this pull request Jun 19, 2017
@kamipo kamipo deleted the fix_exists_queries_with_cache branch June 20, 2017 03:58
kamipo added a commit to kamipo/rails that referenced this pull request Jun 30, 2017
`test_middleware_caches` is sometimes failed since rails#29454.
The failure is due to schema statements are affected by query caching.
Bypassing query caching for schema statements to avoid the issue.
kamipo added a commit to kamipo/rails that referenced this pull request Jul 20, 2017
`test_middleware_caches` is sometimes failed since rails#29454.
The failure is due to schema statements are affected by query caching.
Bypassing query caching for schema statements to avoid the issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants