Skip to content

Perf: Optimize shares loading by eliminating N+1 queries #2293

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.

Sign up for GitHub

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

Open
Koc wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Perf: Optimize shares loading by eliminating N+1 queries #2293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ecb0d5
Perf: Optimize shares loading by eliminating N+1 queries
Koc Jan 31, 2026
98bb04a
Perf: Optimize shares loading by eliminating N+1 queries (code review…
Koc Feb 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions lib/Db/BulkFetchTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

/**
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\Tables\Db;

use OCP\IDBConnection;

/**
* Trait that helps mappers to avoid errors with too many parameters
*/
trait BulkFetchTrait {
private function getChunkSize(int $extraParameters = 0): int {
$maxParameters = match ($this->db->getDatabaseProvider()) {
IDBConnection::PLATFORM_ORACLE => 1000,
default => 65_535,
};
return $maxParameters - $extraParameters;
}
}
35 changes: 24 additions & 11 deletions lib/Db/ShareMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

/** @template-extends QBMapper<Share> */
class ShareMapper extends QBMapper {
use BulkFetchTrait;

protected string $table = 'tables_shares';
protected LoggerInterface $logger;

Expand Down Expand Up @@ -91,23 +93,34 @@ public function findShareForNode(int $nodeId, string $nodeType, string $receiver

/**
* @param string $nodeType
* @param string $receiver
* @param string[]|int[] $receivers
* @param string $userId
* @param string|null $receiverType
*
* @return array
* @return Share[]
*
* @throws Exception
*/
public function findAllSharesFor(string $nodeType, string $receiver, string $userId, ?string $receiverType = 'user'): array {
$qb = $this->db->getQueryBuilder();
$qb->select('*')
->from($this->table)
->where($qb->expr()->eq('receiver', $qb->createNamedParameter($receiver, IQueryBuilder::PARAM_STR)))
->andWhere($qb->expr()->neq('sender', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
->andWhere($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType, IQueryBuilder::PARAM_STR)))
->andWhere($qb->expr()->eq('receiver_type', $qb->createNamedParameter($receiverType, IQueryBuilder::PARAM_STR)));
return $this->findEntities($qb);
public function findAllSharesFor(string $nodeType, array $receivers, string $userId, ?string $receiverType = 'user'): array {
if (!$receivers) {
return [];
}

$chunks = [];
// deduct extra parameters (sender, node type, receiver type)
foreach (array_chunk($receivers, $this->getChunkSize(3)) as $receiversChunk) {
$qb = $this->db->getQueryBuilder();
$qb->select('*')
->from($this->table)
->where($qb->expr()->in('receiver', $qb->createNamedParameter($receiversChunk, IQueryBuilder::PARAM_STR_ARRAY)))
->andWhere($qb->expr()->neq('sender', $qb->createNamedParameter($userId)))
->andWhere($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType)))
->andWhere($qb->expr()->eq('receiver_type', $qb->createNamedParameter($receiverType)));

$chunks[] = $this->findEntities($qb);
}

return array_merge(...$chunks);
}

/**
Expand Down
38 changes: 38 additions & 0 deletions lib/Db/TableMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

/** @template-extends QBMapper<Table> */
class TableMapper extends QBMapper {
use BulkFetchTrait;

protected string $table = 'tables_tables';
protected CappedMemoryCache $cache;
public function __construct(
Expand Down Expand Up @@ -50,6 +52,42 @@ public function find(int $id): Table {
return $this->cache[$cacheKey];
}

/**
* @param int[] $ids
* @return array<int, Table> indexed by table id
*/
public function findMany(array $ids): array {
$missing = [];
$result = [];
foreach ($ids as $id) {
$cacheKey = (string)$id;
if (isset($this->cache[$cacheKey])) {
$result[$id] = $this->cache[$cacheKey];
} else {
$missing[$id] = true;
}
}

if (!$missing) {
return $result;
}

$missing = array_keys($missing);
foreach (array_chunk($missing, $this->getChunkSize()) as $missingChunk) {
$qb = $this->db->getQueryBuilder();
$qb->select('*')
->from($this->table)
->where($qb->expr()->in('id', $qb->createNamedParameter($missingChunk, IQueryBuilder::PARAM_INT_ARRAY)));

foreach ($this->findEntities($qb) as $entity) {
$id = $entity->getId();
$this->cache[(string)$id] = $entity;
$result[$id] = $entity;
}
}
return $result;
}

/**
* @param string|null $userId
* @return Table[]
Expand Down
39 changes: 39 additions & 0 deletions lib/Db/ViewMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

/** @template-extends QBMapper<View> */
class ViewMapper extends QBMapper {
use BulkFetchTrait;

protected string $table = 'tables_views';

protected CappedMemoryCache $cache;
Expand Down Expand Up @@ -50,6 +52,43 @@ public function find(int $id): View {
return $this->cache[$cacheKey];
}

/**
* @param int[] $ids
* @return array<int, View> indexed by view id
*/
public function findMany(array $ids): array {
$missing = [];
$result = [];
foreach ($ids as $id) {
Copy link
Member

@blizzz blizzz Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably more theoretical than not, it must not be more than 1k $ids. On the safe side it would be be wrapped in array_chunk, for instance.

Copy link
Contributor Author

@Koc Koc Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, in theory it's can be possible, see #2242, but limit is 65535 for Postgres. Not sure, that it's real case scenario to have so many views. For rows definitely, but for views or tables... I'm not sure.

If you really want to add array_chunk here - just confirm and I will do it.

Copy link
Member

@blizzz blizzz Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server will complain at 1000 for Oracle (cf. #1061). Smallest possible value wins there.

I am a bit torn about a defensive, more solid approach and little YAGNI. Views is more likely to reach that number. In the regular case, it won't really have much of an impact either. Let's do it with chunking.

Copy link
Contributor Author

@Koc Koc Feb 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added trait to determinate max available parameters for db vendor + chunking for shares/tables/views

Copy link
Member

@blizzz blizzz Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would still run against https://github.com/nextcloud/server/blob/master/lib/private/DB/QueryBuilder/QueryBuilder.php#L225

We avoid special solutions for different DB engines, or move it as much up as possible in the stack, if there has to be some sort of handling. In this case the change would also be better moving up; the risk is that it becomes harder to spot such cases.

Copy link
Contributor Author

@Koc Koc Feb 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mate, sorry, I didn't get what to do here?

A. just hardcode 1000 for all DB engines
B. open separate PR to the server repo (we can wait years in that case)
C. something else?

Copy link
Contributor Author

@Koc Koc Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW there is similar PR from you where you don't care about parameters count

$cacheKey = (string)$id;
if (isset($this->cache[$cacheKey])) {
$result[$id] = $this->cache[$cacheKey];
} else {
$missing[$id] = true;
}
}

if (!$missing) {
return $result;
}

$missing = array_keys($missing);
foreach (array_chunk($missing, $this->getChunkSize()) as $missingChunk) {
$qb = $this->db->getQueryBuilder();
$qb->select('v.*', 't.ownership')
->from($this->table, 'v')
->innerJoin('v', 'tables_tables', 't', 't.id = v.table_id')
->where($qb->expr()->in('v.id', $qb->createNamedParameter($missingChunk, IQueryBuilder::PARAM_INT_ARRAY)));

foreach ($this->findEntities($qb) as $entity) {
$id = $entity->getId();
$this->cache[(string)$id] = $entity;
$result[$id] = $entity;
}
}
return $result;
}

public function delete(Entity $entity): View {
unset($this->cache[(string)$entity->getId()]);
return parent::delete($entity);
Expand Down
1 change: 0 additions & 1 deletion lib/Helper/CircleHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ public function getCircleDisplayName(string $circleId, string $userId): string {
/**
* @param string $userId
* @return Circle[]
* @throws InternalError
*/
public function getUserCircles(string $userId): array {
if (!$this->circlesEnabled) {
Expand Down
69 changes: 28 additions & 41 deletions lib/Service/ShareService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use InvalidArgumentException;
use OC\User\User;
use OCA\Circles\Model\Circle;
use OCA\Tables\AppInfo\Application;
use OCA\Tables\Constants\ShareReceiverType;
use OCA\Tables\Db\Context;
Expand All @@ -39,6 +40,7 @@
use OCP\AppFramework\Db\TTransactional;
use OCP\DB\Exception;
use OCP\IDBConnection;
use OCP\IGroup;
use OCP\IUserManager;
use OCP\Security\ISecureRandom;
use Psr\Log\LoggerInterface;
Expand Down Expand Up @@ -181,7 +183,7 @@ protected function generateShareToken(): ShareToken {

/**
* @param string|null $userId
* @return array
* @return array<int, View> Indexed by view id
* @throws InternalError
*/
public function findViewsSharedWithMe(?string $userId = null): array {
Expand All @@ -190,7 +192,7 @@ public function findViewsSharedWithMe(?string $userId = null): array {

/**
* @param string|null $userId
* @return array
* @return array<int, Table> Indexed by table id
* @throws InternalError
*/
public function findTablesSharedWithMe(?string $userId = null): array {
Expand All @@ -203,52 +205,38 @@ public function findTablesSharedWithMe(?string $userId = null): array {
private function findElementsSharedWithMe(string $elementType = 'table', ?string $userId = null): array {
$userId = $this->permissionsService->preCheckUserId($userId);

$returnArray = [];

$shares = [];
try {
// get all views or tables that are shared with me as user
$elementsSharedWithMe = $this->mapper->findAllSharesFor($elementType, $userId, $userId);
$shares['user'] = $this->mapper->findAllSharesFor($elementType, [$userId], $userId);

// get all views or tables that are shared with me by group
$userGroups = $this->userHelper->getGroupsForUser($userId);
foreach ($userGroups as $userGroup) {
$shares = $this->mapper->findAllSharesFor($elementType, $userGroup->getGid(), $userId, ShareReceiverType::GROUP);
$elementsSharedWithMe = array_merge($elementsSharedWithMe, $shares);
}

// get all views or tables that are shared with me by circle
if ($this->circleHelper->isCirclesEnabled()) {
$userCircles = $this->circleHelper->getUserCircles($userId);
$userGroupIds = array_map(static fn (IGroup $group) => $group->getGid(), $userGroups);
$shares['groups'] = $this->mapper->findAllSharesFor($elementType, $userGroupIds, $userId, ShareReceiverType::GROUP);

foreach ($userCircles as $userCircle) {
$shares = $this->mapper->findAllSharesFor($elementType, $userCircle->getSingleId(), $userId, ShareReceiverType::CIRCLE);
$elementsSharedWithMe = array_merge($elementsSharedWithMe, $shares);
}
}
$userCircles = $this->circleHelper->getUserCircles($userId);
$userCircleIds = array_map(static fn (Circle $circle) => $circle->getSingleId(), $userCircles);
$shares['circles'] = $this->mapper->findAllSharesFor($elementType, $userCircleIds, $userId, ShareReceiverType::CIRCLE);
} catch (Throwable $e) {
throw new InternalError($e->getMessage());
throw new InternalError($e->getMessage(), previous: $e);
}
foreach ($elementsSharedWithMe as $share) {
try {
if ($elementType === 'table') {
$element = $this->tableMapper->find($share->getNodeId());
} elseif ($elementType === 'view') {
$element = $this->viewMapper->find($share->getNodeId());
} else {
throw new InternalError('Cannot find element of type ' . $elementType);
}
// Check if en element with this id is already in the result array
$index = array_search($element->getId(), array_column($returnArray, 'id'));
if (!$index) {
$returnArray[] = $element;
}
} catch (DoesNotExistException|Exception|MultipleObjectsReturnedException $e) {
throw new InternalError($e->getMessage());
}

$elementIds = [];
foreach (array_merge($shares['user'], $shares['groups'], $shares['circles']) as $share) {
/** @var Share $share */
$elementIds[$share->getNodeId()] = true;
}
$elementIds = array_keys($elementIds);

if ($elementType === 'table') {
return $this->tableMapper->findMany($elementIds);
}
return $returnArray;
}

if ($elementType === 'view') {
return $this->viewMapper->findMany($elementIds);
}

throw new InternalError('Cannot find element of type ' . $elementType);
}

/**
* @param int $elementId
Expand Down Expand Up @@ -286,7 +274,6 @@ public function create(int $nodeId, string $nodeType, string $receiver, string $
$this->logger->error($e->getMessage(), ['exception' => $e]);
throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': ' . $e->getMessage());
}

$time = new DateTime();
$item = new Share();
$item->setSender($sender);
Expand Down
14 changes: 4 additions & 10 deletions lib/Service/ViewService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,30 +163,24 @@ public function findSharedViewsWithMe(?string $userId = null): array {
return [];
}

$allViews = [];

$sharedViews = $this->shareService->findViewsSharedWithMe($userId);
foreach ($sharedViews as $sharedView) {
$allViews[$sharedView->getId()] = $sharedView;
}

$contexts = $this->contextService->findAll($userId);
foreach ($contexts as $context) {
$nodes = $context->getNodes();
foreach ($nodes as $node) {
if ($node['node_type'] !== Application::NODE_TYPE_VIEW
|| isset($allViews[$node['node_id']])
|| isset($sharedViews[$node['node_id']])
) {
continue;
}
$allViews[$node['node_id']] = $this->find($node['node_id'], false, $userId);
$sharedViews[$node['node_id']] = $this->find($node['node_id'], false, $userId);
}
}

foreach ($allViews as $view) {
foreach ($sharedViews as $view) {
$this->enhanceView($view, $userId);
}
return array_values($allViews);
return array_values($sharedViews);
}


Expand Down
Loading