Optimize join for sharedWith by moving logic to DB #27436

mrow4a · 2017-03-20T16:46:28Z

In best case it gets rid of doubled queries $shares, $this->shareManager->getSharedWith for both GROUP and USER shares, as in the PR body. Currently, if there are both user and group shares, we need to do 2 queries. With this PR, if we have both group and user shares, we need to do 1 query. If we have only user shares, old and new implementation will do 1 query per user.

Anyways, it is big win, since this query does... 2 Left Joins! These are very expensive operations in DBMS for enterprise scale tables.

Unit tests
Needs to use $this->resolveGroupShares Optimize resolveGroupShare #27434

mention-bot · 2017-03-20T16:46:30Z

@mrow4a, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @DeepDiver1975 and @phisch to be potential reviewers.

PVince81 · 2017-03-21T18:37:38Z

lib/public/Share/IShareProvider.php

@@ -136,6 +136,18 @@ public function getShareById($id, $recipientId = null);
 	 */
 	public function getSharesByPath(Node $path);

+
+	/**
+	 * Get shared with the given user for shares of all available types for this share provider within all nodes


what do you mean with "within" ? do you mean "for all given nodes including their subnodes" ? (and not strictly for the given nodes)

hmm and $node is singular, not plural but your PHPDoc talks about "all nodes".

Please clarify and adjust the parameter name if applicable.

PVince81 · 2017-03-21T18:38:38Z

apps/files_sharing/lib/MountProvider.php

@@ -68,8 +68,9 @@ public function __construct(IConfig $config, IManager $shareManager, ILogger $lo
 	 * @return \OCP\Files\Mount\IMountPoint[]
 	 */
 	public function getMountsForUser(IUser $user, IStorageFactory $storageFactory) {
-		$shares = $this->shareManager->getSharedWith($user->getUID(), \OCP\Share::SHARE_TYPE_USER, null, -1);
-		$shares = array_merge($shares, $this->shareManager->getSharedWith($user->getUID(), \OCP\Share::SHARE_TYPE_GROUP, null, -1));
+		$requiredShareTypes = [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP];


ah, indeed that makes sense

PVince81 · 2017-03-21T18:42:18Z

lib/private/Share20/DefaultShareProvider.php

+		$qb = $this->dbConn->getQueryBuilder();
+		// Create array of sharedWith objects (target user -> $userId or group of which user is a member
+		$user = $this->userManager->get($userId);
+		$allGroups = $this->groupManager->getUserGroups($user, 'sharing');


not sure if passing "sharing" here is correct.

@IljaN could mess up with the guest users ?
Some context: this function is called when retrieving the list of shares for which a SharedMount needs to be setup for the recipient. From my understanding passing "sharing" here risks excluding guest users and those users wouldn't see their received shares mounted.

Needs testing...

ah never mind, it's late... this was copied from the old code that already works https://github.com/owncloud/core/pull/27436/files#diff-83377463cf0d718950be360f7fcabd50L710

PVince81 · 2017-03-21T18:46:14Z

lib/private/Share20/DefaultShareProvider.php

+			);
+
+			$qb->andWhere($qb->expr()->in('share_with', $qb->createNamedParameter(
+				$sharedWithUserOrGroupChunk,


this will boom boom 💥 💥 if there are users and groups with the same names, say a group called "meow" and also a user called "meow" (yes, I've seen such setups before (well, setups with both user and group called the same, not setups with "meow"))

please make sure to add a test for this, test first FTW 😉

Hmm, why, these are two separate shares of different types, isnt it? I will try it out though in unit test, good point!

Yes, they might be different types.

But say if there is a crazy scenario where user "user1" is in the group "meow".
With this query you'd also select the incoming shares for the user "meow" which is wrong.

The way I see the query like: and ((share_type = GROUP and share_with in ($userGroups) or (share_type = USER and share_with = $userId))

Hmm, this will slow down the query, will check if this scenario really does not work with the query I implemented.

Yep, boom booms, will write unit test protecting this case and apply a bit slower query if there are any groups

Yeah, if the user is not member of any groups you could just discard the group filter bit completely.

PVince81 · 2017-03-21T18:49:15Z

Apart from the "meow" issue (see comments), the code looks good overall.

jvillafanez · 2017-03-22T12:59:19Z

lib/private/Share20/DefaultShareProvider.php

+		$user = $this->userManager->get($userId);
+		$allGroups = $this->groupManager->getUserGroups($user, 'sharing');
+
+		$sharedWithUserOrGroup = array_map(function(IGroup $group) { return $group->getGID(); }, $allGroups);


Better rewrite this:

$sharedWithUserOrGroup = array_map(function(IGroup $group) { return $group->getGID(); }, $allGroups);

jvillafanez · 2017-03-22T13:08:46Z

lib/private/Share20/DefaultShareProvider.php

+				->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'))
+				->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id'));
+
+			$qb->where($qb->expr()->in(


any reason to not keep the chained calls? I mean:

$qb->select(....) ->selectAlias(....) ->from(...) ->leftJoin(...) ->leftJoin(...) ->where(....) ->andWhere(...)

unless there is a conditional clause that needs to be inserted in the middle, I think it's better this way. If the condition can be moved to the end without harming the performance, I'd move it. If the conditional clause have to be inserted in the middle, then there is no other choice.

jvillafanez · 2017-03-22T13:16:32Z

apps/federatedfilesharing/lib/FederatedShareProvider.php

+	/**
+	 * @inheritdoc
+	 */
+	public function getAllSharedWith($userId, $shareTypes, $node) {


I think a comment about why $sharetypes is being ignored here is needed. I pass some share types and not only it ignores them, but also uses another type. My first thought is "this is wrong"

jvillafanez · 2017-03-22T13:27:19Z

lib/public/Share/IManager.php

+	 * Filter by $node if provided
+	 * 
+	 * @param string $userId
+	 * @param int[] $shareTypes


This might need a better explanation... If some constants are expected, either list them or point the user to where the constants are.

DeepDiver1975 · 2017-03-23T16:23:25Z

apps/federatedfilesharing/lib/FederatedShareProvider.php

+	 * @inheritdoc
+	 */
+	public function getAllSharedWith($userId, $node) {
+		$this->getSharedWith($userId, self::SHARE_TYPE_REMOTE, $node, -1, 0);


missing return? there is also no unit test on this I assume

DeepDiver1975 · 2017-03-23T16:24:23Z

lib/private/Share20/Manager.php

+	 * @inheritdoc
+	 */
+	public function getAllSharedWith($userId, $shareTypes, $node = null) {
+		$shares = array();


DeepDiver1975 · 2017-03-23T16:25:04Z

lib/public/Share/IManager.php

@@ -90,7 +90,7 @@ public function moveShare(IShare $share, $recipientId);
 	 * Get all shares shared by (initiated) by the provided user for specific node IDs.
 	 *
 	 * @param string $userId
-	 * @param int[] $shareTypes
+	 * @param \OC\Share\Constants[] $shareTypes


OC namespace in an OCP interface? NO

DeepDiver1975 · 2017-03-23T16:25:34Z

lib/public/Share/IManager.php

+	 * 
+	 * @param string $userId
+	 * @param \OC\Share\Constants[] $shareTypes
+	 * @param Node|null $node


add this to the argument declaration

DeepDiver1975 · 2017-03-23T16:26:07Z

lib/public/Share/IShareProvider.php

@@ -94,7 +94,7 @@ public function move(\OCP\Share\IShare $share, $recipient);
 	 * Get all shares by the given user
 	 *
 	 * @param string $userId
-	 * @param int $shareType
+	 * @param \OC\Share\Constants $shareType


👎 you cannot change the definition of a public api

DeepDiver1975 · 2017-03-23T16:27:05Z

tests/lib/Share20/ManagerTest.php

+	}
+
+	public function testGetAllSharedWith() {
+		$user = $this->createMock('\OCP\IUser');


IUser::class

PVince81 · 2017-03-28T07:21:21Z

conflicts and comments to be resolved

mrow4a · 2017-03-28T10:05:44Z

Ready for review if tests are passing.

PVince81 · 2017-03-28T10:24:38Z

lib/private/Share20/DefaultShareProvider.php

+							IQueryBuilder::PARAM_STR_ARRAY
+						))
+					),
+					$qb->expr()->andX(


you could have stored these in variables $usersCondition and $groupCondition to avoid duplicating the code here and in the block below

I changed queries a little and not applicable now

jvillafanez · 2017-03-29T07:45:23Z

lib/private/Share20/DefaultShareProvider.php

+		$groupShares = [];
+
+		// Check if user is member of some groups and chunk them
+		$sharedWithGroup = array_map(function(IGroup $group) { return $group->getGID(); }, $allGroups);


Adjust code style here:

$sharedWithGroup = array_map(function(IGroup $group) { return $group->getGID(); }, $allGroups);

Honestly, I adjusted to the one which is in the "original function getSharedWith" :> Question means which is "code style" :D

jvillafanez · 2017-03-29T07:48:48Z

lib/private/Share20/DefaultShareProvider.php

+
+		for ($chunkNo = 0; $chunkNo < $chunkNoRequired; $chunkNo++) {
+			// Query for user/group shares
+			$qb = $this->dbConn->getQueryBuilder();


Could we move all the DB logic to another function?

jvillafanez · 2017-03-29T07:53:22Z

lib/private/Share20/Manager.php

+			if (!isset($providerIdMap[$providerId])) {
+				$providerIdMap[$providerId] = array();
+			}
+			array_push($providerIdMap[$providerId], $shareType);


I think unless you're pushing several items, $providerIdMap[$providerId][] = $shareType might perform better, and also is consistent with the rest of the code.

mrow4a · 2017-05-23T20:10:50Z

lib/private/Share20/DefaultShareProvider.php

+	 * @param Node|null $node
+	 * @return DB\QueryBuilder\IQueryBuilder $qb
+	 */
+	public function getSharedWithUserQuery($userId, $node) {


Created separate function to create query for user shares

mrow4a · 2017-05-23T20:11:01Z

lib/private/Share20/DefaultShareProvider.php

+	 * @param Node|null $node
+	 * @return DB\QueryBuilder\IQueryBuilder $qb
+	 */
+	public function getSharedWithGroupQuery($groups, $node) {


Created separate function to create query for group shares

mrow4a · 2017-05-23T20:11:20Z

lib/private/Share20/DefaultShareProvider.php

+	 * @param Node|null $node
+	 * @return DB\QueryBuilder\IQueryBuilder $qb
+	 */
+	public function getSharedWithUserGroupQuery($groups, $userId, $node) {


Created separate function to create query for both group and user shares

PVince81 · 2017-05-30T12:07:04Z

lib/private/Share20/DefaultShareProvider.php

+				$groups = $sharedWithGroupChunks[$chunkNo];
+				if ($chunkNo === 0) {
+					// First chunk, so we need to obtain user
+					$qb = $this->getSharedWithUserGroupQuery($groups, $userId, $node);


I don't understand this. Why the first chunk always calls this one function but other chunks the other one ? What happens if there are more than 100 in a chunk ?

What magic guarantees that the first chunk is only about users ?

Either change the code to be more readable or adjust PHPDoc to explain why and avoid future confusion

or don't cheat and use two arrays, one for users and one for groups instead of putting both in one and using the first chunk with a special meaning

The idea here is that if there are both user sharess and group shares, dont do 2 queries, do one as above.

In case of chunks, you only to get user shares only once, you dont need to do that for each chunk, this is why with each next chunk you dont fetch user, but only groups.

are these two scenarios this covered by unit tests / integration tests ? if yes then I'm fine...

mrow4a · 2017-07-27T20:23:25Z

lib/private/Share20/DefaultShareProvider.php

+			$cursor->closeCursor();
+		} else {
+			// There are groups, query both for user and for groups
+			$userSharesRetrieved = false;


@PVince81 is it clearer now?

mrow4a · 2017-07-27T20:23:44Z

@jvillafanez @PVince81 @DeepDiver1975 more ready it wont be :>

jvillafanez · 2017-07-28T06:47:12Z

I have nothing against 👍

PVince81 · 2017-07-28T08:57:40Z

Thanks! Please backport to stable10.

mrow4a · 2017-07-28T09:22:23Z

Never backported in my life, so how to do that properly? I need to patch stable10 with this commit? Cherry-pick ?

PVince81 · 2017-07-28T09:24:58Z

@mrow4a branch off stable10, cherry-pick, retest, then send a PR with target stable10

lock · 2019-08-02T10:01:47Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mrow4a force-pushed the optimize_sharedwith branch from 88ed99a to a20bdf9 Compare March 21, 2017 12:42

mrow4a added the 2 - Developing label Mar 21, 2017

mrow4a force-pushed the optimize_sharedwith branch from a20bdf9 to 12b8b8b Compare March 21, 2017 14:50

mrow4a requested review from DeepDiver1975, jvillafanez and PVince81 March 21, 2017 17:25

mrow4a added 3 - To Review and removed 2 - Developing labels Mar 21, 2017

PVince81 reviewed Mar 21, 2017

View reviewed changes

jvillafanez reviewed Mar 22, 2017

View reviewed changes

mrow4a force-pushed the optimize_sharedwith branch 7 times, most recently from da11834 to 20bc46b Compare March 23, 2017 12:47

DeepDiver1975 requested changes Mar 23, 2017

View reviewed changes

mrow4a force-pushed the optimize_sharedwith branch from 20bc46b to c6d3ed8 Compare March 28, 2017 10:02

PVince81 reviewed Mar 28, 2017

View reviewed changes

mrow4a force-pushed the optimize_sharedwith branch from c6d3ed8 to d92c4eb Compare March 28, 2017 12:10

jvillafanez reviewed Mar 29, 2017

View reviewed changes

mrow4a force-pushed the optimize_sharedwith branch from d92c4eb to 3b93a61 Compare March 29, 2017 21:01

DeepDiver1975 modified the milestones: 10.0.1, 10.1 May 17, 2017

mrow4a force-pushed the optimize_sharedwith branch from 83e4c2b to 3058fd4 Compare May 23, 2017 20:09

mrow4a commented May 23, 2017

View reviewed changes

mrow4a assigned PVince81 May 24, 2017

PVince81 reviewed May 30, 2017

View reviewed changes

Optimize join for sharedWith by moving logic to DB

e6b3a0c

mrow4a force-pushed the optimize_sharedwith branch 2 times, most recently from 75fb6d0 to 8000bab Compare July 27, 2017 10:58

Simplify getSharedWith and getAllSharedWith

9279ed3

mrow4a force-pushed the optimize_sharedwith branch from 8000bab to 9279ed3 Compare July 27, 2017 12:12

mrow4a commented Jul 27, 2017

View reviewed changes

PVince81 merged commit bd2ae81 into master Jul 28, 2017

PVince81 deleted the optimize_sharedwith branch July 28, 2017 08:57

PVince81 added the backport-request label Jul 28, 2017

mrow4a mentioned this pull request Jul 28, 2017

Stable10 optimize1 #28524

Merged

PVince81 removed the backport-request label Aug 15, 2017

DeepDiver1975 modified the milestones: 10.1, development Oct 10, 2017

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Optimize join for sharedWith by moving logic to DB #27436

Optimize join for sharedWith by moving logic to DB #27436

Conversation

mrow4a commented Mar 20, 2017 • edited Loading

mention-bot commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Mar 28, 2017

mrow4a commented Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a commented Jul 27, 2017

jvillafanez commented Jul 28, 2017

PVince81 commented Jul 28, 2017

mrow4a commented Jul 28, 2017

PVince81 commented Jul 28, 2017

lock bot commented Aug 2, 2019

mrow4a commented Mar 20, 2017 •

edited

Loading

mrow4a Mar 22, 2017 •

edited

Loading

mrow4a commented Mar 28, 2017 •

edited

Loading