Skip to content

Conversation

@ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Dec 23, 2021

This is a performance regression of #27426.

All other queries of this class select rows by their propertypath and userid, so the index from #20716 is used. The new query only selects by propertypath, therefore the old index did not apply.

Todo

  • Apply the index during the initial migration that creates the table

How to test

Run EXPLAIN SELECT * FROM `oc_properties` WHERE `propertypath` = 'calendars/admin/inbox'; on mariadb/mysql or the equivalent for any other DB before/after applying the optional index. Before no index used. Afterwards it uses an index.

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 23, 2021
@ChristophWurst ChristophWurst marked this pull request as ready for review December 23, 2021 12:27
@ChristophWurst
Copy link
Member Author

/backport to stable23

@ChristophWurst
Copy link
Member Author

/backport to stable22

@ChristophWurst ChristophWurst changed the title Add missing index for propertypath only queries against properties Add missing index for propertypath only queries of DAV properties Dec 23, 2021
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

Index is created

image

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 23, 2021
@ChristophWurst ChristophWurst force-pushed the fix/missing-properties-propertypath-index branch from 681e52f to ed84f07 Compare December 23, 2021 14:52
@juliusknorr
Copy link
Member

Psalm failure unrelated.

@juliusknorr juliusknorr merged commit 23985b6 into master Dec 28, 2021
@juliusknorr juliusknorr deleted the fix/missing-properties-propertypath-index branch December 28, 2021 12:22
@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

@solracsf
Copy link
Member

Backports failed. 😢

@ChristophWurst
Copy link
Member Author

Weird. It works just fine locally.

@ChristophWurst
Copy link
Member Author

/backport to stable23

@ChristophWurst
Copy link
Member Author

/backport to stable22

@ChristophWurst
Copy link
Member Author

/backport to stable21

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 feature: caldav Related to CalDAV internals feature: carddav Related to CardDAV internals feature: dav performance 🚀 regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants