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

MariaDb changes for MariaDb 10.6 #39286

Merged
merged 1 commit into from
Oct 6, 2021
Merged

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Sep 25, 2021

Description

These are the code changes cherry-picked from PR #39282

They are code changes that make CI pass against MariaDb 10.6

This PR will confirm if these code changes are OK for the oldest supported MariaDb version 10.2.

Related Issue

How Has This Been Tested?

CI

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

@phil-davis phil-davis self-assigned this Sep 25, 2021
@phil-davis phil-davis marked this pull request as ready for review September 27, 2021 12:54
@phil-davis
Copy link
Contributor Author

@IljaN these are the changes that you made to get CI passing with MariaDb 10.6. The CI passes here with MariaDb 10.2, so the changes are backward-compatible on a newly-installed system.

  1. can you please write a changelog for this?

For everyone:
2) what should we do about checking upgrades of existing oC10 systems that use MariaDb 10.2/3/4/5 ?
3) what else can we think of that might be a problem?

@AlexAndBear
Copy link

Do we know how long it might take to migrate for a user with a big database ?

@IljaN
Copy link
Member

IljaN commented Oct 4, 2021

Do we know how long it might take to migrate for a user with a big database ?

I couldn't find any info on this. So I guess we need to test. If it's only a metadata DDL change for MariaDB then it might not take long (depends). By the looks of it (just guessing) MariaDB used to store Columns (and indices?) in a compressed form and deprecated this now. Two scenarios I can think of:

  • We only change the DDL and Maria is smart enough to convert it's internal structure on the fly / on use
  • Database needs to be taken offline and converted/migrated where all data on disk is rewritten

@mrow4a
Copy link
Contributor

mrow4a commented Oct 4, 2021

In this PR migration step for ALTERING ROW_FORMAT for tables is missing.

@IljaN
Copy link
Member

IljaN commented Oct 4, 2021

Is the migration something which should be done during a normal ownCloud (minor) upgrade or does it need planing by the admin? There is also the option to set the row type globally so we might be able to remove it from table definitions?

nextcloud/server#25436 (comment)

@mrow4a
Copy link
Contributor

mrow4a commented Oct 5, 2021

@IljaN ok, maybe we then provide Repair command only for these that fail to upgrade ? For all new instances we just do not use ROW_FORMAT ? Plus we make sure when MariaDB upgrade is attempted we throw proper error with what to do?

@AlexAndBear
Copy link

@IljaN @mrow4a I would rather love to have a migration command (if the migrate process is quite short),
ROW_FORMAT DYNAMIC also has the advantage of a faster database.

From a user perspective, figuring out the problem, reading the docs, run the repair command is pretty laborious,
what do you think?

core/Migrations/Version20211005081736.php Outdated Show resolved Hide resolved
core/Migrations/Version20211005081736.php Outdated Show resolved Hide resolved
core/Migrations/Version20211005081736.php Outdated Show resolved Hide resolved
core/Migrations/Version20181220085457.php Outdated Show resolved Hide resolved
.htaccess Outdated Show resolved Hide resolved
@AlexAndBear AlexAndBear force-pushed the MariaDb-changes-for-10.6 branch 2 times, most recently from f293010 to 5c0ca6d Compare October 5, 2021 10:04
@jvillafanez
Copy link
Member

I'd vote for removing the row format from our code. From my point of view, we shouldn't need to care about it. We could suggest to use a specific format because of performance reasons, but I don't think we should enforce it. The problem we have now is that we enforce to use a specific format that is now deprecated.

Ideally, that should be fixed by a DBA, completely isolated from ownCloud: switch off ownCloud, convert the DB (on your own, with whatever tools the DB provides), switch on ownCloud.

@AlexAndBear
Copy link

AlexAndBear commented Oct 5, 2021

I'd vote for removing the row format from our code

I agree, I will check if we can set the row_format=DEFAULT in the migration which is DYNAMIC in MariaDB

NOTE: changed to DEFAULT, after migration row_format is DYNAMIC in mariadb10.6, which is good

lib/private/DB/ConnectionFactory.php Outdated Show resolved Hide resolved
@phil-davis
Copy link
Contributor Author

We could bump the oC10 version from 10.8.1 prealpha to 10.9.0 prealpha in this PR. That should trigger the migration to auto-run when any developer/QA person updates. That will allow someone to test by installing MariaDb 10.5 with oC 10.8.0 checked out, then checkout this branch (or head of master when this is merged) and php occ:upgrade and it should "just work".

@AlexAndBear
Copy link

@phil-davis good idea, still wondering if we should migrate or repair (which should be triggered), my favor is migrate

@phil-davis
Copy link
Contributor Author

@phil-davis good idea, still wondering if we should migrate or repair (which should be triggered), my favor is migrate

I am happy whichever is decided - auto-migrate, or something for an admin to do explicitly.

@jvillafanez
Copy link
Member

I'm not sure ownCloud 10.x can work normally without having that row format in the DB, at least until the next core upgrade.

Assuming OC 10.x can work without problems (at least until the next core upgrade), I'd remove the migration.

