From 76a86c452ebf83d7fd04e2066d8a52e9ddba9c6c Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 12 Feb 2023 13:09:24 +0100 Subject: [PATCH 1/5] Optimize tags list query performance by using more subqueries --- config/autoload/logger.local.php.dist | 8 ++- .../Core/src/Tag/Repository/TagRepository.php | 63 ++++++++++++++----- .../WithInlinedApiKeySpecsEnsuringJoin.php | 2 +- 3 files changed, 52 insertions(+), 21 deletions(-) diff --git a/config/autoload/logger.local.php.dist b/config/autoload/logger.local.php.dist index 7288ed06..919f6cdb 100644 --- a/config/autoload/logger.local.php.dist +++ b/config/autoload/logger.local.php.dist @@ -5,14 +5,16 @@ declare(strict_types=1); use Monolog\Level; use Shlinkio\Shlink\Common\Logger\LoggerType; -$isSwoole = extension_loaded('openswoole'); +use function Shlinkio\Shlink\Config\runningInOpenswoole; + +$logToStream = runningInOpenswoole(); return [ 'logger' => [ 'Shlink' => [ - // For swoole, send logs as stream - 'type' => $isSwoole ? LoggerType::STREAM->value : LoggerType::FILE->value, + // For openswoole, send logs as stream + 'type' => $logToStream ? LoggerType::STREAM->value : LoggerType::FILE->value, 'level' => Level::Debug->value, ], ], diff --git a/module/Core/src/Tag/Repository/TagRepository.php b/module/Core/src/Tag/Repository/TagRepository.php index 5dd9dcd9..825a4249 100644 --- a/module/Core/src/Tag/Repository/TagRepository.php +++ b/module/Core/src/Tag/Repository/TagRepository.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Tag\Repository; +use Doctrine\DBAL\Query\QueryBuilder as NativeQueryBuilder; use Doctrine\ORM\Query\ResultSetMappingBuilder; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Happyr\DoctrineSpecification\Spec; @@ -45,7 +46,6 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $orderDir = $filtering?->orderBy?->direction; $orderMainQuery = $orderField !== null && OrderableField::isAggregateField($orderField); - $conn = $this->getEntityManager()->getConnection(); $subQb = $this->createQueryBuilder('t'); $subQb->select('t.id', 't.name'); @@ -53,15 +53,51 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $subQb->orderBy('t.name', $orderDir ?? 'ASC') ->setMaxResults($filtering?->limit ?? PHP_INT_MAX) ->setFirstResult($filtering?->offset ?? 0); + // TODO Check if applying limit/offset ot visits sub-queries is needed with large amounts of tags } + $conn = $this->getEntityManager()->getConnection(); + $buildVisitsSubQuery = static function (bool $excludeBots, string $aggregateAlias) use ($conn) { + $visitsSubQuery = $conn->createQueryBuilder(); + $commonJoinCondition = $visitsSubQuery->expr()->eq('v.short_url_id', 's.id'); + $visitsJoin = ! $excludeBots + ? $commonJoinCondition + : $visitsSubQuery->expr()->and( + $commonJoinCondition, + $visitsSubQuery->expr()->eq('v.potential_bot', $conn->quote('0')) + ); + + return $visitsSubQuery + ->select('st.tag_id AS tag_id', 'COUNT(DISTINCT v.id) AS ' . $aggregateAlias) + ->from('visits', 'v') + ->join('v', 'short_urls', 's', $visitsJoin) + ->join('s', 'short_urls_in_tags', 'st', $visitsSubQuery->expr()->eq('st.short_url_id', 's.id')) + ->groupBy('st.tag_id'); + }; + $allVisitsSubQuery = $buildVisitsSubQuery(false, 'visits'); + $nonBotVisitsSubQuery = $buildVisitsSubQuery(true, 'non_bot_visits'); + $searchTerm = $filtering?->searchTerm; if ($searchTerm !== null) { $subQb->andWhere($subQb->expr()->like('t.name', $conn->quote('%' . $searchTerm . '%'))); + // TODO Check if applying this to all sub-queries makes it faster or slower } $apiKey = $filtering?->apiKey; + $applyApiKeyToNativeQuery = static fn (?ApiKey $apiKey, NativeQueryBuilder $nativeQueryBuilder) => + $apiKey?->mapRoles(static fn (Role $role, array $meta) => match ($role) { + Role::DOMAIN_SPECIFIC => $nativeQueryBuilder->andWhere( + $nativeQueryBuilder->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))), + ), + Role::AUTHORED_SHORT_URLS => $nativeQueryBuilder->andWhere( + $nativeQueryBuilder->expr()->eq('s.author_api_key_id', $conn->quote($apiKey->getId())), + ), + }); + + // Apply API key specification to all sub-queries $this->applySpecification($subQb, new WithInlinedApiKeySpecsEnsuringJoin($apiKey), 't'); + $applyApiKeyToNativeQuery($apiKey, $allVisitsSubQuery); + $applyApiKeyToNativeQuery($apiKey, $nonBotVisitsSubQuery); // A native query builder needs to be used here, because DQL and ORM query builders do not support // sub-queries at "from" and "join" level. @@ -71,29 +107,22 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito ->select( 't.id_0 AS id', 't.name_1 AS name', + 'v.visits', + 'v2.non_bot_visits', 'COUNT(DISTINCT s.id) AS short_urls_count', - 'COUNT(DISTINCT v.id) AS visits', // Native queries require snake_case for cross-db compatibility - 'COUNT(DISTINCT v2.id) AS non_bot_visits', ) ->from('(' . $subQb->getQuery()->getSQL() . ')', 't') // @phpstan-ignore-line ->leftJoin('t', 'short_urls_in_tags', 'st', $nativeQb->expr()->eq('t.id_0', 'st.tag_id')) ->leftJoin('st', 'short_urls', 's', $nativeQb->expr()->eq('s.id', 'st.short_url_id')) - ->leftJoin('st', 'visits', 'v', $nativeQb->expr()->eq('st.short_url_id', 'v.short_url_id')) - ->leftJoin('st', 'visits', 'v2', $nativeQb->expr()->and( // @phpstan-ignore-line - $nativeQb->expr()->eq('st.short_url_id', 'v2.short_url_id'), - $nativeQb->expr()->eq('v2.potential_bot', $conn->quote('0')), + ->leftJoin('t', '(' . $allVisitsSubQuery->getSQL() . ')', 'v', $nativeQb->expr()->eq('t.id_0', 'v.tag_id')) + ->leftJoin('t', '(' . $nonBotVisitsSubQuery->getSQL() . ')', 'v2', $nativeQb->expr()->eq( + 't.id_0', + 'v2.tag_id', )) - ->groupBy('t.id_0', 't.name_1'); + ->groupBy('t.id_0', 't.name_1', 'v.visits', 'v2.non_bot_visits'); // Apply API key role conditions to the native query too, as they will affect the amounts on the aggregates - $apiKey?->mapRoles(static fn (Role $role, array $meta) => match ($role) { - Role::DOMAIN_SPECIFIC => $nativeQb->andWhere( - $nativeQb->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))), - ), - Role::AUTHORED_SHORT_URLS => $nativeQb->andWhere( - $nativeQb->expr()->eq('s.author_api_key_id', $conn->quote($apiKey->getId())), - ), - }); + $applyApiKeyToNativeQuery($apiKey, $nativeQb); if ($orderMainQuery) { $nativeQb @@ -107,9 +136,9 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito $rsm = new ResultSetMappingBuilder($this->getEntityManager()); $rsm->addScalarResult('name', 'tag'); - $rsm->addScalarResult('short_urls_count', 'shortUrlsCount'); $rsm->addScalarResult('visits', 'visits'); $rsm->addScalarResult('non_bot_visits', 'nonBotVisits'); + $rsm->addScalarResult('short_urls_count', 'shortUrlsCount'); return map( $this->getEntityManager()->createNativeQuery($nativeQb->getSQL(), $rsm)->getResult(), diff --git a/module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php b/module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php index 8e535570..2e84d835 100644 --- a/module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php +++ b/module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php @@ -11,7 +11,7 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; class WithInlinedApiKeySpecsEnsuringJoin extends BaseSpecification { - public function __construct(private ?ApiKey $apiKey, private string $fieldToJoin = 'shortUrls') + public function __construct(private readonly ?ApiKey $apiKey, private readonly string $fieldToJoin = 'shortUrls') { parent::__construct(); } From dd049feb40466da2be78d16839e222f0ef90d7d0 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 12 Feb 2023 13:12:09 +0100 Subject: [PATCH 2/5] Add migration with new index for short_url_id+potential_bot on visits table --- data/migrations/Version20230211171904.php | 27 +++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 data/migrations/Version20230211171904.php diff --git a/data/migrations/Version20230211171904.php b/data/migrations/Version20230211171904.php new file mode 100644 index 00000000..1d1acbf7 --- /dev/null +++ b/data/migrations/Version20230211171904.php @@ -0,0 +1,27 @@ +getTable('visits'); + $this->skipIf($visits->hasIndex(self::INDEX_NAME)); + + $visits->addIndex(['short_url_id', 'potential_bot'], self::INDEX_NAME); + } + + public function isTransactional(): bool + { + return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform); + } +} From 49b60635014e422ac94a9825087b405ddc1676cd Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 12 Feb 2023 13:35:05 +0100 Subject: [PATCH 3/5] Fix ordering on Postgres --- module/Core/src/Tag/Repository/TagRepository.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/module/Core/src/Tag/Repository/TagRepository.php b/module/Core/src/Tag/Repository/TagRepository.php index 825a4249..740a79ae 100644 --- a/module/Core/src/Tag/Repository/TagRepository.php +++ b/module/Core/src/Tag/Repository/TagRepository.php @@ -107,8 +107,8 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito ->select( 't.id_0 AS id', 't.name_1 AS name', - 'v.visits', - 'v2.non_bot_visits', + 'COALESCE(v.visits, 0) AS visits', // COALESCE required for postgres to properly order + 'COALESCE(v2.non_bot_visits, 0) AS non_bot_visits', // COALESCE required for postgres to properly order 'COUNT(DISTINCT s.id) AS short_urls_count', ) ->from('(' . $subQb->getQuery()->getSQL() . ')', 't') // @phpstan-ignore-line From e48d0f4f0ce6c2212715ba12d7bd4141314846f2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 12 Feb 2023 19:07:07 +0100 Subject: [PATCH 4/5] Upgrade deps for MSSQL tests --- data/infra/ci/install-ms-odbc.sh | 4 ++-- module/Core/src/Tag/Repository/TagRepository.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/data/infra/ci/install-ms-odbc.sh b/data/infra/ci/install-ms-odbc.sh index 1efdf8a3..f8226db8 100755 --- a/data/infra/ci/install-ms-odbc.sh +++ b/data/infra/ci/install-ms-odbc.sh @@ -3,7 +3,7 @@ set -ex curl https://packages.microsoft.com/keys/microsoft.asc | apt-key add - -curl https://packages.microsoft.com/config/ubuntu/20.04/prod.list > /etc/apt/sources.list.d/mssql-release.list +curl https://packages.microsoft.com/config/ubuntu/22.04/prod.list > /etc/apt/sources.list.d/mssql-release.list apt-get update -ACCEPT_EULA=Y apt-get install msodbcsql17 +ACCEPT_EULA=Y apt-get install msodbcsql18 apt-get install unixodbc-dev diff --git a/module/Core/src/Tag/Repository/TagRepository.php b/module/Core/src/Tag/Repository/TagRepository.php index 740a79ae..4e9eafac 100644 --- a/module/Core/src/Tag/Repository/TagRepository.php +++ b/module/Core/src/Tag/Repository/TagRepository.php @@ -64,13 +64,13 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito ? $commonJoinCondition : $visitsSubQuery->expr()->and( $commonJoinCondition, - $visitsSubQuery->expr()->eq('v.potential_bot', $conn->quote('0')) + $visitsSubQuery->expr()->eq('v.potential_bot', $conn->quote('0')), ); return $visitsSubQuery ->select('st.tag_id AS tag_id', 'COUNT(DISTINCT v.id) AS ' . $aggregateAlias) ->from('visits', 'v') - ->join('v', 'short_urls', 's', $visitsJoin) + ->join('v', 'short_urls', 's', $visitsJoin) // @phpstan-ignore-line ->join('s', 'short_urls_in_tags', 'st', $visitsSubQuery->expr()->eq('st.short_url_id', 's.id')) ->groupBy('st.tag_id'); }; From 6da8b116749acaa98373720f6b5d244b7d6ad28e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 12 Feb 2023 19:52:22 +0100 Subject: [PATCH 5/5] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5426a069..01c481b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Fixed * [#1698](https://github.com/shlinkio/shlink/issues/1698) Fixed error 500 in `robots.txt`. +* [#1688](https://github.com/shlinkio/shlink/issues/1688) Fixed huge performance degradation on `/tags/stats` endpoint. ## [3.5.1] - 2023-02-04