-
Notifications
You must be signed in to change notification settings - Fork 286
perf: query search results directly #11432
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
base: main
Are you sure you want to change the base?
Conversation
Query the search results (messages) directly instead of querying their ids first by the search query and then doing another query for the actual messages. Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
public function findIdsByQuery(Mailbox $mailbox, SearchQuery $query, string $sortOrder, ?int $limit, ?array $uids = null): array { | ||
return array_map( | ||
static fn (Message $message) => $message->getId(), | ||
$this->findByQueryWithoutRelatedData( | ||
$mailbox, | ||
$query, | ||
$sortOrder, | ||
$limit, | ||
$uids, | ||
), | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential drawback: Overfetching
If only the IDs are required, the whole row is fetched without being required.
This method is currently only used in \OCA\Mail\Service\Sync\SyncService::getDatabaseSyncChanges()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the number of emails shown in the client this will be 20 or more rows to overfetch.
What could we do to prevent this? Duplicate findByQueryWithoutRelatedData (ugly) or pass the columns as argument (even uglier)? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are the two options I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, we factor the body of the method (query logic) out and provide two entry points. This makes sense as the columns to be selected is the only thing changing.
EDIT: Ok, this is not much better than just passing the columns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed the version with the columns-as-argument.
For #10668
Ref #10723 (comment)
Get rid of one SQL query for every call to
MessagesController::index()
(and other consumers ofIMailSearch::findMessages()
).Query the search results (messages) directly instead of querying their ids first by the search filter and then doing another query for the actual messages by their ids.