-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Always cache the full result #5003
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
Conversation
304ad32 to
b63f43a
Compare
b63f43a to
373db6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think docs/en/reference/caching.rst will need to be changed, since users no longer need to explicitly read the entire result set from the database. We should instead warn them that using the caching feature does that implicitly. Maybe we should also document it as a minor BC break in the upgrade guide?
373db6a to
9f64c2b
Compare
Thanks! Updated.
Should we? IMO the current implementation is the most reasonable and doesn't need documenting technical details.
What is the BC break? |
greg0ire
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the BC break?
The result set is now implicitly read entirely. If somebody uses this API to read the first row of a query that returns many results, they might have memory issues. But As I write this lines, I realize this is too far-fetched 😅 . Ignore me.
Fixes #4178. Closes #4189.
CachingResultto the wrapper connection. This way, we can remove the redundant call to the cache.CachingResultas no longer used.testDontFinishNoCache()as no longer valid.ResultCacheTestsince it covers the whole feature, not a specific class.