MariaDB 10.2.7+ #29100

VicDeo · 2017-09-26T22:12:26Z

Description

Backup solution for #28695
I'm not happy with this solution but if upstream doesn't manage to fix the issue until 10.0.4 we'll need it

Here we bypass native Doctrine/DBAL MySqlSchemaManager::_getPortableTableColumnDefinition implementation for MariaDB >= 10.2.7 and use customized implementation that unquotes default values.
All DB engines from non-MySQL family work as before. For MySQL family a new listener is registered.
Triggering native/customized codeflow is done inside the listener itself.

Related Issue

#28695

Motivation and Context

Fixes MariaDB 10.2.7+ support

How Has This Been Tested?

By installing ownCloud with MariaDB 10.2.7+ as a DB engine

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

PVince81 · 2017-10-09T13:48:13Z

@VicDeo I'm setting this to p1 to keep it in view before releasing 10.0.4. When the time comes we'll decide whether to use the PR or upstream depending on the progress.

VicDeo · 2017-10-13T12:23:01Z

Stable10: #29240

PVince81 · 2017-10-13T13:04:48Z

It looks like it's likely that we won't be able to use the fix from upstream because it will likely not be backported: doctrine/dbal#2825 (comment)

And OC 10 still needs to support PHP 5.6.

So I suggest we merge this for now and kill it once we deprecate PHP 7.0...

@DeepDiver1975 do you still have objections ?

PVince81 · 2017-10-16T08:49:40Z

let's get an explicit statement here: doctrine/dbal#2825 (comment)

codecov · 2017-10-18T14:49:58Z

Codecov Report

Merging #29100 into master will increase coverage by 0.24%.
The diff coverage is 78.15%.

@@             Coverage Diff              @@
##             master   #29100      +/-   ##
============================================
+ Coverage     60.84%   61.08%   +0.24%     
- Complexity    17238    17287      +49     
============================================
  Files          1032     1033       +1     
  Lines         57365    57486     +121     
============================================
+ Hits          34904    35116     +212     
+ Misses        22461    22370      -91

Impacted Files	Coverage Δ	Complexity Δ
lib/private/DB/ConnectionFactory.php	`82.89% <100%> (+19.88%)`	`21 <0> (ø)`	⬇️
...private/DB/MySqlSchemaColumnDefinitionListener.php	`77.47% <77.47%> (ø)`	`46 <46> (?)`
lib/private/DB/Connection.php	`67.91% <80%> (+2.01%)`	`48 <3> (+3)`	⬆️
lib/private/Repair/SqliteAutoincrement.php	`85.18% <0%> (-3.28%)`	`9% <0%> (ø)`
lib/private/DB/AdapterSqlite.php	`83.33% <0%> (-2.88%)`	`7% <0%> (ø)`
lib/private/Repair/RepairMismatchFileCachePath.php	`82.52% <0%> (+0.4%)`	`48% <0%> (ø)`	⬇️
lib/private/DB/QueryBuilder/QueryBuilder.php	`90.68% <0%> (+0.49%)`	`67% <0%> (ø)`	⬇️
lib/private/DB/MDB2SchemaManager.php	`85.96% <0%> (+5.26%)`	`17% <0%> (ø)`	⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 800f59f...a0ed768. Read the comment docs.

PVince81 · 2017-10-30T20:30:03Z

seems no upstream backport doctrine/dbal#2825 (comment)

PVince81 · 2017-11-02T10:47:23Z

@VicDeo please write a unit test for the listener and make codecov happy.

If we're going to carry this workaround long term, better have it properly covered.
Thanks.

VicDeo · 2017-11-02T11:16:37Z

@PVince81 oook... :-/
(I already checked doctrine unit tests and know that they are not reusable here...)

DeepDiver1975 · 2017-11-02T12:23:39Z

If we're going to carry this workaround long term, better have it properly covered.
Thanks.

@SergioBertolinSG is working on getting coverage for mysql specific code - see #29410
Once merged I assume codecov will be happy here as well ...

DeepDiver1975 · 2017-11-02T12:24:43Z

@SergioBertolinSG is working on getting coverage for mysql specific code - see #29410
Once merged I assume codecov will be happy here as well ...

looks like we need to add mariadb 10.2.7 to the test matrix

PVince81 · 2017-11-03T13:59:29Z

can we get this finished for RC1?

…ition

VicDeo · 2017-11-03T16:29:22Z

@DeepDiver1975 yes, looking right now.
Doctrine\DBAL\Driver\PDOException: SQLSTATE[22007]: Invalid datetime format: 1366 Incorrect string value: '\xF0\x9F\x98\x90, ...' for column 'path' at row 1 seems to related to utf8mb4

