Skip to content

Add maintenance:repair step for file size of unencrypted files #36220

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

Open
simonspa wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add maintenance:repair step for file size of unencrypted files #36220

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2421404
Add repair step for unencrypted_size
Jan 18, 2023
c9b6d5e
Register new repair step
Jan 18, 2023
8a03a1d
Fix psalm complaints and add log
Jan 18, 2023
768436c
Satisfy linter :)
Jan 18, 2023
6856c38
Only touch problematic rows
simonspa Jan 18, 2023
112bbd2
Fix query
Jan 18, 2023
43da5b0
RepairStep for unencrypted size: make DB query more efficient
Jan 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/private/Repair.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
use OC\Repair\RepairDavShares;
use OC\Repair\RepairInvalidShares;
use OC\Repair\RepairMimeTypes;
use OC\Repair\RepairUnencryptedSize;
use OC\Repair\SqliteAutoincrement;
use OC\Template\JSCombiner;
use Psr\Log\LoggerInterface;
Expand Down Expand Up @@ -176,6 +177,7 @@ public static function getRepairSteps(): array {
return [
new Collation(\OC::$server->getConfig(), \OC::$server->get(LoggerInterface::class), \OC::$server->getDatabaseConnection(), false),
new RepairMimeTypes(\OC::$server->getConfig(), \OC::$server->getDatabaseConnection()),
new RepairUnencryptedSize(\OC::$server->getDatabaseConnection()),
new CleanTags(\OC::$server->getDatabaseConnection(), \OC::$server->getUserManager()),
new RepairInvalidShares(\OC::$server->getConfig(), \OC::$server->getDatabaseConnection()),
new MoveUpdaterStepFile(\OC::$server->getConfig()),
Expand Down
55 changes: 55 additions & 0 deletions lib/private/Repair/RepairUnencryptedSize.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<?php
/**
* @copyright Copyright (c) 2023, Nextcloud GmbH.
*
* @author Simon Spannagel <[email protected]>
*
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/
namespace OC\Repair;

use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;
use OCP\Migration\IOutput;
use OCP\Migration\IRepairStep;

class RepairUnencryptedSize implements IRepairStep {
/** @var IDBConnection */
protected $connection;

public function __construct(IDBConnection $connection) {
$this->connection = $connection;
}

public function getName() {
return 'Repair file size of unencrypted files';
}

/**
* Fix unencrypted file size
*/
public function run(IOutput $output) {
$update = $this->connection->getQueryBuilder();
$update->update('filecache')
->set('unencrypted_size', "0")
->where($update->expr()->eq('encrypted', "0", IQueryBuilder::PARAM_INT))
->andWhere($update->expr()->neq('unencrypted_size', "0", IQueryBuilder::PARAM_INT));
Copy link
Member

@juliusknorr juliusknorr Jan 19, 2023

Choose a reason for hiding this comment

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

There is no index on neither of those columns, so we may want to add at least to the unencrypted size.

I think the most safe way to use such an index then would be:

Suggested change
->where($update->expr()->eq('encrypted', "0", IQueryBuilder::PARAM_INT))
->andWhere($update->expr()->neq('unencrypted_size', "0", IQueryBuilder::PARAM_INT));
->andWhere($update->expr()->gt('unencrypted_size', "0", IQueryBuilder::PARAM_INT));
->andWhere($update->expr()->eq('encrypted', "0", IQueryBuilder::PARAM_INT))

Copy link
Member

@juliusknorr juliusknorr Jan 19, 2023

Choose a reason for hiding this comment

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

For the reasoning:

  • neq may not use the index
  • the order of where statements sometimes also matters to decide if the index is applied

Copy link
Member

@juliusknorr juliusknorr Jan 19, 2023

Choose a reason for hiding this comment

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

EXPLAIN of the current query does a full table scan:


explain update oc_filecache set unencrypted_size = 0 where encrypted = 0 and unencrypted_size != 0;
+------+-------------+--------------+-------+---------------+---------+---------+------+------+-------------+
| id   | select_type | table        | type  | possible_keys | key     | key_len | ref  | rows | Extra       |
+------+-------------+--------------+-------+---------------+---------+---------+------+------+-------------+
|    1 | SIMPLE      | oc_filecache | index | NULL          | PRIMARY | 8       | NULL | 8479 | Using where |
+------+-------------+--------------+-------+---------------+---------+---------+------+------+-------------+

Copy link
Contributor Author

@simonspa simonspa Jan 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

After reversal of arguments and using > query affects smaller number of rows:

explain update oc_filecache set unencrypted_size = 0 where unencrypted_size > 0 and encrypted = 0;
+------+-------------+--------------+-------+---------------+---------+---------+------+------+-------------+
| id   | select_type | table        | type  | possible_keys | key     | key_len | ref  | rows | Extra       |
+------+-------------+--------------+-------+---------------+---------+---------+------+------+-------------+
|    1 | SIMPLE      | oc_filecache | index | NULL          | PRIMARY | 8       | NULL | 490  | Using where |
+------+-------------+--------------+-------+---------------+---------+---------+------+------+-------------+

Copy link
Contributor Author

@simonspa simonspa Jan 29, 2023

Choose a reason for hiding this comment

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

I implemented the suggested change - but adding an index to either of these columns would have to done separately, and probably run manually, right? How would that work together with an automatic repair step?


if ($update->execute()) {
$output->info('Fixed file size of unencrypted files');
}
}
}