Skip to content

Add index on addressbookid - #37152

Merged
micbar merged 2 commits into
masterfrom
index-for-adressbooks
Mar 28, 2020
Merged

Add index on addressbookid#37152
micbar merged 2 commits into
masterfrom
index-for-adressbooks

Conversation

@mrow4a

@mrow4a mrow4a commented Mar 21, 2020

Copy link
Copy Markdown
Contributor

Fixes owncloud/enterprise#3625. The work should add index for 'addressbookid' 'name' 'value' that allows to improve scan performance of search addressbook query when e.g. end of term wildcard (term%) is used.

Please note combinations that allow to fully utilize index according to #36225, and comment explaining why index cannot be utilized in these cases https://github.com/owncloud/enterprise/issues/3625#issuecomment-573861541:

  • accounts.enable_medial_search=true - %value% - index ignored
  • accounts.enable_medial_search=false - value% - index used

However, query with this index should be faster in general too regardles of wildcard on value, due to index on 'addressbookid', 'name', that will prefilter search

Targeting 10.5 (@micbar )

@mrow4a mrow4a self-assigned this Mar 21, 2020
@update-docs

update-docs Bot commented Mar 21, 2020

Copy link
Copy Markdown

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.

@codecov

codecov Bot commented Mar 21, 2020

Copy link
Copy Markdown

Codecov Report

Merging #37152 into master will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37152   +/-   ##
=========================================
  Coverage     64.85%   64.85%           
- Complexity    19136    19139    +3     
=========================================
  Files          1267     1268    +1     
  Lines         74895    74902    +7     
  Branches       1331     1331           
=========================================
+ Hits          48575    48581    +6     
- Misses        25928    25929    +1     
  Partials        392      392           
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 66.05% <0.00%> (+<0.01%) 19139.00 <3.00> (+3.00)
Impacted Files Coverage Δ Complexity Δ
...s/dav/appinfo/Migrations/Version20200114181454.php 0.00% <0.00%> (ø) 3.00 <3.00> (?)
apps/dav/lib/CardDAV/CardDavBackend.php 83.90% <0.00%> (+1.36%) 85.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cacea1...e43325c. Read the comment docs.

@mrow4a
mrow4a force-pushed the index-for-adressbooks branch from 0226e16 to e43325c Compare March 21, 2020 08:41
@mrow4a

mrow4a commented Mar 21, 2020

Copy link
Copy Markdown
Contributor Author

added some unit tests just to be sure

@jvillafanez jvillafanez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code looks good

@mrow4a

mrow4a commented Mar 23, 2020

Copy link
Copy Markdown
Contributor Author

@micbar shall we merge as part of 10.5?

@micbar
micbar merged commit 1c9d8e5 into master Mar 28, 2020
@delete-merged-branch
delete-merged-branch Bot deleted the index-for-adressbooks branch March 28, 2020 21:19
@phil-davis

phil-davis commented Mar 29, 2020

Copy link
Copy Markdown
Contributor

Note: this is (correctly) after the release-10.4.1 branch point - good.
"some time soon" we should update version.php to say 10.5.0 - but doing that while 10.4.1 branch is still in progress will cause a merge conflict for that branch. So it might be easiest to wait for now.

I raised issue #37178

@mrow4a

mrow4a commented Jun 9, 2020

Copy link
Copy Markdown
Contributor Author

@pmaier1 migration complexity -> we need to build index on oc_cards_properties for all rows and columns 'addressbookid', 'name', 'value' . This can be quite long operation on tables with a lot of entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment