Do not query result cache twice in case of cache miss #4169
Conversation
| * @param string $cacheKey | ||
| * @param string $realKey | ||
| * @param int $lifetime | ||
| * @param array<string,mixed>|null $fetchedData |
There was a problem hiding this comment.
Should this new argument become mandatory in 3.0.x?
There was a problem hiding this comment.
I think, yes.
However, the class is renamed to CachingResult in 3.0
There was a problem hiding this comment.
Why don't we implement a proper fix in 3.0.x first and then backport in a BC-way? Otherwise, it's not clear what the new intended logic is.
There was a problem hiding this comment.
I don't know how you releasing process is working. I consider this bug as a minor so it doesn't make sense to break backward compatibility.
There was a problem hiding this comment.
Let's at least make it clear in the parameter description that this parameter will be mandatory in 3.0. Otherwise, it's not clear why fetching the data has to be implemented twice.
There was a problem hiding this comment.
I feel confused about how to do it all in 2.11.
I have opened the new #4189 for 3.0.x version with BC Break and mandatory $fetchedData field.
There was a problem hiding this comment.
I'm not that well familiar with the design of the caching layer and I still don't get the design: instead of fetching the data from the result, why does the result now use the fetched data and still can fetch from the result via fetch*() methods? Is this still necessary?
There was a problem hiding this comment.
The result cache is fetched in ResultCacheStatement only if no fetchedData is provided to the constructor. It is done for backward compatibility - someone could use ResultCacheStatement before and it should continue working without the last parameter.
DBAL uses this class only in one place and it always sends fetchedData. So, result cache won't be fetched 2 times if DBAL is used ordinarily.
There was a problem hiding this comment.
The result cache is fetched in
ResultCacheStatementonly if nofetchedDatais provided to the constructor.
Let's then continue the discussion in #4189 where BC is of no concern. The new class still depends on the Result.
k0ka
force-pushed
the
2.11-double-cache-fetch
branch
from
3f847c0 to
c61ef4a
Compare
k0ka
force-pushed
the
2.11-double-cache-fetch
branch
from
9e290c8 to
c3704d7
Compare
morozov
left a comment
morozov
left a comment
There was a problem hiding this comment.
There are three new code execution branches and not a single test. There must be a unit test that covers the logic of interaction with the external cache.
| $this->fetchedData = $this->resultCache->fetch($this->cacheKey); | ||
| } | ||
|
|
||
| if (! $this->fetchedData) { |
There was a problem hiding this comment.
Please avoid implicit conversion to boolean. It's not allowed in newer branches.
There was a problem hiding this comment.
Ok. This line was written before me - I've just change the variable name.
| $stmt = new ArrayStatement([]); | ||
| } | ||
| } else { | ||
| $data = []; |
There was a problem hiding this comment.
It is needed to distinguish "data from query cache is not loaded" and "data from query cache is loaded and empty".
This data was be set to empty array in ResultCacheStatement before -
3 participants
Summary
If the data is not found in the query cache, it will be queried again to append a new key to it. This PR removes the second fetching by passing the first result to ResultCacheStatement.
First cache fetch at
Connection::executeCacheQuery():dbal/lib/Doctrine/DBAL/Connection.php
Line 1201 in 8c9e92a
Second cache fetch at
ResultCacheStatement::saveToCache():dbal/lib/Doctrine/DBAL/Cache/ResultCacheStatement.php
Line 339 in 8c9e92a