Repair step to fix orphan reshares #30653

sharidas · 2018-03-01T08:46:34Z

Its being reported that shares are orphaned.
And in such state when an upgrade operation is
performed results in failure. This repair
step fixes the orphan shares.

Signed-off-by: Sujith H [email protected]

Description

This change would help to fix the failures caused during upgrade operation, when orphan reshares are residing in oc_share.

Related Issue

#30199

Motivation and Context

This change would help to fix the failures caused during upgrade operation, when orphan reshares are residing in oc_share. This change tries to remove the orphan shares fro the table which doesn't have parent.

How Has This Been Tested?

Verified by creating the same scenario explained in 8.2.11 upgrade to 10.0.6.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

sharidas · 2018-03-01T08:48:29Z

lib/private/Repair.php

@@ -191,6 +192,7 @@ public static function getBeforeUpgradeRepairSteps() {
 			new InnoDB(),
 			new Collation(\OC::$server->getConfig(), $connection),
 			new SqliteAutoincrement($connection),
+			new RepairOrphanedSubshare($connection),


Added the change here so that we trigger the repair step in the initial stage.

sharidas · 2018-03-01T08:49:21Z

lib/private/Repair.php

@@ -249,7 +251,7 @@ public function advance($step = 1, $description = '') {
 	}

 	/**
-	 * @param int $max
+	 * emit signal


This has nothing to do with the commit. But then it doesn't make sense to have a PHPdoc which is wrong.

sharidas · 2018-03-01T08:56:12Z

lib/private/Repair/RepairOrphanedSubshare.php

+	 * @param $pageLimit
+	 * @param $paginationOffset
+	 */
+	private function setSelectGetAllParents($pageLimit, $paginationOffset) {


The idea with this method is to get all the parents grouped and get the result.

sharidas · 2018-03-01T08:57:09Z

lib/private/Repair/RepairOrphanedSubshare.php

+	 * This select query is to get the result of parents === id
+	 * @param $parentId
+	 */
+	private function setSelectShareId($parentId) {


This query will help us know if we have parents available in the table.

sharidas · 2018-03-01T08:57:40Z

lib/private/Repair/RepairOrphanedSubshare.php

+	 * This delete query deletes orphan shares whose parents are missing
+	 * @param $parentId
+	 */
+	private function setDeleteOrphanSharesQuery($parentId) {


This query will delete the orphan shares.

sharidas · 2018-03-01T08:59:46Z

lib/private/Repair/RepairOrphanedSubshare.php

+				 */
+				if ((count($getIdRows) === 0) &&
+					(in_array($row['parent'], $missingParents, true) === false)) {
+					$missingParents[] = $row['parent'];


If the parents are missing, that is if the share is orphan, then add to $missingParents array.

sharidas · 2018-03-01T09:00:34Z

lib/private/Repair/RepairOrphanedSubshare.php

+			if (count($missingParents) > 0) {
+				foreach ($missingParents as $missingParent) {
+					$this->setDeleteOrphanSharesQuery($missingParent);
+					$deleteRows[] = $this->deleteOrphanReshares;


Delete the rows if the $missingParents array has elements.

codecov · 2018-03-01T09:08:40Z

Codecov Report

Merging #30653 into master will increase coverage by 0.01%.
The diff coverage is 91.17%.

@@             Coverage Diff              @@
##             master   #30653      +/-   ##
============================================
+ Coverage     62.25%   62.27%   +0.01%     
- Complexity    18205    18211       +6     
============================================
  Files          1140     1141       +1     
  Lines         68042    68076      +34     
  Branches       1230     1230              
============================================
+ Hits          42361    42392      +31     
- Misses        25318    25321       +3     
  Partials        363      363

Flag	Coverage Δ	Complexity Δ
#javascript	`51.99% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`63.44% <91.17%> (+0.01%)`	`18211 <6> (+6)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Repair.php	`25.88% <0%> (-0.31%)`	`21 <0> (ø)`
lib/private/Repair/RepairOrphanedSubshare.php	`93.93% <93.93%> (ø)`	`6 <6> (?)`

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 b800886...d3df649. Read the comment docs.

PVince81 · 2018-03-01T09:20:09Z

Why not add your github comments directly in the PHP code, if you think they are needed to understand the code ?

sharidas · 2018-03-02T06:22:36Z

Why not add your github comments directly in the PHP code, if you think they are needed to understand the code ?

Sure

sharidas · 2018-03-12T06:24:52Z

lib/private/Repair/RepairOrphanedSubshare.php

+	 * @param int $pageLimit
+	 * @param int $paginationOffset
+	 */
+	private function setParentsToExcludeFromShare($pageLimit, $paginationOffset) {


This query looks bit expensive to me. Any pointers to reduce the load would be helpful.

PVince81 · 2018-03-12T10:19:48Z

lib/private/Repair/RepairOrphanedSubshare.php

+		$builder = $this->connection->getQueryBuilder();
+		$builder
+			->delete('share')
+			->where($builder->expr()->eq('parent', $builder->createNamedParameter($parentId)));


please build the query without a value and use a real parameter name that you set later

see https://github.com/owncloud/core/blob/v10.0.7/lib/private/SystemTag/SystemTagManager.php#L172

PVince81 · 2018-03-14T09:43:37Z

lib/private/Repair/RepairOrphanedSubshare.php

+			->from('share');
+		$builder->where($builder->expr()->notIn('parent', $builder->createFunction($qb->getSQL())));
+		$builder->groupBy('parent');
+		$builder->setMaxResults($pageLimit);


these need to be set by whatever code is using the query, not in the generic query

PVince81 · 2018-03-14T09:44:04Z

lib/private/Repair/RepairOrphanedSubshare.php

+				$lastCount++;
+				$paginationOffset += $pageLimit;
+				foreach ($rows as $row) {
+					$this->setDeleteOrphanSharesQuery();


no, the query is only created once in the constructor and never again

then the code just sets the parameters

PVince81 · 2018-03-19T10:03:24Z

lib/private/Repair/RepairOrphanedSubshare.php

+			$rows = $results->fetchAll();
+			$results->closeCursor();
+			if (count($rows) > 0) {
+				$lastCount++;


you could simply use empty($rows) in the while condition, no need for this extra variable

additionally $lastCount seems to only ever count from 0 to 1, it doesn't count the results or pages, which could make it confusing for devs having a quick read. Better remove it.

Latest change removes lastCount var and uses empty($rows) in the while condition.

PVince81 · 2018-03-19T10:04:37Z

lib/private/Repair/RepairOrphanedSubshare.php

+				$lastCount++;
+				$paginationOffset += $pageLimit;
+				foreach ($rows as $row) {
+					$deleteParents[] = $row['parent'];


Should we delete the parents right away ?

If we do, then you don't need pagination in the missingParents query as the next iteration will not return the already deleted entries.

This proposed approach would likely save memory in case of lots of shares and avoid having a too long list stored in $deleteParents

The latest change does the deletion right away.

PVince81 · 2018-03-19T10:05:25Z

tests/lib/Repair/RepairOrphanedSubshareTest.php

+		//Total 10 rows where there, we deleted 3 and the repair deleted another
+		//3 rows. So total 6 rows were deleted and 4 rows were remaining
+		//Hence 2 is the expected result.
+		//For pgsql the result comes as 3, because null is included


can you exclude null somehow by adding a "is not null" in the query somehow and make it consistent across DBs ?

PVince81

Looks better. A few more adjustments needed.

Its being reported that shares are orphaned. And in such state when an upgrade operation is performed results in failure. This repair step fixes the orphan shares. Signed-off-by: Sujith H <[email protected]>

PVince81

Looks good 👍

Please backport

PVince81 · 2018-04-03T09:53:41Z

@sharidas I might have merged too quickly :-(

the main query is doing pagination
once you start deleting entries, does it affect the results of the first query ?
for example say there are 2500 broken entries coming from the missingParents query
you get the first 1000, then delete children
if you rerun misisngParents query, it would only return 1500 entries, not 2500, so the pagination is shifting forward despite the already deleted entries
did I understand this correctly?

if yes then you need to remove pagination and only keep querying the first 1000 entries until there is nothing to fix

PVince81 · 2018-04-03T13:41:04Z

backport will need to include:

this PR
Remove pagination from the repair step #30989
and 5446681
and Remove pagination from the repair step #30989

sharidas · 2018-04-04T11:53:42Z

Backport PR: #31004

lock · 2019-07-31T11:02:11Z

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.

sharidas added the 2 - Developing label Mar 1, 2018

sharidas added this to the development milestone Mar 1, 2018

sharidas self-assigned this Mar 1, 2018

sharidas commented Mar 1, 2018

View reviewed changes

sharidas force-pushed the repair-orphaned-reshare branch from 67917b2 to 00fbe49 Compare March 1, 2018 09:08

sharidas force-pushed the repair-orphaned-reshare branch 5 times, most recently from 055e710 to b1b43d1 Compare March 1, 2018 15:00

sharidas force-pushed the repair-orphaned-reshare branch 11 times, most recently from bf04aae to 91c18e9 Compare March 2, 2018 12:11

sharidas removed the 2 - Developing label Mar 9, 2018

sharidas force-pushed the repair-orphaned-reshare branch from 8c99c8c to f04cdd6 Compare March 12, 2018 06:21

sharidas commented Mar 12, 2018

View reviewed changes

sharidas force-pushed the repair-orphaned-reshare branch from f04cdd6 to a1d00b8 Compare March 12, 2018 08:20

PVince81 reviewed Mar 12, 2018

View reviewed changes

sharidas force-pushed the repair-orphaned-reshare branch from a1d00b8 to 3a14dd1 Compare March 12, 2018 10:31

PVince81 reviewed Mar 14, 2018

View reviewed changes

sharidas force-pushed the repair-orphaned-reshare branch from 3a14dd1 to bbe5aaf Compare March 16, 2018 14:58

PVince81 reviewed Mar 19, 2018

View reviewed changes

Repair step to fix orphan reshares

d3df649

Its being reported that shares are orphaned. And in such state when an upgrade operation is performed results in failure. This repair step fixes the orphan shares. Signed-off-by: Sujith H <[email protected]>

sharidas force-pushed the repair-orphaned-reshare branch from bbe5aaf to d3df649 Compare March 20, 2018 10:07

ownclouders added the status/STALE label Mar 28, 2018

sharidas removed the status/STALE label Mar 28, 2018

PVince81 approved these changes Apr 3, 2018

View reviewed changes

PVince81 merged commit 8302243 into master Apr 3, 2018

PVince81 deleted the repair-orphaned-reshare branch April 3, 2018 09:47

phil-davis added the backport-request label Apr 3, 2018

sharidas mentioned this pull request Apr 3, 2018

Remove pagination from the repair step #30989

Merged

9 tasks

sharidas mentioned this pull request Apr 4, 2018

[stable10] Repair step to fix orphan reshares #31004

Merged

9 tasks

butonic mentioned this pull request Apr 5, 2018

fix repair orphaned shares sql and loop condition #31014

Closed

PVince81 removed the backport-request label Apr 10, 2018

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Repair step to fix orphan reshares #30653

Repair step to fix orphan reshares #30653

sharidas commented Mar 1, 2018

sharidas Mar 1, 2018

sharidas Mar 1, 2018

sharidas Mar 1, 2018

sharidas Mar 1, 2018

sharidas Mar 1, 2018

sharidas Mar 1, 2018

sharidas Mar 1, 2018

codecov bot commented Mar 1, 2018 •

edited

Loading

PVince81 commented Mar 1, 2018

sharidas commented Mar 2, 2018

sharidas Mar 12, 2018

PVince81 Mar 12, 2018

PVince81 Mar 14, 2018

PVince81 Mar 14, 2018

PVince81 Mar 19, 2018

sharidas Mar 20, 2018

PVince81 Mar 19, 2018

sharidas Mar 20, 2018

PVince81 Mar 19, 2018

sharidas Mar 20, 2018

PVince81 left a comment

PVince81 left a comment

PVince81 commented Apr 3, 2018

PVince81 commented Apr 3, 2018 •

edited

Loading

sharidas commented Apr 4, 2018

lock bot commented Jul 31, 2019

Repair step to fix orphan reshares #30653

Repair step to fix orphan reshares #30653

Conversation

sharidas commented Mar 1, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

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

codecov bot commented Mar 1, 2018 • edited Loading

Codecov Report

PVince81 commented Mar 1, 2018

sharidas commented Mar 2, 2018

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 left a comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Apr 3, 2018

PVince81 commented Apr 3, 2018 • edited Loading

sharidas commented Apr 4, 2018

lock bot commented Jul 31, 2019

codecov bot commented Mar 1, 2018 •

edited

Loading

PVince81 commented Apr 3, 2018 •

edited

Loading