Opened 10 years ago

Closed 10 years ago

#1261 closed defect (fixed)

Rails ActiveRecord Query Cache should be disabled for API requests

Reported by: Grant Slater Owned by: Tom Hughes
Priority: minor Milestone: OSM 0.5
Component: api Version:
Keywords: ActiveRecord QueryCache Cc: shaun@…

Description

Since Rails 2.0 all ActiveRecord? queries are cached.

Caching is unneeded, slows down API and uses more memory. I suspect it negatively affects garbage collection.

Attached is a patch which disables AR Query Cache globally. ~15% API speed improvement on my test setup. Ideal would be to only disable for API requests.

config/initializers/query_cached_off.rb

Attachments (1)

query_cached_off.rb (150 bytes) - added by Grant Slater 10 years ago.
Disable Rails AR Query Cache

Download all attachments as: .zip

Change History (7)

Changed 10 years ago by Grant Slater

Attachment: query_cached_off.rb added

Disable Rails AR Query Cache

comment:1 Changed 10 years ago by Tom Hughes

What is the basis of your statement that caching is unneeded? Presumably the rails developers didn't add it for fun, and given that there is (judging from your patch) no easy way to turn it off it seems odd that it would never be of any use.

I can understand that it may not be a good idea for the API but are you really saying that absolutely none of our code benefits from it?

comment:2 Changed 10 years ago by Grant Slater

The cache results are only used within the same web request and on the exact same query.

The patch only suites the API requests, and likely not the web requests. I'll have a go at a patch that only affects the API requests.

I would appreciate it if we could test disabling query cache for bulk queries. (response time + memory usage).

comment:3 Changed 10 years ago by Tom Hughes

I'll have a look at it this evening.

comment:4 Changed 10 years ago by shaun@…

Cc: shaun@… added

comment:5 Changed 10 years ago by Tom Hughes

Having now looked at the query cache code in rails I'm very much inclined to agree that turning it off globally may be a good thing - it is stunningly bad code that is missing any attempt whatsoever to manage the size of the cache so it has the potential to eat memory like mad.

It also has at least one wonderful bug that I spotted from a simple code inspection - if you change the database inside an uncached block that the cache won't be cleared so when you leave the uncached block you will have a stale cache... So using uncached blocks in critical places is probably not a good solution for us.

comment:6 Changed 10 years ago by Tom Hughes

Resolution: fixed
Status: newclosed

I've deployed this now so we can see how it goes.

Note: See TracTickets for help on using tickets.