Add a partial paginator to prevent SQL COUNT queries #1292

meyerbaptiste · 2017-07-28T15:06:49Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	api-platform/docs#266

Recently we had to profile an API Platform application. This one had quite long response times because of the Doctrine paginator and more specifically the SQL COUNT() query it executes. Based on theses observations, I created a partial paginator which doesn't execute this query and therefore returns only the previous and the next page.

Next step: next and previous page with a cursor (Facebook, Twitter, etc. like).

Cc @thomasglachant

meyerbaptiste · 2017-07-28T15:12:37Z

src/Bridge/Doctrine/Orm/AbstractPaginator.php

+use ApiPlatform\Core\DataProvider\PartialPaginatorInterface;
+use Doctrine\ORM\Tools\Pagination\Paginator;
+
+abstract class AbstractPaginator implements \IteratorAggregate, PartialPaginatorInterface


FYI,

abstract class AbstractPaginator implements PartialPaginatorInterface, \IteratorAggregate => fatal error
abstract class AbstractPaginator implements \IteratorAggregate, PartialPaginatorInterface => it works

🤔 🤣

soyuka · 2017-07-28T15:23:34Z

src/Bridge/Doctrine/Orm/Extension/QueryResultItemExtensionInterface.php

     *
     * @return mixed
     */
-    public function getResult(QueryBuilder $queryBuilder);
+    public function getResult(QueryBuilder $queryBuilder/*, string $resourceClass, string $operationName = null*/);


To keep consistency with QueryResultCollectionExtensionInterface but yes, maybe it's useless...

Keep it, at least we have them if we need it later :P.

soyuka · 2017-07-28T15:25:55Z

Shouldn't you change the CollectionDataProvider and the SubresourceDataProvider also?

bendavies · 2017-07-28T15:30:01Z

I also noticed this performance hit.
One thing that could be improved is removing left joins on the count query, which dramatically improved performance for my use case.

soyuka · 2017-07-28T15:49:10Z

👍 @bendavies thought about this too but the doctrine paginator is a dark place to change things like this (it works on the same basis QueryBuilder for all the queries). If you managed to do it I'd be interested in the how.

bendavies · 2017-07-28T16:01:30Z

@soyuka i can't see much apart from writing our own Paginator or decorating it and overriding count

soyuka · 2017-07-28T18:57:08Z

@soyuka i can't see much apart from writing our own Paginator or decorating it and overriding count

I meant to remove leftJoins from the COUNT query but not from the end query. Anyway this pull request gives the ability to skip the COUNT :).

meyerbaptiste · 2017-07-28T21:18:52Z

If I remember correctly, on the project we profiled there was already a decorator to remove in some cases the LEFT JOIN part of the query but that did not prevent a long execution time.

soyuka · 2017-07-31T14:49:33Z

src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php

@@ -96,13 +96,16 @@ public function getConfigTreeBuilder()
                            ->addDefaultsIfNotSet()
                            ->children()
                                ->booleanNode('enabled')->defaultTrue()->info('To enable or disable pagination for all resource collections by default.')->end()
+                                ->booleanNode('partial')->defaultFalse()->info('')->end()


Can you add an info?

soyuka · 2017-07-31T14:52:35Z

src/Hal/Serializer/CollectionNormalizer.php


-        $currentPage = $lastPage = $itemsPerPage = null;
-        if ($isPaginator) {
+        $currentPage = $lastPage = $itemsPerPage = $pageTotalItems = null;


Whew, lots of variables initiated to nil, is this required?

Because of this?

------ --------------------------------------------- Line src/Hal/Serializer/CollectionNormalizer.php ------ --------------------------------------------- 87 Undefined variable: $currentPage 92 Undefined variable: $lastPage 97 Undefined variable: $currentPage 98 Undefined variable: $currentPage 101 Undefined variable: $currentPage 101 Undefined variable: $pageTotalItems 101 Undefined variable: $itemsPerPage 102 Undefined variable: $currentPage 123 Undefined variable: $itemsPerPage ------ --------------------------------------------- ------ ---------------------------------------------------------- Line src/Hydra/Serializer/PartialCollectionViewNormalizer.php ------ ---------------------------------------------------------- 75 Undefined variable: $currentPage 80 Undefined variable: $lastPage 85 Undefined variable: $currentPage 86 Undefined variable: $currentPage 89 Undefined variable: $currentPage 89 Undefined variable: $pageTotalItems 89 Undefined variable: $itemsPerPage 90 Undefined variable: $currentPage ------ ----------------------------------------------------------

soyuka · 2017-07-31T14:53:49Z

src/Hydra/Serializer/PartialCollectionViewNormalizer.php

-            if (1. === $currentPage && 1. === $lastPage) {
-                // Consider the collection not paginated if there is only one page
-                $paginated = false;
+        $currentPage = $lastPage = $itemsPerPage = $pageTotalItems = null;


same I wonder why is this needed

soyuka · 2017-07-31T16:13:45Z

src/Bridge/Doctrine/Orm/AbstractPaginator.php

+
+        if (null === ($firstResult = $query->getFirstResult()) || null === $maxResults = $query->getMaxResults()) {
+            throw new InvalidArgumentException(sprintf('"%1$s::setFirstResult()" or/and "%1$s::setMaxResults()" was/were not applied to the query.', Query::class));
+        }


I saw the test below but I don't get why we need such message (as we didn't needed it before)?

Because these two methods can potentially return null. See the Doctrine PHPDoc:

Returns NULL if {link setFirstResult} was not applied to this query.
Returns NULL if {link setMaxResults} was not applied to this query.

We always call Query::setFirstResult() and Query::setMaxResults() before instantiating our Paginator class, it's why we never had the bug.

We always call Query::setFirstResult() and Query::setMaxResults() before instantiating our Paginator class, it's why we never had the bug.

Yes that's the reason why we don't need the test no? I mean we're still always using setFirstResult and setMaxResults or I miss something.

Yes but this is a library and this class is not internal so users are free to use it as they want. IMO we have to prevent a fatal error by preventing this specific use case.

Okay wasn't sure this was the reason but I thought about it! Thanks for clarifying!

soyuka

phpstan issues but then 👍

Simperfit · 2017-08-23T20:40:04Z

@meyerbaptiste could you please rebase ? this should be ok to merge, @dunglas could you please look at this ?

Simperfit · 2017-09-05T15:19:36Z

@meyerbaptiste could you rebase again, so we can merge it ?

dunglas · 2017-09-13T05:11:39Z

src/Bridge/Doctrine/Orm/AbstractPaginator.php

+    /**
+     * {@inheritdoc}
+     */
+    public function getIterator()


You can add the \Traversable return type even if not in the parent class.

dunglas · 2017-09-13T05:13:18Z

phpstan.neon

@@ -14,3 +14,8 @@ parameters:
        - '#Parameter \#2 \$dqlPart of method Doctrine\\ORM\\QueryBuilder::add\(\) expects Doctrine\\ORM\\Query\\Expr\\Base, Doctrine\\ORM\\Query\\Expr\\Join\[\] given#' # Fixed in Doctrine's master
        - '#Call to an undefined method Doctrine\\Common\\Persistence\\ObjectManager::getConnection\(\)#'
        - '#Parameter \#1 \$callable of static method Doctrine\\Common\\Annotations\\AnnotationRegistry::registerLoader\(\) expects callable, mixed\[\] given#'
+        - '#Method ApiPlatform\\Core\\Bridge\\Doctrine\\Orm\\Extension\\QueryResult(Item|Collection)ExtensionInterface::getResult\(\) invoked with 3 parameters, 1 required#'
+        - '#Undefined variable: \$currentPage#'


These Undefined variable look like code smell.

dunglas · 2017-09-13T05:13:38Z

src/Bridge/Doctrine/Orm/AbstractPaginator.php

+    /**
+     * {@inheritdoc}
+     */
+    public function count()


Same here (: int)

dunglas · 2017-09-13T05:15:07Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

+        if (func_num_args() >= 2) {
+            $resourceClass = func_get_arg(1);
+        } else {
+            @trigger_error(sprintf('Method %s() will have a 2nd `string $resourceClass` argument in version 3.0. Not defining it is deprecated since 2.1.', __METHOD__), E_USER_DEPRECATED);


2.2 (same everywhere)

dunglas · 2017-09-13T05:16:02Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

        $doctrineOrmPaginator = new DoctrineOrmPaginator($queryBuilder, $this->useFetchJoinCollection($queryBuilder));
        $doctrineOrmPaginator->setUseOutputWalkers($this->useOutputWalkers($queryBuilder));

+        $resourceMetadata = null !== $resourceClass ? $this->resourceMetadataFactory->create($resourceClass) : null;


null === $resourceClass ? null : $this->resourceMetadataFactory->create($resourceClass) (negation not needed).

dunglas · 2017-09-13T05:17:16Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

+        $enabled = $this->partial;
+        $clientEnabled = $this->clientPartial;
+
+        if ($resourceMetadata) {


null !== (same for all tests below)

Should I do it after yesterday's conversation? 😛

dunglas · 2017-09-13T05:19:10Z

src/Bridge/Doctrine/Orm/Paginator.php


 /**
 * Decorates the Doctrine ORM paginator.
 *
 * @author Kévin Dunglas <[email protected]>
 */
-final class Paginator implements \IteratorAggregate, PaginatorInterface
+final class Paginator extends AbstractPaginator implements PaginatorInterface


implements PaginatorInterface can be removed (already implemented by the abstract class).

Nop, AbstractPaginator implements PartialPaginatorInterface.

No, keep it as is :) Sorry.

dunglas · 2017-09-13T05:20:48Z

src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php

                                ->booleanNode('client_enabled')->defaultFalse()->info('To allow the client to enable or disable the pagination.')->end()
                                ->booleanNode('client_items_per_page')->defaultFalse()->info('To allow the client to set the number of items per page.')->end()
+                                ->booleanNode('client_partial')->defaultFalse()->info('')->end()


info('') must be filled (BTW, I'm not sure that it's really necessary to be able to set this option client-side, but as it's implemented, keep it)

dunglas · 2017-09-13T05:23:05Z

src/Hal/Serializer/CollectionNormalizer.php


-        $currentPage = $lastPage = $itemsPerPage = null;


I think those changes introduce the PHPStan errors. It should probably be reverted.

Yep, see #1292 (comment)

Feels weird to me to declare empty variables but maybe it's better... Your call @dunglas.

It makes the execution path more predictible for static analysis tools (and even for humans IMO).

Humans > Robots

meyerbaptiste · 2017-09-14T12:28:41Z

Done @dunglas.

soyuka · 2017-09-14T12:39:59Z

src/Hal/Serializer/CollectionNormalizer.php

+            if ($object instanceof PaginatorInterface) {
+                $paginated = 1. !== $lastPage = $object->getLastPage();
+            } else {
+                $lastPage = null;


Is this one needed if we declared it above?

soyuka · 2017-09-14T12:41:15Z

src/Hydra/Serializer/PartialCollectionViewNormalizer.php

+            if ($object instanceof PaginatorInterface) {
+                $paginated = 1. !== $lastPage = $object->getLastPage();
+            } else {
+                $lastPage = null;


same now that it's declared above

soyuka · 2017-09-14T14:17:34Z

Thanks @meyerbaptiste :D

thomasglachant · 2017-09-14T14:19:15Z

Thanks a lot, great job 👍

…formances Add a partial paginator to prevent SQL COUNT queries

meyerbaptiste commented Jul 28, 2017

View reviewed changes

soyuka reviewed Jul 28, 2017

View reviewed changes

meyerbaptiste force-pushed the paginator_performances branch 2 times, most recently from 1219528 to d8af927 Compare July 28, 2017 15:45

meyerbaptiste force-pushed the paginator_performances branch 3 times, most recently from c7bc360 to 3a74628 Compare July 28, 2017 22:03

soyuka reviewed Jul 31, 2017

View reviewed changes

meyerbaptiste force-pushed the paginator_performances branch from 3a74628 to 6a2f4ad Compare July 31, 2017 15:49

soyuka reviewed Jul 31, 2017

View reviewed changes

meyerbaptiste force-pushed the paginator_performances branch from 6a2f4ad to 577535f Compare July 31, 2017 22:07

soyuka approved these changes Aug 1, 2017

View reviewed changes

Simperfit approved these changes Aug 1, 2017

View reviewed changes

meyerbaptiste force-pushed the paginator_performances branch from 577535f to c888e89 Compare August 4, 2017 14:11

meyerbaptiste added the [RFR] Ready for Review label Aug 4, 2017

meyerbaptiste assigned dunglas Aug 4, 2017

meyerbaptiste force-pushed the paginator_performances branch 5 times, most recently from 890521a to a15af93 Compare August 11, 2017 12:31

meyerbaptiste force-pushed the paginator_performances branch from a15af93 to 347ad0e Compare August 21, 2017 12:46

Simperfit requested a review from dunglas August 23, 2017 20:40

meyerbaptiste mentioned this pull request Aug 24, 2017

Is there any plan for supporting cursor based pagination? #1328

Closed

meyerbaptiste force-pushed the paginator_performances branch from 347ad0e to b0c86a8 Compare August 24, 2017 08:57

meyerbaptiste unassigned dunglas Aug 24, 2017

meyerbaptiste force-pushed the paginator_performances branch 2 times, most recently from 724dcf2 to ec5d87a Compare September 12, 2017 14:45

dunglas requested changes Sep 13, 2017

View reviewed changes

meyerbaptiste force-pushed the paginator_performances branch 4 times, most recently from 658a499 to cefd25e Compare September 14, 2017 12:27

soyuka reviewed Sep 14, 2017

View reviewed changes

soyuka approved these changes Sep 14, 2017

View reviewed changes

Add a partial paginator to prevent COUNT SQL queries

9480dc7

meyerbaptiste force-pushed the paginator_performances branch from cefd25e to 9480dc7 Compare September 14, 2017 13:06

meyerbaptiste merged commit f21b165 into api-platform:master Sep 14, 2017

meyerbaptiste deleted the paginator_performances branch September 14, 2017 14:13

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Merge pull request api-platform#1292 from meyerbaptiste/paginator_per…

c397d2f

…formances Add a partial paginator to prevent SQL COUNT queries

Uh oh!

Add a partial paginator to prevent SQL COUNT queries #1292

Add a partial paginator to prevent SQL COUNT queries #1292

Uh oh!

Conversation

meyerbaptiste commented Jul 28, 2017 • edited by dunglas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Jul 28, 2017

Uh oh!

bendavies commented Jul 28, 2017

Uh oh!

soyuka commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bendavies commented Jul 28, 2017

Uh oh!

soyuka commented Jul 28, 2017

Uh oh!

meyerbaptiste commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka left a comment

Choose a reason for hiding this comment

Uh oh!

Simperfit commented Aug 23, 2017

Uh oh!

Simperfit commented Sep 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meyerbaptiste commented Jul 28, 2017 •

edited by dunglas

Loading

soyuka commented Jul 28, 2017 •

edited

Loading

dunglas Sep 14, 2017 •

edited

Loading