Skip to content
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

Use paginated search for contacts #18816

Merged
merged 1 commit into from
Apr 23, 2020
Merged

Conversation

juliushaertl
Copy link
Member

This PR drastically improves the performance of searching for sharees when the system addressbook contains a lot of users. Without it all matching contact entries would be queried from the database for both SHARE_TYPE_REMOTE and SHARE_TYPE_EMAIL which led to quite bad performance when searching with queries that matched a lot of entries (1-3 characters)

@juliushaertl juliushaertl added bug 3. to review Waiting for reviews labels Jan 10, 2020
@juliushaertl juliushaertl added this to the Nextcloud 19 milestone Jan 10, 2020
@juliushaertl juliushaertl force-pushed the bugfix/noid/paginate-contacts-search branch from 6f5ba0d to c41a99b Compare January 10, 2020 15:24
@@ -68,8 +68,7 @@ public function search($search, $limit, $offset, ISearchResult $searchResult) {
$resultType = new SearchResultType('remotes');

// Search in contacts
//@todo Pagination missing
$addressBookContacts = $this->contactsManager->search($search, ['CLOUD', 'FN']);
$addressBookContacts = $this->contactsManager->search($search, ['CLOUD', 'FN'], ['limit' => $limit, 'offset' => $offset]);
Copy link
Member

Choose a reason for hiding this comment

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

The problem is, if you e.g. match the local domain, you will get all system address book entries.
But they are skipped with the first if in the loop, but also no further contacts will be found, although you would have matches.

Copy link
Member Author

Choose a reason for hiding this comment

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

Contacts from other addressbooks should still be found, since the search is executed with the limit on each addressbook individually: https://github.com/nextcloud/server/pull/18816/files#diff-905974f5907f69fe7586a80ea67117a6L48

Copy link
Member Author

Choose a reason for hiding this comment

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

Stil concerns @nickvergessen ?

Copy link
Member

Choose a reason for hiding this comment

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

Lets try it

This was referenced Apr 4, 2020
@nickvergessen nickvergessen requested review from nickvergessen and removed request for nickvergessen April 15, 2020 10:34
@nickvergessen nickvergessen added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 15, 2020
@nickvergessen
Copy link
Member

Needs a rebase

This was referenced Apr 15, 2020
@MorrisJobke MorrisJobke force-pushed the bugfix/noid/paginate-contacts-search branch from 043c013 to 8fd2031 Compare April 17, 2020 07:09
@MorrisJobke
Copy link
Member

Needs a rebase

Done.

@blizzz
Copy link
Member

blizzz commented Apr 17, 2020

CI looks quite red

@rullzer rullzer mentioned this pull request Apr 23, 2020
11 tasks
@juliushaertl juliushaertl force-pushed the bugfix/noid/paginate-contacts-search branch from 8fd2031 to 17104b5 Compare April 23, 2020 14:27
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl juliushaertl force-pushed the bugfix/noid/paginate-contacts-search branch from 17104b5 to 6709833 Compare April 23, 2020 17:08
@juliushaertl
Copy link
Member Author

Aw, isset is no function so using \isset was failing 🙈

@blizzz blizzz merged commit 038045d into master Apr 23, 2020
@blizzz blizzz deleted the bugfix/noid/paginate-contacts-search branch April 23, 2020 20:21
@nickvergessen
Copy link
Member

An exception occurred while executing "SELECT c.carddata, c.uri FROM oc_cards c WHERE c.id IN (SELECT DISTINCT cp.cardid FROM oc_cards_properties cp WHERE (cp.addressbookid = ?) AND ((cp.name = ?) OR (cp.name = ?)) AND (cp.value COLLATE utf8mb4_general_ci LIKE ?) LIMIT 10)" with params ["1", "EMAIL", "FN", "%tes%"]:\n\nSQLSTATE[42000]: Syntax error or access violation: 1235 This version of MySQL doesn"t yet support "LIMIT & IN/ALL/ANY/SOME subquery"

:(

@J0WI
Copy link
Contributor

J0WI commented Apr 29, 2020

current syntax
SELECT `c`.`carddata`, `c`.`uri`
FROM `oc_cards` `c`
WHERE `c`.`id` IN (
	SELECT DISTINCT `cp`.`cardid`
	FROM `oc_cards_properties` `cp`
	WHERE (`cp`.`addressbookid` = ?)
	AND ((`cp`.`name` = ?)
	OR (`cp`.`name` = ?))
	AND (`cp`.`value`  COLLATE utf8mb4_general_ci LIKE ?)
	LIMIT 10
);
@nickvergessen AFAIK you can just use an additional dummy subquery as a workaround. Can you try something like:
SELECT `c`.`carddata`, `c`.`uri`
FROM `oc_cards` `c`
WHERE `c`.`id` IN (
	SELECT `cardid` FROM (
		SELECT DISTINCT `cp`.`cardid`
		FROM `oc_cards_properties` `cp`
		WHERE (`cp`.`addressbookid` = ?)
		AND ((`cp`.`name` = ?)
		OR (`cp`.`name` = ?))
		AND (`cp`.`value`  COLLATE utf8mb4_general_ci LIKE ?)
		LIMIT 10
	)
	AS compatlayer
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug performance 🚀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants