Skip to content

Performance of WebDAV search requests with many shared files #37061

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

Closed
starypatyk wants to merge 8 commits into from
Closed

Performance of WebDAV search requests with many shared files #37061

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
51b8e53
Use SQL IN operator for single file shares
starypatyk Mar 7, 2023
f4486d9
Code scanning / Psalm
starypatyk Mar 7, 2023
e063d9f
DIagnostic logging
starypatyk Jul 6, 2023
7814687
Alternative approach - refactor generated filter
starypatyk Jul 11, 2023
f649864
Fix incorrect assumption in checkStorageAndPathFilter
starypatyk Jul 12, 2023
5e1e692
Small refactoring and code clean-up
starypatyk Jul 13, 2023
2987c0d
psalm false positive
starypatyk Jul 13, 2023
f86dab9
Remove forgotten debug logging
starypatyk Jul 18, 2023
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
68 changes: 67 additions & 1 deletion lib/private/Files/Cache/QuerySearchHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
use OC\Files\Cache\Wrapper\CacheJail;
use OC\Files\Search\QueryOptimizer\QueryOptimizer;
use OC\Files\Search\SearchBinaryOperator;
use OC\Files\Search\SearchComparison;
use OC\SystemConfig;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\Files\Cache\ICache;
Expand All @@ -36,6 +37,8 @@
use OCP\Files\IRootFolder;
use OCP\Files\Mount\IMountPoint;
use OCP\Files\Search\ISearchBinaryOperator;
use OCP\Files\Search\ISearchComparison;
use OCP\Files\Search\ISearchOperator;
use OCP\Files\Search\ISearchQuery;
use OCP\IDBConnection;
use OCP\IGroupManager;
Expand Down Expand Up @@ -82,11 +85,74 @@ protected function getQueryBuilder() {
);
}

private function isCompareEqual(ISearchOperator $operator, string $fieldName): bool {
return
$operator instanceof ISearchComparison &&
$operator->getType() === ISearchComparison::COMPARE_EQUAL &&
$operator->getField() === $fieldName;
}

private function checkStorageAndPathFilter(ISearchOperator $operator, array &$storageToPathsMap, array &$storageOtherFilters): void {
if ($operator instanceof ISearchBinaryOperator && $operator->getType() === ISearchBinaryOperator::OPERATOR_AND && count($operator->getArguments()) == 2) {
$a = $operator->getArguments()[0];
$b = $operator->getArguments()[1];
if ($this->isCompareEqual($a, "storage") && $this->isCompareEqual($b, "path")) {
/** @psalm-suppress UndefinedInterfaceMethod */
$storage = $a->getValue();
/** @psalm-suppress UndefinedInterfaceMethod */
$path = $b->getValue();
\OC::$server->getLogger()->debug("QuerySearchHelper::checkStorageAndPathFilter: storage=" . $storage . " " . "path=" . $path);
Copy link
Contributor

@come-nc come-nc Jul 17, 2023

Choose a reason for hiding this comment

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

Suggested change
\OC::$server->getLogger()->debug("QuerySearchHelper::checkStorageAndPathFilter: storage=" . $storage . " " . "path=" . $path);
$this->logger->debug("QuerySearchHelper::checkStorageAndPathFilter: storage=" . $storage . " " . "path=" . $path);

Copy link
Contributor Author

@starypatyk starypatyk Jul 18, 2023

Choose a reason for hiding this comment

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

I forgot to remove the logging. This was useful during development, quite unlikely to be useful in the future. Removed in f86dab9

$storageToPathsMap[$storage][] = $path;
return;
}
}
$storageOtherFilters[] = $operator;
}

