Skip to content

Conversation

@juliusknorr
Copy link
Member

Fixes #30421

IConfig methods may issue queries which are causing a full table scan on oc_preferences with using the existing primary key as an index as it is only partially filtered for in the where clause, as the order seems to be relevant (userid,appid,configkey)

public function getUsersForUserValue($appName, $key, $value) {
// TODO - FIXME
$this->fixDIInit();
$sql = 'SELECT `userid` FROM `*PREFIX*preferences` ' .
'WHERE `appid` = ? AND `configkey` = ? ';

public function deleteAppFromAllUsers($appName) {
// TODO - FIXME
$this->fixDIInit();
$sql = 'DELETE FROM `*PREFIX*preferences` '.
'WHERE `appid` = ?';

@juliusknorr juliusknorr added this to the Nextcloud 24 milestone Feb 7, 2022
@juliusknorr juliusknorr requested review from a team, CarlSchwan, icewind1991 and nickvergessen and removed request for a team February 7, 2022 08:57
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81
Copy link
Member

seems the index has introduced ordering issues:

1) Test\AllConfigTest::testGetUsersForUserValue
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'user1'
+    0 => 'user6'
     1 => 'user2'
-    2 => 'user6'
+    2 => 'user1'
 )

/drone/src/tests/lib/AllConfigTest.php:437

2) Test\AllConfigTest::testGetUsersForUserValueCaseInsensitive
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
-    0 => 'user1'
-    1 => 'user2'
+    0 => 'user2'
+    1 => 'user1'
 )

/drone/src/tests/lib/AllConfigTest.php:458

@nickvergessen
Copy link
Member

Postgres needs a sleep after each insert. Otherwise the input order is not the same as it's journaled at the meantime.

Also:

Conflicting files
core/Application.php
core/Command/Db/AddMissingIndices.php

@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
This was referenced Apr 7, 2022
@PVince81
Copy link
Member

@juliushaertl can you finish this at some point, it looks interesting :-)

…without a user filter is fast

Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr juliusknorr force-pushed the enh/preferences-index branch from 476a41b to 7fbd93b Compare April 19, 2022 10:37
@juliusknorr
Copy link
Member Author

Rebased and added an ORDER BY userid statement to the queries in question which is covered by the already used index and gives a fixed result ordering then.

@juliusknorr
Copy link
Member Author

Another quick re-review on the order by statement would be appreciated before hitting merge ;) @nickvergessen @PVince81

@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@juliusknorr juliusknorr merged commit f4135c7 into master Apr 21, 2022
@juliusknorr juliusknorr deleted the enh/preferences-index branch April 21, 2022 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Index improvements on oc_preferences

5 participants