Skip to content
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

Paginator breaks when DQL query is re-used with different identifier parameters that require conversion #7837

Closed
Ocramius opened this issue Sep 27, 2019 · 20 comments
Assignees
Milestone

Comments

@Ocramius
Copy link
Member

Ocramius commented Sep 27, 2019

Bug Report

Q A
BC Break no
Version 2.6.4

Summary

#7820 and #7821 fix pagination issues happening when the paginator is relying on identifiers that require custom DBAL type conversion.

The type conversion works, but since it has been performed inside the WhereInWalker (to keep things a bit simpler), it is only performed when the query is in Doctrine\ORM\Query::STATE_DIRTY.

If the parameters are changed on a Doctrine\ORM\Query, the query doesn't change parser state, and therefore the WhereInWalker is completely skipped.

I will try to write a small reproducer and fix ASAP, hoping to not cause too big performance regressions in the ORM. I think it may come down to forcing query parsing to be repeated.

Current behavior

$id = generateCustomId();
$a  = new A($id);

$this->em->persist($a);
$this->em->flush();

$query = $this->em->createQuery('SELECT a FROM ' . A::class);

$paginator = new Paginator($query);

$query->setFirstResult(1);

Assert::count(0, \iterator_to_array($paginator));

$query->setFirstResult(0);

Assert::same([$a], \iterator_to_array($paginator));

Expected behavior

The last assertion in the test above fails: this is an unfortunate case of mutable state shared inside Doctrine\ORM\Query.

@Ocramius Ocramius self-assigned this Sep 27, 2019
@Ocramius Ocramius added this to the 2.6.5 milestone Sep 27, 2019
@lcobucci
Copy link
Member

lcobucci commented Oct 1, 2019

