Skip to content

Create new DB index and add new condition #39017

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

Jump to bottom
Merged
merged 5 commits into from
Oct 11, 2021
Merged

Create new DB index and add new condition #39017

merged 5 commits into from
Oct 11, 2021

Conversation

jvillafanez
Copy link
Member

Description

speed up searches in the addressbook in large setups

Related Issue

https://github.com/owncloud/enterprise/issues/4655

Motivation and Context

How Has This Been Tested?

Manually tested with 2 federated servers with around 1000 users each.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@jvillafanez jvillafanez self-assigned this Jul 16, 2021
@update-docs
Copy link

update-docs bot commented Jul 16, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiFederationToShares1-git-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31350/113/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiFederationToShares1-10.6.0-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31354/115/1

@jvillafanez jvillafanez force-pushed the carddav_improvements branch from 05b057b to 5b5fb46 Compare July 20, 2021 06:53
@jvillafanez jvillafanez marked this pull request as ready for review July 20, 2021 08:07
@JammingBen
Copy link
Contributor

LGTM code-wise.

SonarCloud complains about the code coverage though because the migration file is not tested. IMO migration files should not be unit-tested, but I've seen some (core-) apps do it...

@jvillafanez jvillafanez force-pushed the carddav_improvements branch from 5b5fb46 to d52d3d4 Compare August 9, 2021 08:44
@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiFederationToShares1-latest-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31792/115/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiFederationToShares1-git-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31792/114/1

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 9, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

16.7% 16.7% Coverage
0.0% 0.0% Duplication

@mrow4a
Copy link
Contributor

mrow4a commented Aug 16, 2021

@jvillafanez isnt there already one #37152 ?

@jvillafanez
Copy link
Member Author

The index isn't being used properly in that particular query. You can check some research in https://github.com/owncloud/enterprise/issues/4655#issuecomment-878239622

The checked performance is described in https://github.com/owncloud/enterprise/issues/4655#issuecomment-880052549

@mrow4a
Copy link
Contributor

mrow4a commented Aug 30, 2021

@jvillafanez in fact I proposed the index you have in this PR long time ago (#30195 (comment)) but it was rejected at a time , with prefered one also on value. I guess then we need both..

@mrow4a mrow4a self-requested a review August 30, 2021 07:53
@jvillafanez
Copy link
Member Author

Definitely we should have a usable index on the value, but the problems are the case insensitive search and the substring search. I don't think we can do much more without restricting the query, which would remove part of the current functionality, or adding new tables oriented to index this one.

@jvillafanez jvillafanez force-pushed the carddav_improvements branch from d52d3d4 to 0d83d28 Compare October 8, 2021 08:39
@jvillafanez
Copy link
Member Author

Rebased and tests added. The migration has been moved to the dav app.

AlexAndBear
AlexAndBear suggested changes Oct 8, 2021
View reviewed changes
@AlexAndBear AlexAndBear self-requested a review October 8, 2021 12:12
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 8, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@JammingBen JammingBen merged commit 72cf325 into master Oct 11, 2021
@delete-merged-branch delete-merged-branch bot deleted the carddav_improvements branch October 11, 2021 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrow4a mrow4a mrow4a approved these changes

@AlexAndBear AlexAndBear AlexAndBear approved these changes

@JammingBen JammingBen JammingBen approved these changes

Assignees

@jvillafanez jvillafanez

Labels
3 - To Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jvillafanez @ownclouders @JammingBen @mrow4a @AlexAndBear