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.

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

feat(setupchecks): add row format setup check for MySQL databases #48547

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

Altahrim
Copy link
Collaborator

@Altahrim Altahrim commented Oct 3, 2024

Summary

Add a setup check to warn about tables using ROW_FORMAT=Compressed
MySQL specific
Screenshot_2024-10-08_170508
Screenshot_2024-10-08_170554

Checklist

@Altahrim Altahrim added the 3. to review Waiting for reviews label Oct 3, 2024
@Altahrim Altahrim added this to the Nextcloud 31 milestone Oct 3, 2024
@Altahrim Altahrim requested review from ChristophWurst and a team October 3, 2024 08:50
@Altahrim Altahrim self-assigned this Oct 3, 2024
@Altahrim Altahrim requested review from yemkareems, provokateurin and sorbaugh and removed request for a team October 3, 2024 08:50
apps/settings/lib/SetupChecks/MysqlRowFormat.php Outdated Show resolved Hide resolved

public function run(): SetupResult {
if (!$this->connection->getDatabasePlatform() instanceof MySQLPlatform) {
return SetupResult::success($this->l10n->t('You are not using MySQL'));
Copy link
Contributor

@kesselb kesselb Oct 3, 2024

Choose a reason for hiding this comment

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

Do you think it makes sense to introduce something like SetupResult::skipped and then filter out the setup checks with severity = skipped by default? If you think so, I would log a feature request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea!

apps/settings/lib/SetupChecks/MysqlRowFormat.php Outdated Show resolved Hide resolved
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good!

apps/settings/lib/SetupChecks/MysqlRowFormat.php Outdated Show resolved Hide resolved
Comment on lines 59 to 63
$sql = 'SELECT table_name
FROM information_schema.tables
WHERE table_schema = ?
AND table_name LIKE "*PREFIX*%"
AND row_format = "Compressed";';
Copy link
Member

Choose a reason for hiding this comment

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

can you not run this with our query builder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question!
I didn't find it useful because it's only for MySQL like DBs and I am not sure I can query another database 🤔

@Altahrim Altahrim added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 8, 2024
@Altahrim Altahrim added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 15, 2024
@Altahrim Altahrim merged commit 7e99fd3 into master Oct 18, 2024
177 checks passed
@Altahrim Altahrim deleted the feat/row_format_check branch October 18, 2024 10:00
@Altahrim Altahrim removed the 3. to review Waiting for reviews label Oct 18, 2024

return SetupResult::warning(
$this->l10n->n(
'Table %s is not using ROW_FORMAT=Dynamic. This format offers the best database performances for Nextcloud. Please change the row format to Dynamic.',
Copy link
Member

Choose a reason for hiding this comment

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

plurals without %n are confusing for translators.
I would say the last line of https://docs.nextcloud.com/server/latest/developer_manual/basics/front-end/l10n.html#correct-plurals is applicable here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add setup warning in case row_format=compressed is still used
5 participants