If the admin keeps using mariaDB 10.5, the only changes we have is that we no longer require the compressed row format, so the repair step won't do anything.

If the admin wants to use mariaDB 10.6, he can change the row format at any time; there is no need to wait for the upgrade (assuming OC doesn't give any problem). The only thing he needs to be careful about is to update to 10.9 for the next upgrade. The upgrade itself on our side won't do anything special.
The only problematic scenario is that the admin, having mariaDB 10.6, updates to OC 10.8 or earlier. In that case, I expect the migration to take some time because I expect ownCloud to change the DB row format back to compressed (due to the repair steps). Anyway, I don't think we can do anything about this other than document that mariaDB 10.6 is "officially supported" from 10.9 onward.

For an update from mariaDB 10.5 + OC 10.8 to mariaDB 10.6 + OC 10.9, I think the best path is:

  1. Change row format in mariaDB 10.5
  2. Update mariaDB to 10.6
  3. Update OC to 10.9
    ownCloud should be able to run after each step without problem. In theory, you should be able to stay for a while in step 1, and the same for step 2.

@AlexAndBear
Copy link

AlexAndBear commented Oct 5, 2021

@jvillafanez

If the admin keeps using mariaDB 10.5, the only changes we have is that we no longer require the compressed row format, so the repair step won't do anything.

This is not quite correct, I will give the admin the possibility to change MariaDB to 10.6 easily.
Why do we want to enforce our users to search for a problem, which we then need to explain and document, so the user needs to run several queries and commands?
We can solve all this overhead for us and the admin, by a simple migration

@phil-davis
Copy link
Contributor Author

If we have a migration, then it is currently going to run on every installation where $platformName == 'mysql'

I guess that will be every installation that uses "real" MySQL and that uses any supported MariaDb 10.2, 10.3 10.4 or 10.5.

Do installations with "real" MySQL need to do this change at all?
(Is a similar deprecation coming for MySQL?)

@IljaN
Copy link
Member

IljaN commented Oct 5, 2021

Do installations with "real" MySQL need to do this change at all?
(Is a similar deprecation coming for MySQL?)

MySQL is not affected.

@AlexAndBear
Copy link

@phil-davis according to @jvillafanez and also this comment nextcloud/server#25436 (comment)
I agree that row_format should be set by the DBA if needed and not by ownCloud, so I like to
pull right away

@AlexAndBear AlexAndBear force-pushed the MariaDb-changes-for-10.6 branch 4 times, most recently from f7cc00d to aeedc05 Compare October 6, 2021 07:59
@AlexAndBear AlexAndBear requested review from AlexAndBear and removed request for AlexAndBear October 6, 2021 08:08
@AlexAndBear AlexAndBear self-requested a review October 6, 2021 08:09
@owncloud owncloud deleted a comment from update-docs bot Oct 6, 2021
core/Command/Db/ConvertRowFormat.php Outdated Show resolved Hide resolved
core/Command/Db/ConvertRowFormat.php Outdated Show resolved Hide resolved
core/Command/Db/ConvertRowFormat.php Outdated Show resolved Hide resolved
core/Command/Db/ConvertRowFormat.php Outdated Show resolved Hide resolved
core/Command/Db/ConvertRowFormat.php Outdated Show resolved Hide resolved
@jvillafanez
Copy link
Member

It seems to be a mismatch between the code and the expectations. The code seems to convert the table row format to the default one, however, the command's help suggests that it's possible to convert to other row formats, but this isn't the case.

@AlexAndBear
Copy link

@jvillafanez We might extend it in the future, but not for now.

@jvillafanez
Copy link
Member

@jvillafanez We might extend it in the future, but not for now.

That's ok, no problem with that.

To clarify a bit my points:

  • Any content that is facing the users or admins needs to reflect the actual state of how the code behaves. In this case, the command only allows conversion to the default format, which means that the option should be removed (we aren't allowing the admin to choose), and the help message should be adjusted (the conversion is to the default row format of the DB, not anyone)
  • Future plans should be documented in the code. The idea is that, after forgetting about this piece of code, you should be able to reconstruct your thought process and implement what is missing following the original plan. Depending on how far you want to implement those plans now, you might need more or less documentation. For this case, I think the options are:
    • Implement the code as if only the the default row format will be used. No real reference that additional row formats could be used in the future.
    • Implement the code as if only the the default row format will be used but having a list of allowed formats with "default" being the only one allowed (I think this is the current state). It's a good point to have some comments around to explain the future plans and what is left to do.
    • Assume that, somehow, the row format will be gotten from the admin, although it's currently hardcoded to use "default" now. The code will have to assume a variable row format, and it might be a good idea to play around a bit by changing the hardcoded value to ensure the code works properly. The idea is to reduce the amount of changes to be done once we start implementing the plans.

@sonarcloud
Copy link

sonarcloud bot commented Oct 6, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

96.7% 96.7% Coverage
0.0% 0.0% Duplication

Copy link
Contributor Author

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

LGTM - I am the PR "owner" so I can't approve.

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.

None yet

6 participants