While Test\DB\SchemaDiffTest::testZeroChangeOnSchemaMigrations failures are fishy indeed.

VicDeo · 2017-11-06T23:04:27Z

@DeepDiver1975 @PVince81 Tests related to false positives for default values are passing now.
Can't reproduce failures related to 4 byte utf-8 with my MariaDB.

VicDeo · 2017-11-07T00:11:31Z

Ok. Drone uses MariaDb 10.3.2
Variable innodb_file_format doesn't exist in MariaDB since 10.3
See https://mariadb.com/kb/en/library/changes-improvements-in-mariadb-103/

innodb_file_format is used for utf8mb4 detection
https://github.com/owncloud/core/blob/master/lib/private/DB/MySqlTools.php#L36
no utf8mb4 support. Tests are failing...

PVince81 · 2017-11-07T08:18:57Z

@VicDeo if mb4 is default in 10.3, we might need some kind of migration for the data... raise new ticket ?

PVince81 · 2017-11-07T08:25:00Z

@VicDeo I've raised #29483 to look into the absence of said switch.

As @DeepDiver1975 said we need to define max versions for databases. This means we might need to fix our Drone setup for now to test the older MariaDB and bypass the missing innodb switch. @SergioBertolinSG

VicDeo · 2017-11-07T18:26:35Z

@PVince81 as you see everything passes here. I adjusted MariaDB version for Drone in the very last commit

Merging #29100 into master will increase coverage by 0.24%.
The diff coverage is 78.15%.

PVince81 · 2017-11-07T20:03:53Z

lib/private/DB/MySqlSchemaColumnDefinitionListener.php

-		$eventArgs->setColumn($column);
+		$version = \OC::$server->getDatabaseConnection()->getDatabaseVersionString();
+		$mariadb = false !== stripos($version, 'mariadb');
+		if ($mariadb && version_compare($this->getMariaDbMysqlVersionNumber($version), '10.2.7', '>=')) {


10.2.7, really ? not even 10.2.0 ? 😢

@PVince81 https://jira.mariadb.org/browse/MDEV-13132
Fix Version/s: 10.2.7

PVince81 · 2017-11-07T20:07:33Z

lib/private/DB/MySqlSchemaColumnDefinitionListener.php

-		$column = $this->_getPortableTableColumnDefinition($tableColumn);
-		$eventArgs->setColumn($column);
+		$version = \OC::$server->getDatabaseConnection()->getDatabaseVersionString();
+		$mariadb = false !== stripos($version, 'mariadb');


what if $version is null ?

@PVince81 stripos will return false without a warning

PVince81

Code looks good to me. See comments.

If @DeepDiver1975 also agrees with the change then let's merge.

PVince81

👍

PVince81 · 2017-11-08T09:02:18Z

I've explained your changes to @DeepDiver1975 and he agreed for merging

lock · 2019-08-02T00:02:41Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

VicDeo added the 2 - Developing label Sep 26, 2017

VicDeo added this to the development milestone Sep 26, 2017

VicDeo changed the title ~~Add draft solution~~ MariaDB 10.2.7+ Sep 26, 2017

PVince81 added the p1-urgent Critical issue, need to consider hotfix with just that issue label Oct 9, 2017

VicDeo force-pushed the fix-28695-backup branch 5 times, most recently from e74bdc1 to 3a9b4e4 Compare October 12, 2017 21:28

VicDeo added 3 - To Review and removed 2 - Developing labels Oct 12, 2017

VicDeo force-pushed the fix-28695-backup branch from 3a9b4e4 to 5b4c939 Compare October 13, 2017 12:13

VicDeo mentioned this pull request Oct 13, 2017

[Stable10] MariaDB 10.2.7+ #29240

Merged

9 tasks

VicDeo force-pushed the fix-28695-backup branch from 5b4c939 to e24c924 Compare October 18, 2017 14:49

VicDeo self-assigned this Oct 19, 2017

VicDeo force-pushed the fix-28695-backup branch 2 times, most recently from 7062d25 to 562d82d Compare October 20, 2017 21:23

VicDeo force-pushed the fix-28695-backup branch from 562d82d to 0926541 Compare October 30, 2017 15:53

Minimal portion of code to bypass native _getPortableTableColumnDefin…

e6b52ca

…ition

DeepDiver1975 mentioned this pull request Nov 6, 2017

[stable10] load app type only once #29428

Merged

PVince81 modified the milestones: development, planned Nov 6, 2017

Detect MariaDB 10.2.7+, unquote all defaults

82c780a

VicDeo force-pushed the fix-28695-backup branch from bedb2d7 to 82c780a Compare November 6, 2017 22:34

VicDeo force-pushed the fix-28695-backup branch from bda7a2d to 766e7ae Compare November 7, 2017 00:00

VicDeo force-pushed the fix-28695-backup branch from e6639ec to 6908672 Compare November 7, 2017 00:19

10.3 is alpha, test with 10.2

a0ed768

VicDeo force-pushed the fix-28695-backup branch from 6908672 to a0ed768 Compare November 7, 2017 00:57

PVince81 mentioned this pull request Nov 7, 2017

Release checklist 10.0.4 RC1 #29468

Closed

22 tasks

VicDeo added 3 - To Review and removed 2 - Developing labels Nov 7, 2017

PVince81 reviewed Nov 7, 2017

View reviewed changes

VicDeo requested a review from DeepDiver1975 November 8, 2017 08:32

PVince81 approved these changes Nov 8, 2017

View reviewed changes

PVince81 merged commit c369e11 into master Nov 8, 2017

PVince81 deleted the fix-28695-backup branch November 8, 2017 09:02

ownclouders added the status/STALE label Dec 20, 2017

PVince81 mentioned this pull request Nov 7, 2018

Add mariadb 10.3 + mysql 8 to Drone, remove 10.2 workarounds #33255

Merged

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MariaDB 10.2.7+ #29100

MariaDB 10.2.7+ #29100

VicDeo commented Sep 26, 2017 •

edited

Loading

PVince81 commented Oct 9, 2017

VicDeo commented Oct 13, 2017

PVince81 commented Oct 13, 2017

PVince81 commented Oct 16, 2017

codecov bot commented Oct 18, 2017 •

edited

Loading

PVince81 commented Oct 30, 2017

PVince81 commented Nov 2, 2017

VicDeo commented Nov 2, 2017 •

edited

Loading

DeepDiver1975 commented Nov 2, 2017

DeepDiver1975 commented Nov 2, 2017

PVince81 commented Nov 3, 2017

VicDeo commented Nov 3, 2017 •

edited

Loading

VicDeo commented Nov 6, 2017 •

edited

Loading

VicDeo commented Nov 7, 2017 •

edited

Loading

PVince81 commented Nov 7, 2017

PVince81 commented Nov 7, 2017

VicDeo commented Nov 7, 2017 •

edited

Loading

PVince81 Nov 7, 2017

VicDeo Nov 8, 2017

PVince81 Nov 7, 2017

VicDeo Nov 8, 2017

PVince81 left a comment

PVince81 left a comment

PVince81 commented Nov 8, 2017

lock bot commented Aug 2, 2019

MariaDB 10.2.7+ #29100

MariaDB 10.2.7+ #29100

Conversation

VicDeo commented Sep 26, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

PVince81 commented Oct 9, 2017

VicDeo commented Oct 13, 2017

PVince81 commented Oct 13, 2017

PVince81 commented Oct 16, 2017

codecov bot commented Oct 18, 2017 • edited Loading

Codecov Report

PVince81 commented Oct 30, 2017

PVince81 commented Nov 2, 2017

VicDeo commented Nov 2, 2017 • edited Loading

DeepDiver1975 commented Nov 2, 2017

DeepDiver1975 commented Nov 2, 2017

PVince81 commented Nov 3, 2017

VicDeo commented Nov 3, 2017 • edited Loading

VicDeo commented Nov 6, 2017 • edited Loading

VicDeo commented Nov 7, 2017 • edited Loading

PVince81 commented Nov 7, 2017

PVince81 commented Nov 7, 2017

VicDeo commented Nov 7, 2017 • edited Loading

PVince81 Nov 7, 2017

Choose a reason for hiding this comment

VicDeo Nov 8, 2017

Choose a reason for hiding this comment

PVince81 Nov 7, 2017

Choose a reason for hiding this comment

VicDeo Nov 8, 2017

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Nov 8, 2017

lock bot commented Aug 2, 2019

VicDeo commented Sep 26, 2017 •

edited

Loading

codecov bot commented Oct 18, 2017 •

edited

Loading

VicDeo commented Nov 2, 2017 •

edited

Loading

VicDeo commented Nov 3, 2017 •

edited

Loading

VicDeo commented Nov 6, 2017 •

edited

Loading

VicDeo commented Nov 7, 2017 •

edited

Loading

VicDeo commented Nov 7, 2017 •

edited

Loading