private function optimizeStorageFilters(array $storageFilters): array {
//
// Optimize the storage filters query, when there are many shared files.
//
// Originally for each shared file the following section is added to the SQL WHERE clause:
//
// (`storage` = <storage-id>) AND (`path` = <file-path>)
//
// When many files are shared between the same two users, the storage part of the filter is repeated many times.
//
// Here we want to refactor the query to have a single filter for each storage
// and provide all `path_hash` values for the same storage in the IN clause:
//
// (`storage` = <storage-id>) AND (`path_hash` IN (<path-hash-1>, <path-hash-2>, ...))

// Pick up single file shares to prepare more efficient query
$storageToPathsMap = [];
$storageOtherFilters = [];
foreach ($storageFilters as $storageFilter) {
$this->checkStorageAndPathFilter($storageFilter, $storageToPathsMap, $storageOtherFilters);
}

// Create filters for single file shares
$singleFileFilters = [];
foreach ($storageToPathsMap as $storage => $paths) {
\OC::$server->getLogger()->debug("QuerySearchHelper::optimizeStorageFilters: storage=" . $storage . " " . "paths=" . implode(", ", $paths));
Copy link
Contributor

@come-nc come-nc Jul 17, 2023

Choose a reason for hiding this comment

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

Suggested change
\OC::$server->getLogger()->debug("QuerySearchHelper::optimizeStorageFilters: storage=" . $storage . " " . "paths=" . implode(", ", $paths));
$this->logger->debug("QuerySearchHelper::optimizeStorageFilters: storage=" . $storage . " " . "paths=" . implode(", ", $paths));

Copy link
Contributor Author

@starypatyk starypatyk Jul 18, 2023

Choose a reason for hiding this comment

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

I forgot to remove the logging. This was useful during development, quite unlikely to be useful in the future. Removed in f86dab9

$singleFileFilters[] = new SearchBinaryOperator(
ISearchBinaryOperator::OPERATOR_AND,
[
new SearchComparison(ISearchComparison::COMPARE_EQUAL, 'storage', $storage),
new SearchComparison(ISearchComparison::COMPARE_IN, 'path_hash', array_map(fn ($path) => md5($path), $paths))
Copy link
Member

@AndyScherzinger AndyScherzinger Jun 8, 2023

Choose a reason for hiding this comment

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

Be careful with "IN" it'll break i.e. on Oracle for 1000+ items.

Copy link
Contributor Author

@starypatyk starypatyk Jun 8, 2023

Choose a reason for hiding this comment

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

Indeed. Would it make sense to provide a workaround at a lower level?

Rewrite the query with a sequence of ((x IN (list1-1000)) or (x IN (list1001-2000)) ...) e.g. here:

server/lib/private/DB/QueryBuilder/ExpressionBuilder/OCIExpressionBuilder.php

Line 121 in ca9d6db

public function in($x, $y, $type = null): string {

I thought this might be resolved already by Doctrine DBAL, but apparently a similar workaround has been rejected. 😞

Copy link
Member

@AndyScherzinger AndyScherzinger Jun 8, 2023

Choose a reason for hiding this comment

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

I can't tell since I am not an expert on php/doctrine, but @nickvergessen @juliushaertl or @icewind1991 might know.

Copy link
Contributor

@kesselb kesselb Aug 31, 2023

Choose a reason for hiding this comment

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

We are using (IN(1-1000) OR IN(1001-2000)) in mail: https://github.com/nextcloud/mail/blob/0da32c97f64e0cdf950368257447ec8fa6fe7fea/lib/Db/MessageMapper.php#L986-L992.

Not a big fan, but there is no other way if you want to work with all IDs in one result.

]
);
}

return array_merge($storageOtherFilters, $singleFileFilters);
}

protected function applySearchConstraints(CacheQueryBuilder $query, ISearchQuery $searchQuery, array $caches): void {
$storageFilters = array_values(array_map(function (ICache $cache) {
return $cache->getQueryFilterForStorage();
}, $caches));
$storageFilter = new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, $storageFilters);
$optimizedStorageFilters = $this->optimizeStorageFilters($storageFilters);
$storageFilter = new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_OR, $optimizedStorageFilters);
$filter = new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [$searchQuery->getSearchOperation(), $storageFilter]);
$this->queryOptimizer->processOperator($filter);

Expand Down
7 changes: 6 additions & 1 deletion lib/private/Files/Cache/SearchBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class SearchBuilder {
ISearchComparison::COMPARE_GREATER_THAN_EQUAL => 'gte',
ISearchComparison::COMPARE_LESS_THAN => 'lt',
ISearchComparison::COMPARE_LESS_THAN_EQUAL => 'lte',
ISearchComparison::COMPARE_IN => 'in',
];

protected static $searchOperatorNegativeMap = [
Expand Down Expand Up @@ -187,6 +188,7 @@ private function validateComparison(ISearchComparison $operator) {
'favorite' => 'boolean',
'fileid' => 'integer',
'storage' => 'integer',
'path_hash' => 'array',
];
$comparisons = [
'mimetype' => ['eq', 'like'],
Expand All @@ -199,6 +201,7 @@ private function validateComparison(ISearchComparison $operator) {
'favorite' => ['eq'],
'fileid' => ['eq'],
'storage' => ['eq'],
'path_hash' => ['in'],
];

if (!isset($types[$operator->getField()])) {
Expand All @@ -217,7 +220,9 @@ private function getParameterForValue(IQueryBuilder $builder, $value) {
if ($value instanceof \DateTime) {
$value = $value->getTimestamp();
}
if (is_numeric($value)) {
if (is_array($value)) {
$type = IQueryBuilder::PARAM_STR_ARRAY;
} elseif (is_numeric($value)) {
$type = IQueryBuilder::PARAM_INT;
} else {
$type = IQueryBuilder::PARAM_STR;
Expand Down
6 changes: 3 additions & 3 deletions lib/private/Files/Search/SearchComparison.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class SearchComparison implements ISearchComparison {
private $type;
/** @var string */
private $field;
/** @var string|integer|\DateTime */
/** @var string|integer|\DateTime|array */
private $value;
private $hints = [];

Expand All @@ -38,7 +38,7 @@ class SearchComparison implements ISearchComparison {
*
* @param string $type
* @param string $field
* @param \DateTime|int|string $value
* @param \DateTime|int|string|array $value
*/
public function __construct($type, $field, $value) {
$this->type = $type;
Expand All @@ -61,7 +61,7 @@ public function getField() {
}

/**
* @return \DateTime|int|string
* @return \DateTime|int|string|array
*/
public function getValue() {
return $this->value;
Expand Down
3 changes: 2 additions & 1 deletion lib/public/Files/Search/ISearchComparison.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ interface ISearchComparison extends ISearchOperator {
public const COMPARE_LESS_THAN_EQUAL = 'lte';
public const COMPARE_LIKE = 'like';
public const COMPARE_LIKE_CASE_SENSITIVE = 'clike';
public const COMPARE_IN = 'in';

public const HINT_PATH_EQ_HASH = 'path_eq_hash'; // transform `path = "$path"` into `path_hash = md5("$path")`, on by default

Expand All @@ -58,7 +59,7 @@ public function getField();
/**
* Get the value to compare the field with
*
* @return string|integer|\DateTime
* @return string|integer|\DateTime|array
* @since 12.0.0
*/
public function getValue();
Expand Down