[MDL-51080] upgrading on OS where mysql changed to mariadb does not give useful advice Created: 12/Aug/15  Updated: 29/Jun/18  Resolved: 29/Jun/18

Status: Closed
Project: Moodle
Component/s: Database SQL/XMLDB
Affects Version/s: 2.9.1, 3.0.3, 3.4.3, 3.5, 3.6
Fix Version/s: 3.4.4, 3.5.1

Type: Bug Priority: Major
Reporter: John Heidemann Assignee: Matteo Scaramuccia
Resolution: Fixed Votes: 2
Labels: ci, triaged
Remaining Estimate: 0 minutes
Time Spent: Not Specified
Original Estimate: 0 minutes

Attachments: PNG File mariadb_detected.png    
Issue Links:
Bonfire Testing
Testing discovered MDL-62824 Review <code> styling, specially when... Open
Dependency
will help resolve MDL-61263 Windows bundle 3.2.7 and site upgrade Open
will help resolve MDL-49275 Cannot install/upgrade with MariaDB Closed
Duplicate
is duplicated by MDL-53496 Moodle upgrade to 3.0.2 from 3.0.1 do... Closed
Relates
has been marked as being related by MDL-61702 Can't install Moodle version > 3.1 wi... Closed
Peer reviewer: Nicolas Martignoni
Integrator: Eloy Lafuente (stronk7)
Tester: Eloy Lafuente (stronk7)
Participants:
Testing Instructions:

Test

  • Start installing Moodle on a MariaDB server supported version while selecting mysqli as the DB driver
  • The Server checks should block the next stage, telling you that you have wrongly define the DB Type: Wrong $CFG->dbtype: you need to change it in your config.php file, from 'mysql' to 'mariadb'.

Regressions

  • Start installing Moodle on a MySQL server supported version while selecting mysqli as the DB driver
  • The Server checks should go further to the next stage, w/o any blocking message
Difficulty: Easy
Affected Branches: MOODLE_29_STABLE, MOODLE_30_STABLE, MOODLE_34_STABLE, MOODLE_35_STABLE, MOODLE_36_STABLE
Fixed Branches: MOODLE_34_STABLE, MOODLE_35_STABLE
Pull from Repository: https://github.com/scara/moodle
Pull Master Branch: m36_MDL-51080_MAriaDB_Wrong_DB_Type
Pull Master Diff URL: https://github.com/scara/moodle/commit/09ca9728773bf039bf40e9353d17a01727eb5a5b
Fix Release Date: 9/Jul/18
Component watchers:
Jake Dallimore, Jun Pataleta

 Description   

When upgrading Moodle, it checks the database version.

Some OSes (at least Fedora and Ubuntu) have switched from MySql to MariaDB. Moodle supports both.

But if moodle was configured to use mysql (installed in a prior OS version), the OS changes to MariaDB, and moodle doesn't, then moodle will fail to upgrade.

This problem was reported at http://www.learnbydoingit.org/2015/01/moodle-version-5-5-31-required-running-5-5-5-10-0-x/
with the fix: change dbtype to mariadb in config.php.

WHAT ACTUALLY HAPPENS:
Moodle's upgrade check gives only the error: "version 5.5.31 is required and you are running 5.5.5.10.0.20". This error leads to things like issue #45070 and not the fix in the blog.

WHAT SHOULD HAPPEN: the mysql-version checking code should generate an explicit recommendation "please change dbtype to mariadb in config.php" if it detects -10. in the version string.

STEPS TO REPRODUCE: install moodle-2.8 on an OS with mysql (say, fedora 18). Upgrade the OS to pick up maria db (say, fedora 19 or later). Upgrade Moodle. Hilarity ensues.



 Comments   
Comment by Marina Glancy [ 12/Aug/15 ]

Thanks for reporting this.

I've put it on the backlog.

In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

Comment by Dan Poltawski [ 03/Nov/15 ]

+1 to this.

Comment by Pavan Karthik [ 04/Dec/16 ]

Hi, I would like to work on this bug.
But I am new to moodle development,So I would appreciate some help or some pointers in setting up the developer environment and on how to proceed.
thanks.

Comment by Matteo Scaramuccia [ 14/Dec/17 ]

Another interesting thread: https://moodle.org/mod/forum/discuss.php?d=362906.
It's worth noting that the Wiki page linked by the error cannot exist since it depends on the version of MariaDB.

Comment by Nicolas Martignoni [ 14/May/18 ]

Another one here: https://moodle.org/mod/forum/discuss.php?d=367311#p1493704 (as of Moodle 3.5 beta).

Resolution of this would certainly help people.

Comment by Matteo Scaramuccia [ 10/Jun/18 ]

Hello Everyone,
though MariaDB is still a MySQL drop-in replacement, there are several differences in many of the common features.
An example:

I'll propose a change to recognize that Moodle is running on MariaDB and the DB Type is wrongly configured as mysql.
The opposite should require more work, not strictly related to this issue.

HTH,
Matteo

Comment by Nicolas Martignoni [ 11/Jun/18 ]

Matteo Scaramuccia: +1 to this. As both DB will probably diverge, this seems a good way to handle the issue.

Comment by Matteo Scaramuccia [ 15/Jun/18 ]

Hello Everyone,
here is the hack, available for peer review, before creating the other branches.

HTH,
Matteo

Comment by CiBoT [ 15/Jun/18 ]

Fails against automated checks.

Checked MDL-51080 using repository: https://github.com/scara/moodle

Should these errors be fixed?