@Ocramius this is an interesting one (which I didn't manage to reproduce in the way you described)...

As far as I've seen the Paginator doesn't influence the original query state because it clones it before applying the changes.

However, things get a bit weird when using (and changing) a query builder instead of a query object. This happens because QueryBuilder#getQuery() always returns a new query object:

<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\Ticket;

use Doctrine\Common\Collections\Criteria;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\StringType;
use Doctrine\DBAL\Types\Type;
use Doctrine\ORM\Tools\Pagination\Paginator;
use Doctrine\Tests\OrmFunctionalTestCase;
use function is_string;
use function iterator_to_array;

/**
 * @group GH7837
 */
final class GH7837Test extends OrmFunctionalTestCase
{
    private const SONG = [
        'What is this song all about?',
        'Can\'t figure any lyrics out',
        'How do the words to it go?',
        'I wish you\'d tell me, I don\'t know',
        'Don\'t know, don\'t know, don\'t know, I don\'t know!',
        'Don\'t know, don\'t know, don\'t know...',
    ];

    protected function setUp() : void
    {
        parent::setUp();

        if (! Type::hasType(GH7837LineTextType::class)) {
            Type::addType(GH7837LineTextType::class, GH7837LineTextType::class);
        }

        $this->setUpEntitySchema([GH7837Line::class, GH7837LineAuthor::class]);

        foreach (self::SONG as $index => $line) {
            $this->_em->persist(new GH7837Line(GH7837LineText::fromText($line), $index));
        }

        $this->_em->flush();
    }

    /** @test */
    public function paginatorWithQueryBuilder() : void // fails
    {
        $query = $this->_em->getRepository(GH7837Line::class)
            ->createQueryBuilder('l')
            ->orderBy('l.lineNumber', Criteria::ASC);

        $paginator = new Paginator($query);

        $query->setFirstResult(6);

        self::assertCount(0, iterator_to_array($paginator));

        $query->setFirstResult(5);

        $result = iterator_to_array($paginator);
        self::assertCount(1, $result);
        self::assertSame(self::SONG[5], $result[0]->toString());
    }

    /** @test */
    public function paginatorWithQueryFromQueryBuilder() : void // passes
    {
        $query = $this->_em->getRepository(GH7837Line::class)
            ->createQueryBuilder('l')
            ->orderBy('l.lineNumber', Criteria::ASC)
            ->getQuery();

        $paginator = new Paginator($query);

        $query->setFirstResult(6);

        self::assertCount(0, iterator_to_array($paginator));

        $query->setFirstResult(5);

        $result = iterator_to_array($paginator);
        self::assertCount(1, $result);
        self::assertSame(self::SONG[5], $result[0]->toString());
    }

    /** @test */
    public function paginatorWithDQL() : void // passes
    {
        $query = $this->_em->createQuery('SELECT l FROM ' . GH7837Line::class . ' l ORDER BY l.lineNumber ASC');

        $paginator = new Paginator($query);

        $query->setFirstResult(6);

        self::assertCount(0, iterator_to_array($paginator));

        $query->setFirstResult(5);

        $result = iterator_to_array($paginator);
        self::assertCount(1, $result);
        self::assertSame(self::SONG[5], $result[0]->toString());
    }
}

/** @Entity */
class GH7837LineAuthor
{
    /**
     * @Id
     * @Column(type="integer")
     *
     * @var int
     */
    public $id;

    /**
     * @Column
     *
     * @var string
     */
    public $name;

    /**
     * @ManyToOne(targetEntity=GH7837Line::class)
     * @JoinColumn(referencedColumnName="text")
     *
     * @var GH7837Line
     */
    public $line;

    public function __construct(int $id, string $name, GH7837Line $line)
    {
        $this->id = $id;
        $this->name = $name;
        $this->line = $line;
    }
}

/** @Entity */
class GH7837Line
{
    /**
     * @var GH7837LineText
     * @Id()
     * @Column(type="Doctrine\Tests\ORM\Functional\Ticket\GH7837LineTextType")
     */
    private $text;

    /**
     * @var int
     * @Column(type="integer")
     */
    private $lineNumber;

    public function __construct(GH7837LineText $text, int $index)
    {
        $this->text       = $text;
        $this->lineNumber = $index;
    }

    public function toString() : string
    {
        return $this->text->getText();
    }
}

final class GH7837LineText
{
    /** @var string */
    private $text;

    private function __construct(string $text)
    {
        $this->text = $text;
    }

    public static function fromText(string $text) : self
    {
        return new self($text);
    }

    public function getText() : string
    {
        return $this->text;
    }

    public function __toString() : string
    {
        return 'Line: ' . $this->text;
    }
}

final class GH7837LineTextType extends StringType
{
    public function convertToPHPValue($value, AbstractPlatform $platform)
    {
        $text = parent::convertToPHPValue($value, $platform);

        if (! is_string($text)) {
            return $text;
        }

        return GH7837LineText::fromText($text);
    }

    public function convertToDatabaseValue($value, AbstractPlatform $platform)
    {
        if (! $value instanceof GH7837LineText) {
            return parent::convertToDatabaseValue($value, $platform);
        }

        return parent::convertToDatabaseValue($value->getText(), $platform);
    }

    /** {@inheritdoc} */
    public function getName() : string
    {
        return self::class;
    }
}

@Ocramius
Copy link
Member Author

Ocramius commented Oct 1, 2019 via email

@ofriedrich
Copy link

Maybe this is interesting too: KnpLabs/KnpPaginatorBundle#578

@Ocramius
Copy link
Member Author

Ocramius commented Oct 9, 2019

Ok, the bug is related to caching: the WhereInWalker is only hit if the query is not present in the query cache.

If there's a cache hit for the parser result, then the walker is not reached, and parameters aren't converted.

I'll write up a test case tonight.

Ocramius added a commit to Ocramius/doctrine2 that referenced this issue Oct 10, 2019
Since `WhereInWalker` does not run, query parameters are not translated
from their in-memory type to the expected SQL type when the paginator
is run again with the same DQL string. This is an architectural
issue, since (for the sake of simplicity) we moved parameter
translation into the SQL walker, we didn't consider that SQL
walkers only act when no cache is in place. The translatio
needs to be moved into the paginator logic again.
Ocramius added a commit to Ocramius/doctrine2 that referenced this issue Oct 10, 2019
…ng used

In order to figure out the paginated query identifier type, we would
have to parse the DQL query into an AST+SQL anyway, so we'd have
to re-parse it manually: instead of doing that, we can force the
`WhereInWalker` to be reached at all times by forcing the
`$whereInQuery` to use no query cache.

While it is a sad performance regression, it is also not a
noticeable one, since we'll be performing an `O(1)` operation
around an I/O one (query execution, in this case).
@akorz
Copy link
Contributor

akorz commented Oct 23, 2019

I'm using ramsey/uuid and after my latest dependecy update I hit that error. I tried to add your fix in my vendor code and run my tests. No luck. the same errors :(

@Ocramius
Copy link
Member Author

Have you tried the related patch?

@akorz
Copy link
Contributor

akorz commented Oct 23, 2019

Do you mean this 023e946 ? Yes, I added that line in my code and problem is still there :(

@Ocramius
Copy link
Member Author

@akorz can you try distilling that into a test case then, please? I think you hit a different scenario...

@akorz
Copy link
Contributor

akorz commented Oct 23, 2019

@Ocramius let me figure out how to do it

@akorz
Copy link
Contributor

akorz commented Oct 24, 2019

@Ocramius I tried to adapt existed test for enities with custom id. Here is my test https://github.com/akorz/orm/commit/a6ce54330eb69afdc66c8dbc03b7c795262a2804

And the result error is a different what I get on real project, but probably it's somehow releted

There was 1 error:

1) Doctrine\Tests\ORM\Functional\PaginationCustomTypeIdTest::testQueryWalkerIsKept
Exception: [PHPUnit\Framework\Error\Notice] Trying to get property 'id' of non-object

With queries:
198. SQL: 'SELECT DISTINCT c0_.id AS id_0 FROM cms_users_customid c0_ WHERE c0_.name = 'Name1'' Params: 
197. SQL: '"COMMIT"' Params: 
196. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
195. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
194. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
193. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
192. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
191. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
190. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
189. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
188. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
187. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
186. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
185. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
184. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
183. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
182. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
181. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
180. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
179. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
178. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
177. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
176. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
175. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
174. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject

Trace:
/app/tests/Doctrine/Tests/DbalTypes/CustomIdObjectType.php:18
/app/vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:1499
/app/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php:181
/app/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php:182
/app/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php:119
/app/lib/Doctrine/ORM/Query/TreeWalkerChain.php:113
/app/lib/Doctrine/ORM/Query/Parser.php:389
/app/lib/Doctrine/ORM/Query.php:286
/app/lib/Doctrine/ORM/Query.php:298
/app/lib/Doctrine/ORM/AbstractQuery.php:968
/app/lib/Doctrine/ORM/AbstractQuery.php:923
/app/lib/Doctrine/ORM/AbstractQuery.php:726
/app/lib/Doctrine/ORM/Tools/Pagination/Paginator.php:170
/app/tests/Doctrine/Tests/ORM/Functional/PaginationCustomTypeIdTest.php:641
/app/vendor/phpunit/phpunit/src/Framework/TestCase.php:1154
/app/vendor/phpunit/phpunit/src/Framework/TestCase.php:842
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:693
/app/vendor/phpunit/phpunit/src/Framework/TestCase.php:796
/app/vendor/phpunit/phpunit/src/Framework/TestSuite.php:746
/app/vendor/phpunit/phpunit/src/Framework/TestSuite.php:746
/app/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:652
/app/vendor/phpunit/phpunit/src/TextUI/Command.php:206
/app/vendor/phpunit/phpunit/src/TextUI/Command.php:162
/app/vendor/phpunit/phpunit/phpunit:61


/app/tests/Doctrine/Tests/OrmFunctionalTestCase.php:842

Caused by
PHPUnit\Framework\Error\Notice: Trying to get property 'id' of non-object

/app/tests/Doctrine/Tests/DbalTypes/CustomIdObjectType.php:18
/app/vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:1499
/app/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php:181
/app/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php:182
/app/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php:119
/app/lib/Doctrine/ORM/Query/TreeWalkerChain.php:113
/app/lib/Doctrine/ORM/Query/Parser.php:389
/app/lib/Doctrine/ORM/Query.php:286
/app/lib/Doctrine/ORM/Query.php:298
/app/lib/Doctrine/ORM/AbstractQuery.php:968
/app/lib/Doctrine/ORM/AbstractQuery.php:923
/app/lib/Doctrine/ORM/AbstractQuery.php:726
/app/lib/Doctrine/ORM/Tools/Pagination/Paginator.php:170
/app/tests/Doctrine/Tests/ORM/Functional/PaginationCustomTypeIdTest.php:641

--

@Ocramius
Copy link
Member Author

That diff is gigantic: can you reduce it to just the failure?

@lcobucci
Copy link
Member

And also try to keep the entities in the same file as the test 👍

@akorz
Copy link
Contributor

akorz commented Oct 24, 2019

@lcobucci What do you mean? I made it in the same way how did pagination test for entities with autogenerated id

@akorz
Copy link
Contributor

akorz commented Oct 25, 2019

@Ocramius I got that in

    public function testQueryWalkerIsKept()
    {
        $dql = 'SELECT u FROM Doctrine\Tests\Models\CMSCustomId\CmsUser u';
        $query = $this->_em->createQuery($dql);
        $query->setHint(Query::HINT_CUSTOM_TREE_WALKERS, [CustomPaginationTestTreeWalker::class]);

        $paginator = new Paginator($query, true);
        $paginator->setUseOutputWalkers(false);
        $this->assertCount(1, $paginator->getIterator());
        $this->assertEquals(1, $paginator->count());
    }

I guess bug came from #7820
Pay attention that it test uses Doctrine\Tests\Models\CMSCustomId\CmsUser

Where

class CmsUser
{
   /**
    * @Id
    * @Column(type="CustomIdObject")
    * @GeneratedValue(strategy="NONE")
    */
   public $id;

@akorz
Copy link
Contributor

akorz commented Oct 25, 2019

I tried to simple delete that code
https://github.com/doctrine/orm/pull/7821/files#diff-1a0c7b79092b175a78f64ed6c44c8c8cR113

My test passed.

So, looks like it's a reason for #7821 (comment) and why #7865 did not help me

@akorz
Copy link
Contributor

akorz commented Oct 29, 2019

@Ocramius had you get a chance to check it out?

@Ocramius
Copy link
Member Author

@akorz can you open a PR with the failing test? Also, what if #7865 is factored in?

@lcobucci
Copy link
Member

@lcobucci What do you mean? I made it in the same way how did pagination test for entities with autogenerated id

@akorz the link you sent has entities and test in separate files. For the sake of isolation and easier reproducibility we're asking to always create new entities in the same file as the test.

@akorz
Copy link
Contributor

akorz commented Nov 2, 2019

@Ocramius sorry for delay. Pull is here #7886 I hope I made it correct

@lcobucci I see. Of course I can do it, but I though that models in tests/Doctrine/Tests/Models/CMS were created with some intention behind. Thats why I did it similar.

lcobucci added a commit that referenced this issue Nov 15, 2019
…-identifier-types-even-with-cached-dql-parsing

#7837 paginate with custom identifier types even with enabled DQL query cache
@lcobucci
Copy link
Member

Handled by #7865

greg0ire added a commit that referenced this issue Jan 21, 2023
Make sure tests from #7837 are actually run
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 22, 2023
This PR prevents the Paginator from causing OpCache "wasted memory" to increase _on every request_ when used with Symfony's `PhpFilesAdapter` as the cache implementation for the query cache.

Depending on configured thresholds, wasted memory this will either cause periodic opcache restarts or running out of memory and not being able to cache additional scripts ([Details](https://tideways.com/profiler/blog/fine-tune-your-opcache-configuration-to-avoid-caching-suprises)).

Fixes doctrine#9917, closes doctrine#10095.

There is a long story (doctrine#7820, doctrine#7821, doctrine#7837, doctrine#7865) behind how the Paginator can take care of DBAL type conversions when creating the pagination query. This conversion has to transform identifier values before they will be used as a query parameter, so it has to happen every time the Paginator is used.

For reasons, this conversion happens inside `WhereInWalker`. Tree walkers like this are used only during the DQL parsing/AST processing steps. Having a DQL query in the query cache short-cuts this step by fetching the parsing/processing result from the cache.

So, to make sure the conversion happens also with the query cache being enabled, this line

https://github.com/doctrine/orm/blob/1753d035005c1125c9fb4855c3fa629341e5734d/lib/Doctrine/ORM/Tools/Pagination/Paginator.php#L165

was added in doctrine#7837. It causes `\Doctrine\ORM\Query::parse()` to re-parse the query every time, but will also put the result into the query cache afterwards.

At this point, the setup described in doctrine#9917 – which, to my knowledge, is the default in Symfony + DoctrineBundle projects – will ultimately bring us to this code:

https://github.com/symfony/symfony/blob/4b3391725f2fc4a072e776974f00a992cbc70515/src/Symfony/Component/Cache/Adapter/PhpFilesAdapter.php#L248-L249

When writing a cache item with an already existing key, the driver has to make sure the opcache will honor the changed PHP file. This is what causes _wasted memory_ to increase.

Instead of using `\Doctrine\ORM\Query::expireQueryCache()`, which will force `\Doctrine\ORM\Query::parse()` to parse the query again before putting it into the cache, use `\Doctrine\ORM\Query::useQueryCache(false)`. The subtle difference is the latter will not place the processed query in the cache in the first place.

A test case is added to check that repeated use of the paginator does not call the cache to update existing keys. That should suffice to make sure we're not running into the issue, while at the same time not complicating tests by using the `PhpFilesAdapter` directly.

Note that in order to observe the described issue in tests, you will need to use the `PhpFilesDriver` and also make sure that OpCache is enabled and also activated for `php-cli` (which is running the unit tests).

This particular subquery generated/used by the Paginator is not put into the query cache. The DQL parsing/to-SQL conversion has to happen _every time_ the Paginator is used.

This, however, was already the case before this PR. In other words, this PR only changes that we do not store/update the cached result every time, but instead completely omit caching the query.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 23, 2023
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 identifier, and we have to derive it from 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. In addition to the processing overhead, this may also waste opcache memory (doctrine#9917).

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.
derrabus pushed a commit that referenced this issue Jan 23, 2023
This PR prevents the Paginator from causing OpCache "wasted memory" to increase _on every request_ when used with Symfony's `PhpFilesAdapter` as the cache implementation for the query cache.

Depending on configured thresholds, wasted memory this will either cause periodic opcache restarts or running out of memory and not being able to cache additional scripts ([Details](https://tideways.com/profiler/blog/fine-tune-your-opcache-configuration-to-avoid-caching-suprises)).

Fixes #9917, closes #10095.

There is a long story (#7820, #7821, #7837, #7865) behind how the Paginator can take care of DBAL type conversions when creating the pagination query. This conversion has to transform identifier values before they will be used as a query parameter, so it has to happen every time the Paginator is used.

For reasons, this conversion happens inside `WhereInWalker`. Tree walkers like this are used only during the DQL parsing/AST processing steps. Having a DQL query in the query cache short-cuts this step by fetching the parsing/processing result from the cache.

So, to make sure the conversion happens also with the query cache being enabled, this line

https://github.com/doctrine/orm/blob/1753d035005c1125c9fb4855c3fa629341e5734d/lib/Doctrine/ORM/Tools/Pagination/Paginator.php#L165

was added in #7837. It causes `\Doctrine\ORM\Query::parse()` to re-parse the query every time, but will also put the result into the query cache afterwards.

At this point, the setup described in #9917 – which, to my knowledge, is the default in Symfony + DoctrineBundle projects – will ultimately bring us to this code:

https://github.com/symfony/symfony/blob/4b3391725f2fc4a072e776974f00a992cbc70515/src/Symfony/Component/Cache/Adapter/PhpFilesAdapter.php#L248-L249

When writing a cache item with an already existing key, the driver has to make sure the opcache will honor the changed PHP file. This is what causes _wasted memory_ to increase.

Instead of using `\Doctrine\ORM\Query::expireQueryCache()`, which will force `\Doctrine\ORM\Query::parse()` to parse the query again before putting it into the cache, use `\Doctrine\ORM\Query::useQueryCache(false)`. The subtle difference is the latter will not place the processed query in the cache in the first place.

A test case is added to check that repeated use of the paginator does not call the cache to update existing keys. That should suffice to make sure we're not running into the issue, while at the same time not complicating tests by using the `PhpFilesAdapter` directly.

Note that in order to observe the described issue in tests, you will need to use the `PhpFilesDriver` and also make sure that OpCache is enabled and also activated for `php-cli` (which is running the unit tests).

This particular subquery generated/used by the Paginator is not put into the query cache. The DQL parsing/to-SQL conversion has to happen _every time_ the Paginator is used.

This, however, was already the case before this PR. In other words, this PR only changes that we do not store/update the cached result every time, but instead completely omit caching the query.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 23, 2023
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 identifier, and we have to derive it from 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. In addition to the processing overhead, this may also waste opcache memory (doctrine#9917).

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.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 23, 2023
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 identifier, and we have to derive it from 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. In addition to the processing overhead, this may also waste opcache memory (doctrine#9917).

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.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 23, 2023
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 identifier, and we have to derive it from 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. In addition to the processing overhead, this may also waste opcache memory (doctrine#9917).

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.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 23, 2023
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 identifier, and we have to derive it from 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. In addition to the processing overhead, this may also waste opcache memory (doctrine#9917).

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.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 23, 2023
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 pushed a commit to mpdude/doctrine2 that referenced this issue Feb 5, 2023
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.
Gwemox pushed a commit to Gwemox/orm that referenced this issue Feb 10, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants