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

feat(SetupChecks): Refactor DatabaseHasMissingIndices #45272

Merged
merged 1 commit into from
May 13, 2024
Merged

feat(SetupChecks): Refactor DatabaseHasMissingIndices #45272

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 11 additions & 3 deletions apps/settings/lib/SetupChecks/DatabaseHasMissingIndices.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use OCP\DB\Events\AddMissingIndicesEvent;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\SetupCheck\ISetupCheck;
use OCP\SetupCheck\SetupResult;

Expand All @@ -39,6 +40,7 @@ public function __construct(
private IL10N $l10n,
private Connection $connection,
private IEventDispatcher $dispatcher,
private IURLGenerator $urlGenerator,
) {
}

Expand Down Expand Up @@ -90,12 +92,18 @@ public function run(): SetupResult {
if (empty($missingIndices)) {
return SetupResult::success('None');
} else {
$list = '';
$processed = 0;
$list = $this->l10n->t('Missing indices:');
foreach ($missingIndices as $missingIndex) {
$list .= "\n".$this->l10n->t('Missing optional index "%s" in table "%s".', [$missingIndex['indexName'], $missingIndex['tableName']]);
$processed++;
$list .= "\n " . $this->l10n->t('"%s" in table "%s"', [$missingIndex['indexName'], $missingIndex['tableName']]);
if (count($missingIndices) > $processed) {
$list .= ", ";
}
Comment on lines +98 to +102
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally list should be formatted in markdown for security_guard.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can stick a - in front of each item in a follow-up, but it'll look funny in the current Overview since they will get displayed literally. Is there a more correct approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a more correct approach?

I don’t think so. We were pondering cutting after a double line break on core UI to be able to have more info only appearing in cli&security_guard but did not went forward with it.

}
return SetupResult::warning(
$this->l10n->t('The database is missing some indexes. Due to the fact that adding indexes on big tables could take some time they were not added automatically. By running "occ db:add-missing-indices" those missing indexes could be added manually while the instance keeps running. Once the indexes are added queries to those tables are usually much faster.').$list
$this->l10n->t('Detected some missing optional indices. Occasionally new indices are added (by Nextcloud or installed applications) to improve database performance. Adding indices can sometimes take awhile and temporarily hurt performance so this is not done automatically during upgrades. Once the indices are added, queries to those tables should be faster. Use the command `occ db:add-missing-indices` to add them. ') . $list . '.',
$this->urlGenerator->linkToDocs('admin-long-running-migration-steps')
);
}
}
Expand Down