Comment by Matteo Scaramuccia [ 21/Jun/18 ]

Anyone volunteering for a peer review? Nicolas Martignoni ?

TIA,
Matteo

Comment by Nicolas Martignoni [ 21/Jun/18 ]

Never did a review but would do it if I can. Any tips for a rookie?

Comment by Matteo Scaramuccia [ 22/Jun/18 ]

HI Nicolas,
you should follow https://docs.moodle.org/dev/Peer_reviewing with the goal of evaluating the proposed checklist, including a quick smoke test of the PR.
For your convenience, here is a quick example.

TIA,
Matteo

Comment by Nicolas Martignoni [ 22/Jun/18 ]

Hi Matteo,
Thanks for the tips. So here's my first peer review. Hopefully did it right.
[Y] Syntax
[-] Output
[Y] Language
[-] Databases
[N] Testing (instructions and automated tests)
[-] Security
[-] Performance and Clustering
[-] Documentation
[Y] Git
[-] Third party code
[Y] Sanity check
[-] Icons

Your patch seems OK, thanks for working on this. Some notes:

  • Syntax: Correcting var names would affect other parts of code. Passing.
  • Testing: No automated tests. Should any be added?
  • Are Jake Dallimore or Jun Pataleta aware of this?

Ciao
Nicolas

Comment by Matteo Scaramuccia [ 22/Jun/18 ]

Tip for Nicolas , use:
{noformat}
...
{noformat}
to wrap the peer review checklist.

Edit: removed the extra wrapping to show my example by emailing the diff too .

HTH,
Matteo

Comment by Matteo Scaramuccia [ 22/Jun/18 ]

Hi Nicolas,

Testing: No automated tests. Should any be added?

There's IMHO no automation there for the env DB checks and it would require mocking the DB object - integration tests would require a run for each different supported DB, controlling when to use one DB instead of another and usually if you use e.g. Oracle you already select the proper DB Type before running the integration tests - , which is a big effort. That's the reason why I've added no unit tests.
BTW I'm open for proposals, to create a unit for this issue.

HTH,
Matteo

Comment by Nicolas Martignoni [ 22/Jun/18 ]

Ok, here's my review updated.

[Y] Syntax
[-] Output
[Y] Language
[-] Databases
[-] Testing (instructions and automated tests)
[-] Security
[-] Performance and Clustering
[-] Documentation
[Y] Git
[-] Third party code
[Y] Sanity check
[-] Icons

 

 

Comment by Nicolas Martignoni [ 22/Jun/18 ]

Reviewed. Sorry for the spam, didn't know I had the rights to use the peer review buttons.

Comment by Jun Pataleta [ 25/Jun/18 ]

From Nicolas' review below, this seems to be ready for integration review. So I'll be pushing this forward. Thanks!

Comment by CiBoT [ 25/Jun/18 ]

Fails against automated checks.

Checked MDL-51080 using repository: https://github.com/scara/moodle

Should these errors be fixed?

Comment by CiBoT [ 25/Jun/18 ]

Fails against automated checks.

Checked MDL-51080 using repository: https://github.com/scara/moodle

Should these errors be fixed?

Comment by CiBoT [ 25/Jun/18 ]

Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

Comment by CiBoT [ 25/Jun/18 ]

Fails against automated checks.

Checked MDL-51080 using repository: https://github.com/scara/moodle

Should these errors be fixed?

Comment by Matteo Scaramuccia [ 25/Jun/18 ]

Hi Jun,
it's worth mention that I could code the opposite too (type=mariadb but running on MySQL) still based on the same check since MariaDB is, today, hardcoded in the "mysql server version", https://github.com/MariaDB/server/blob/99bcec295d4d367fe8186cc4db70e95fc4149a49/include/mysql_version.h.in#L14, whilst not the same for 5.5.5- which could be removed when using their client.

I could even add two unit tests which test the "mysql server version" assumption above, under both MySQL and MariaDB Moodle DB types.

Note: RPL_VERSION_HACK "5.5.5-" is removed when using an authentication plug-in: https://github.com/MariaDB/server/blob/cac41001864ca503a7812b7f2a3b312435fb4ec4/sql-common/client.c#L3547.

HTH,
Matteo

Comment by Eloy Lafuente (stronk7) [ 28/Jun/18 ]

Integrated (34, 35 and master), thanks!

Comment by Eloy Lafuente (stronk7) [ 28/Jun/18 ]

Have run install and upgrade in master and 34_STABLE and the mix of mysql config with mariadb db was detected perfectly.

If something, I'd say that the message does not look great (because of all those <code> tags and how they are styled, specifically within a notifications. Both clean and pre 3.5 boost look far from perfect. But that's not something to fix here.

So passing, ciao

Comment by Matteo Scaramuccia [ 28/Jun/18 ]

TNX Eloy,
I was in doubt how to backport the "hack" into the other branches.

Please, notify even here about the follow up about <code> in notifications .

TIA,
Matteo

Comment by Eloy Lafuente (stronk7) [ 28/Jun/18 ]

Yeah, i also had some doubts about backporting but, after all, it looked safe enough and better to inform people on next upgrade before becoming crazy with the silent migration.

I've created MDL-62824 about the <code> styling, btw.

Thanks and ciao

Comment by CiBoT [ 29/Jun/18 ]

Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

Closing as fixed!

Generated at Fri Aug 17 10:20:29 AWST 2018 using JIRA 7.3.2#73013-sha1:3d53c97478658c0b98b7301c496605f0c91c20aa.