Invalidate old query cache format#6510
Merged
derrabus merged 1 commit intodoctrine:4.2.xfrom Aug 30, 2024
Merged
Conversation
greg0ire
previously approved these changes
Aug 29, 2024
Member
greg0ire
left a comment
There was a problem hiding this comment.
Looks like it also reduces the cache size 👍
morozov
previously approved these changes
Aug 30, 2024
6fcf73d to
773774e
Compare
Member
|
Thanks @derrabus for handling this! |
Slamdunk
reviewed
Oct 17, 2024
|
|
||
| return new Result(new ArrayResult($columnNames, $rows), $this); | ||
| if (isset($value[$realKey]) && $value[$realKey] instanceof ArrayResult) { | ||
| return new Result($value[$realKey], $this); |
Contributor
There was a problem hiding this comment.
The changes made here in Connection introduced a
BC Break
Before this PR, the values in the cache were uncoupled from the ArrayResult.
Since this PR, the cache and the Result/ArrayResult are now coupled: hitting the \Doctrine\DBAL\Result::free method erases all the cache content as well 😱
Member
Author
There was a problem hiding this comment.
Please file a bug report with clear steps to reproduce.
derrabus
pushed a commit
that referenced
this pull request
Oct 21, 2024
Bug emerged in #6510 (comment) The current implementation of query cache relies on the cache **not** saved by-reference; the bug has never been seen before because by default `new ArrayAdapter()` saves the cache with serialization, hence breaking the by-reference pointer. Once the _by-reference_ tecnique is used, two issues pop up: 1. `\Doctrine\DBAL\Cache\ArrayResult::$num` is never reset, so once it gets incremented in the first `\Doctrine\DBAL\Cache\ArrayResult::fetch` call, the following calls will always fail 2. Even considering fixing the `$num` property reset, a manual call on `\Doctrine\DBAL\Result::free` will by cascade call the `\Doctrine\DBAL\Cache\ArrayResult::free` method erasing all the saved results I think that the `ArrayResult` implementation is not the culprit, but rather the #6510 giving to the cache backend the internal object by reference instead of giving it a copy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I'm changing the query cache format once again: Instead of storing a tuple of arrays, I'm now storing the whole
ArrayResultobject. This way, we can easily detect if we support the data returned from the cache and we can handle future cache format changes in the__unserialize()methods.After #6504, we would run into errors if an app would attempt to us a cache written with DBAL 4.1. With my change, the old cache is ignored and the app would behave as if it had encountered a cache miss instead.
I've also added tests covering the serialization of
ArrayResultobjects.