Issue with Query Cache in Doctrine ORM Causing Cache Memory Overflow

[alebedev80]

Bug Report

Q	A
BC Break	no
Version	2.17.1

Summary

I am experiencing an issue with Doctrine ORM's query caching mechanism, leading to Redis memory overflow. This is primarily due to the way 'limit' and 'offset' values are handled in cached queries.

Current Behavior

In my production environment, Doctrine ORM caches each query including 'limit' and 'offset' as literal values in the SQL statement, rather than as parameters. As a result, every paginated request results in a new cache entry, with the only difference being the varying 'limit' and 'offset' values. This leads to an excessive number of cache entries in Redis.

How to Reproduce

• Configure Doctrine ORM with query caching, using Redis as the cache storage.
• Run a query that includes 'limit' and 'offset' clauses.
• Increment the 'offset' value (simulate pagination) and re-execute the query.
• Observe the creation of multiple cache entries in Redis for each query variation.

See test code here

Expected Behavior

The expected behavior is for Doctrine ORM to treat 'limit' and 'offset' as parameters rather than literals in the query. This approach would allow for a single cache entry to be applicable to multiple paginated requests of the same query, thereby reducing the number of cache entries and preventing cache memory overflow.

Additional Information
Redis version: 7
PHP version: 8.2
DB: MySQL 8
Environment: Production
Addressing this issue is critical for optimizing query caching efficiency in Doctrine ORM, especially for applications that heavily rely on pagination.

[alebedev80]

Hey somebody!

[greg0ire]

Can you detail the "how to reproduce" section with code?

[alebedev80]

@greg0ire please check
https://github.com/alebedev80/doctrine_orm_cache_overflow

[greg0ire]

Looked into it thanks to your reproducer, and this is where the ORM does the hardcoding (actually, you can see it delegates that job to DBAL):

orm/lib/Doctrine/ORM/Query/SqlWalker.php

Line 573 in e50ae06

$sql = $this->platform->modifyLimitQuery($sql, $limit, $offset);

Now we need to figure out why it's done like this.

[greg0ire]

Maybe it was just not possible to use parameter binding at the time, at least for some platforms: doctrine/dbal#3459 (comment)

[alebedev80]

@beberlei please help

[greg0ire]

I think our SECURITY.md should mention https://www.doctrine-project.org/policies/security.html

I'm AFK and will be for a week, so can't help much here. The next step would be to look into implementing a DBAL API that allows to use placeholders with limit and offset, and then leverage it in ORM.

[alebedev80]

I think our SECURITY.md should mention https://www.doctrine-project.org/policies/security.html

Ooops!

"If you think that you have found a security issue in Doctrine, please don't use the issue tracker in GitHub and don't publish it publicly. Instead, all security issues must be sent to..."

The next step would be to look into implementing a DBAL API that allows to use placeholders with limit and offset, and then leverage it in ORM.

So what i should to do? Create issue in doctrine/dbal?

[greg0ire]

Yeah, creating an issue in the DBAL seems good. And let me delete some of the last messages

[beberlei]

Its confusing, the firstResult+maxResults are in Query::getQueryCacheId, but no clue why, they are not relevant I believe. The last change on that line was just coding styles. https://github.com/doctrine/orm/blame/2.17.x/lib/Doctrine/ORM/Query.php#L819

Could you paste the output of your test.php to the README.md of that repo? then i dont have to execute this myself.

[greg0ire]

@beberlei it's like that since 04832e2, prompted by #1859

[alebedev80]

@beberlei please review README.md

[alebedev80]

@beberlei some news?

[mpdude]

One challenge I see is that \Doctrine\ORM\Query\SqlWalker::walkSelectStatement returns a string, and thus we cannot make it use parameters... who would know about these parameters, who would bind their values? The parameter names and/or values would need to be returned alongside the SQL string and be dealt with by the calling code.

Maybe one way out would be to add new methods that allow to set parameter names that shall be used in LIMIT and OFFSET clauses, and then have the same in DBAL to create the platform-specific SQL accordingly?

When choosing parameter names in an automated fashion, we need to take care that they are unique in case people do subqueries/subselects, to avoid parameter name collisions. At the same time, the names must not be random, otherwise they'd mess up the query cache as well. So, like predictable, unique parameter names used in queries that may not fully be constructed at the time the name is needed?

[mpdude]

Regarding the question why getQueryCacheId puts the offset/limit into the cache key... that's because the cache key/id is used to bypass query -> SQL compilation. When the resulting SQL contains the offset/limit as literals, we need to consider them in the lookup key.

[mpdude]

As I've said in the DBAL issue I don't believe this can easily be fixed or even prepared in DBAL, and then we'd also need similar changes here in ORM.

So I'd like to suggest another route:

Would it be possible to put (through a dedicated, additional interface?) the output walker into a dedicated "mode" where it uses a (random) placeholder string instead of the SQL expression for limit/offset handling? Then keep the placeholder name together with the unfinished SQL statement in the DQL->SQL cache, and fill in platform-specific limit/offset expressions at a later time, after the parserResult has been taken from the cache?

Cache key computation would probably need to know if this is going to work and only conditionally include offset/limit.

Things like \Doctrine\ORM\Query::getSQL need to be kept in mind, of course those need to return the full query as previously.

Update: Due to the way \Doctrine\DBAL\Platforms\AbstractPlatform::doModifyLimitQuery works (possibly constructing new SQL queries with the old query being a subquery), it cannot be a placeholder, but rather must be a full, deferred call to that method; and that means also other things (like the lock mode specific SQL) might need to be called after it.

[mpdude]

What cache are we talking about, the query (DQL->SQL) cache or the result cache?

[alebedev80]

What cache are we talking about, the query (DQL->SQL) cache or the result cache?

@mpdude the query (DQL->SQL) cache

[beberlei]

The query cache does not need this theoretically, because we can call modifiyLimit... on the resulting sql coming from the cache.

This should not even be a problem with paginator, because that has query hints that change the cache id

[mpdude]

Do we consider the ParserResult class or the AbstractSqlExecutor and subclasses as part of our public API?

The SqlWalker for sure is, it is subclassed in a documentation example.

[alebedev80]

Hey! how is it going?

[mpdude]

I need to find spare time to think through Benjamin‘s review 😉

[alebedev80]

@mpdude so you are only who is an able to fix it?

[alebedev80]

Half year passed. do you plan to fix it?

[mpdude]

I‘d love to fix it since I already put much effort into it and I think it’s feasible. Also I still have this item on my personal to-do list.

However, unless being paid for it, I’d have to do this in my spare free time, which is rather limited at the moment 😔. I’ll see what I can do, but no promises given.

[mpdude]

#11188 has just been merged into 2.20.x, and I think it fixes this issue here.