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.

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

Add index to properties table #20716

Merged
merged 1 commit into from
May 1, 2020
Merged

Add index to properties table #20716

merged 1 commit into from
May 1, 2020

Conversation

mario
Copy link
Contributor

@mario mario commented Apr 29, 2020

Signed-off-by: Mario Danic mario@lovelyhq.com

@mario mario marked this pull request as draft April 29, 2020 12:12
@mario mario requested review from icewind1991 and rullzer April 29, 2020 12:12
@mario mario force-pushed the index-propertypath branch from 8cca823 to 4946e7c Compare April 29, 2020 13:13
@mario mario marked this pull request as ready for review April 29, 2020 13:27
@mario mario added the 3. to review Waiting for reviews label Apr 29, 2020
@mario
Copy link
Contributor Author

mario commented Apr 29, 2020

Would be awesome to get this in 19.

@mario mario added this to the Nextcloud 19 milestone Apr 29, 2020
@rullzer rullzer mentioned this pull request Apr 29, 2020
Merged
11 tasks
@mario
Copy link
Contributor Author

mario commented Apr 29, 2020

This speeds up access to folders with lots of files considerably.

@mario
Copy link
Contributor Author

mario commented Apr 29, 2020

I'm pretty sure the odd test failure is not related to me adding indexes.

@icewind1991
Copy link
Member

The autoloaders are not up to date
Please run: bash build/autoloaderchecker.sh
And commit the result

is yours

@mario mario force-pushed the index-propertypath branch from 4946e7c to 2db36ad Compare April 29, 2020 20:15
@mario
Copy link
Contributor Author

mario commented Apr 29, 2020

The autoloaders are not up to date
Please run: bash build/autoloaderchecker.sh
And commit the result

is yours

Thanks.

gary-kim
gary-kim previously requested changes Apr 29, 2020
View reviewed changes
Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

I think you may have accidentally committed composer.phar?

@mario mario force-pushed the index-propertypath branch from 2db36ad to 9e2aecb Compare April 29, 2020 22:13
@mario
Copy link
Contributor Author

mario commented Apr 29, 2020

I think you may have accidentally committed composer.phar?

Good catch, fixed.

@gary-kim gary-kim dismissed their stale review April 29, 2020 22:25

Addressed

MorrisJobke
MorrisJobke previously requested changes Apr 30, 2020
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Please add to the check for the missing indices so that admins are notified about this:

server/core/Application.php

Line 74 in cb05782

$eventDispatcher->addListener(IDBConnection::CHECK_MISSING_INDEXES_EVENT,

@mario
Copy link
Contributor Author

mario commented Apr 30, 2020

Please add to the check for the missing indices so that admins are notified about this:

server/core/Application.php

Line 74 in cb05782

$eventDispatcher->addListener(IDBConnection::CHECK_MISSING_INDEXES_EVENT,

Fix pushed.

@mario mario requested review from MorrisJobke and gary-kim April 30, 2020 12:24
@mario mario force-pushed the index-propertypath branch from bad5a14 to 4243d5d Compare April 30, 2020 12:36
@mario
Copy link
Contributor Author

mario commented Apr 30, 2020

So lint is complaining about lib/private/Files/ObjectStore/Swift.php which I didn't touch and I can't run composer run cs:fix because it errors out with:

Fatal error: Uncaught Error: Class 'Nextcloud\CodingStandard\Config' not found in /Users/mario/Projects/server/.php_cs.dist:9

@MorrisJobke
Copy link
Member

So lint is complaining about lib/private/Files/ObjectStore/Swift.php which I didn't touch and I can't run composer run cs:fix because it errors out with:

It is already fixed in #20742

@mario mario force-pushed the index-propertypath branch from 4243d5d to b94240c Compare April 30, 2020 13:03
MorrisJobke
MorrisJobke reviewed Apr 30, 2020
View reviewed changes
core/Application.php Outdated Show resolved Hide resolved
@mario mario force-pushed the index-propertypath branch from d8cc68d to f282975 Compare April 30, 2020 13:06
@mario
Copy link
Contributor Author

mario commented Apr 30, 2020

@MorrisJobke thanks, fixed. Thanks to you and @icewind1991 for mentoring, and @gary-kim and @ChristophWurst of course! :)

@mario mario requested a review from MorrisJobke April 30, 2020 13:18
@rullzer rullzer mentioned this pull request Apr 30, 2020
Merged
2 tasks
MorrisJobke
MorrisJobke reviewed Apr 30, 2020
View reviewed changes
core/Migrations/Version19000Date20200429140134.php Outdated
use OCP\Migration\IOutput;
use OCP\Migration\SimpleMigrationStep;

class Version19000Date20200429140134 extends SimpleMigrationStep {
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that you want to do this in a migration. Then the step in the "add-missing-indices" is not needed. I guess we should not do the migration here (because it can take really long and thus break the update). Better is to put the addIndex() to an old migration, so that it is executed on new installs. And for existing installations we do the approach with the admin notification and the add-missing-indices command to add the index while the instance is online.

Copy link
Member

Choose a reason for hiding this comment

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

@mario I just noticed that I had this as pending open. That's why I said that in the chat back then ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What migration do you want me to put it in @MorrisJobke ?

Copy link
Member

Choose a reason for hiding this comment

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

Right below that line I would say:

server/core/Migrations/Version13000Date20170718121200.php

Line 291 in cb05782

$table->setPrimaryKey(['userid', 'appid', 'configkey']);
(it's the only migration for preferences in core)

Copy link
Member

Choose a reason for hiding this comment

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

Just for reference - that's how we did it for other indices as well: https://github.com/nextcloud/server/pull/13213/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You just confused the hell out of me - I hope you meant few lines further where the migration for properties is, and not preferences? :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for preferences, I need it for properties :P But found it, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes - sorry. Far better now 👍

@mario
Add index to properties table
6e28c28 
Signed-off-by: Mario Danic <mario@lovelyhq.com>
@mario
Copy link
Contributor Author

mario commented Apr 30, 2020

Let's hope its good now :p

@mario mario force-pushed the index-propertypath branch from f282975 to 6e28c28 Compare April 30, 2020 21:05
@mario mario changed the title Add index to oc_properties Add index to properties table Apr 30, 2020
@rullzer rullzer merged commit 80372a3 into master May 1, 2020
@rullzer rullzer deleted the index-propertypath branch May 1, 2020 10:39
This was referenced May 4, 2020
Closed
Closed
@dennisTGC
Copy link

Will this be back-ported to a minor v18 release?

@ChristophWurst ChristophWurst mentioned this pull request Dec 23, 2021
Merged
1 task
@solracsf solracsf mentioned this pull request Oct 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke left review comments

@icewind1991 icewind1991 icewind1991 approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@rullzer rullzer Awaiting requested review from rullzer

@gary-kim gary-kim Awaiting requested review from gary-kim

Assignees
No one assigned
Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 19
Development

Successfully merging this pull request may close these issues.
7 participants
@mario @icewind1991 @MorrisJobke @dennisTGC @ChristophWurst @gary-kim @rullzer