Make Paginator-internal query cacheable in the query cache #10444
Conversation
Marking as draft for now. 🙂 |
mpdude
changed the title
mpdude
force-pushed
the
paginator-dql-cacheable
branch
from
48f1e3f to
b4eb7f3
Compare
@derrabus will you ignore PRs as long as they are drafts, and should I switch back to "ready for review" when I would like to get feedback (even if it is not 💯 ready yet)? |
I can ignore non-draft PRs just as good as drafts, no worries. Don't hack my backlog. ✌🏻 |
mpdude
force-pushed
the
paginator-dql-cacheable
branch
2 times, most recently
from
648c576 to
54cf0ec
Compare
| ); | ||
| } | ||
|
|
||
| public function testWillReplaceBoundQueryIdentifiersWithConvertedTypesAsPerIdentifierMapping(): void |
There was a problem hiding this comment.
This is no longer the WhereInWalker's responsibility
| ); | ||
| } | ||
|
|
||
| public function testWillReplaceBoundQueryIdentifiersWithConvertedTypesAsPerAssociatedEntityIdentifierMapping(): void |
There was a problem hiding this comment.
This is no longer the WhereInWalker's responsibility
mpdude
force-pushed
the
paginator-dql-cacheable
branch
3 times, most recently
from
66a05c2 to
643a5cf
Compare
|
Ready for review. Please advise how to resolve the Psalm issue. |
mpdude
changed the title
|
Rebasing on ed56f42 should fix the issue. |
mpdude
force-pushed
the
paginator-dql-cacheable
branch
from
643a5cf to
63af0bb
Compare
|
Computer says no.
Shall I update the baseline in/for this PR? That's probably gonna cause a merge conflict down the road. |
Oh right, didn't realised but yes, there is one less entry. I wouldn't worry about the merge conflicts. |
Make the `Paginator`-internal query (`... WHERE ... IN (id, id2, id3...)`) cacheable in the query cache again. When the Paginator creates the internal subquery that does the actual result limiting, it has to take DBAL type conversions for the identifier column of the paginated root entity into account (doctrine#7820, fixed in doctrine#7821). In order to perform this type conversion, we need to know the DBAL type class for the root entity's `#[Id]`, and we have to figure it out based on a given (arbitrary) DQL query. This requires DQL parsing and inspecting the AST, so doctrine#7821 placed the conversion code in the `WhereInWalker` where all the necessary information is available. The problem is that type conversion has to happen every time the paginator is run, but the query that results from running `WhereInWalker` would be kept in the query cache. This was reported in doctrine#7837 and fixed by doctrine#7865, by making this particular query expire every time. The query must not be cached, since the necessary ID type conversion happens as a side-effect of running the `WhereInWalker`. The Paginator internal query that uses `WhereInWalker` has its DQL re-parsed and transformed in every request. This PR moves the code that determines the DBAL type out of `WhereInWalker` into a dedicated SQL walker class, `RootTypeWalker`. `RootTypeWalker` uses a ~hack~ clever trick to report the type back: It sets the type as the resulting "SQL" string. The benefit is that `RootTypeWalker` results can be cached in the query cache themselves. Only the first time a given DQL query has to be paginated, we need to run this walker to find out the root entity's ID type. After that, the type will be returned from the query cache. With the type information being provided, `Paginator` can take care of the necessary conversions by itself. This happens every time the Paginator is used. The internal query that uses `WhereInWalker` can be cached again since it no longer has side effects.
greg0ire
force-pushed
the
paginator-dql-cacheable
branch
from
12d78ec to
77df5db
Compare
|
@greg0ire Class is final now, please re-approve |
|
🎉 |
|
Thanks @mpdude ! Right after clicking merged, I noticed the target branch. Not sure it should really have been 2.14.x but oh well… |
|
I think that’s fine. I see no risk for breakage and the same argument as in #8797 (comment) applies |
|
Maybe we can also bring #8797 to an end? It’s closely related, and I’ll resolve merge conflicts shortly. |
Make the
Paginator-internal query (... WHERE ... IN (id, id2, id3...)) cacheable in the query cache again.Background
When the Paginator creates the internal subquery that does the actual result limiting, it has to take DBAL type conversions for the identifier column of the paginated root entity into account (#7820, fixed in #7821).
In order to perform this type conversion, we need to know the DBAL type class for the root entity's
#[Id], and we have to figure it out based on a given (arbitrary) DQL query. This requires DQL parsing and inspecting the AST, so #7821 placed the conversion code in theWhereInWalkerwhere all the necessary information is available.The problem is that type conversion has to happen every time the paginator is run, but the query that results from running
WhereInWalkerwould be kept in the query cache. This was reported in #7837 and fixed by #7865, by making this particular query expire every time. The query must not be cached, since the necessary ID type conversion happens as a side-effect of running theWhereInWalker.Current status
The Paginator internal query that uses
WhereInWalkerhas its DQL re-parsed and transformed in every request.Suggested solution
This PR moves the code that determines the DBAL type out of
WhereInWalkerinto a dedicated SQL walker class,RootTypeWalker.RootTypeWalkeruses ahackclever trick to report the type back: It sets the type as the resulting "SQL" string. The benefit is thatRootTypeWalkerresults can be cached in the query cache themselves. Only the first time a given DQL query has to be paginated, we need to run this walker to find out the root entity's ID type. After that, the type will be returned from the query cache.With the type information being provided,
Paginatorcan take care of the necessary conversions by itself. This happens every time the Paginator is used.The internal query that uses
WhereInWalkercan be cached again since it no longer has side effects.