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

[PVince81]

Add mariadb 10.3 to the unit test matrix.

For #29483 but doesn't cover mb4 AFAIK

[PVince81]

@ownclouders rebase

[ownclouders]

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

[codecov]

Codecov Report

Merging #33255 into master will increase coverage by 0.12%.
The diff coverage is 82.35%.

@@             Coverage Diff              @@
##             master   #33255      +/-   ##
============================================
+ Coverage     64.12%   64.24%   +0.12%     
+ Complexity    18305    18266      -39     
============================================
  Files          1193     1192       -1     
  Lines         69155    69056      -99     
  Branches       1277     1277              
============================================
+ Hits          44343    44363      +20     
+ Misses        24440    24321     -119     
  Partials        372      372

Flag	Coverage Δ	Complexity Δ
#javascript	52.91% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	65.55% <82.35%> (+0.13%)	18266 <11> (-39)	⬇️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/DB/ConnectionFactory.php	89.85% <ø> (+7.91%)	20 <0> (ø)	⬇️
lib/private/DB/MySqlTools.php	88.46% <82.35%> (+88.46%)	11 <11> (+7)	⬆️
lib/private/DB/Connection.php	66.17% <0%> (-2.21%)	49% <0%> (ø)
lib/private/DB/MDB2SchemaWriter.php	94.79% <0%> (+1.04%)	34% <0%> (ø)	⬇️
lib/private/Repair/Collation.php	87.5% <0%> (+5%)	12% <0%> (ø)	⬇️

---

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 81d5e68...a2078b3. Read the comment docs.

[ownclouders]

Automated rebase with GitMate.io was successful! 🎉

[DeepDiver1975]

results look ugly ... needs debugging ....

[VicDeo]

@DeepDiver1975 not ugly but predictable. mb4 support is detected basing on innodb_file_format == barracuda check. innodb_file_format option was deprecated in mariadb 10.2 and removed in mariadb 10.3
see #29483 (comment) if you need more details

[DeepDiver1975]

I was missing the whole conversation - THX

Can you add this maria db version check? THX

[PVince81]

the innodb_file_format was deprecated then removed in 10.3.

while we could rely on its value to default to Barracuda for 10.2.2 there is still the likeliness of distribution packages having the other value in their config.

I think we can first check if these values exist and rely on them. If the variables are gone, fall back to a DB version check. I see the DB version appear in some variables:

MariaDB [(none)]> show variables like '%ver%';
+------------------------------------------+----------------------------------+
| Variable_name                            | Value                            |
+------------------------------------------+----------------------------------+
| aria_force_start_after_recovery_failures | 0                                |
| aria_recover_options                     | BACKUP,QUICK                     |
| character_set_server                     | utf8mb4                          |
| collation_server                         | utf8mb4_general_ci               |
| innodb_force_recovery                    | 0                                |
| innodb_ft_server_stopword_table          |                                  |
| innodb_show_verbose_locks                | 0                                |
| innodb_version                           | 5.7.23                           |
| log_slow_verbosity                       |                                  |
| master_verify_checksum                   | OFF                              |
| myisam_recover_options                   | BACKUP,QUICK                     |
| protocol_version                         | 10                               |
| relay_log_recovery                       | OFF                              |
| server_id                                | 1                                |
| slave_sql_verify_checksum                | ON                               |
| slave_type_conversions                   |                                  |
| thread_pool_oversubscribe                | 3                                |
| version                                  | 10.2.18-MariaDB                  |
| version_comment                          | openSUSE package                 |
| version_compile_machine                  | x86_64                           |
| version_compile_os                       | Linux                            |
| version_malloc_library                   | system                           |
| version_ssl_library                      | OpenSSL 1.1.0h-fips  27 Mar 2018 |
| wsrep_convert_lock_to_trx                | OFF                              |
| wsrep_patch_version                      | wsrep_25.23                      |
| wsrep_recover                            | OFF                              |
+------------------------------------------+----------------------------------+

Interestingly there's also a value character_set_server and collation_server.

This is what I see on my local machine for 10.2:

MariaDB [(none)]> show variables like '%character_set%';
+--------------------------+------------------------------+
| Variable_name            | Value                        |
+--------------------------+------------------------------+
| character_set_client     | utf8                         |
| character_set_connection | utf8                         |
| character_set_database   | utf8mb4                      |
| character_set_filesystem | binary                       |
| character_set_results    | utf8                         |
| character_set_server     | utf8mb4                      |
| character_set_system     | utf8                         |
| character_sets_dir       | /usr/share/mariadb/charsets/ |
+--------------------------+------------------------------+

[VicDeo]

@PVince81

PDO::getAttribute(PDO::ATTR_SERVER_VERSION);

and then borrow a code piece from Doctrine
https://github.com/doctrine/dbal/blob/90f79c8b39fa451264537440ead0fe1ab1c751b8/lib/Doctrine/DBAL/Driver/AbstractMySQLDriver.php#L116-L119

[PVince81]

@VicDeo can I also kill #29100 ? it was stated that we can remove this workaround once DBAL is updated. I need to check what DBAL version has this.

[PVince81]

sorry, meant #29100 (edited comment now).
this was added as a workaround for something.

we're on dbal 2.8.0 on master

[PVince81]

rebased, and added:

• mariadb 10.3.1 detection: 04bb7d2 and default to true for mb4 support
• removed mariadb 10.2 which shouldn't be needed since DBAL 2.8.0: 0cb3266 (please confirm @VicDeo)

I tested the latter and saw that I am able to setup the DB locally with 10.2.

Somehow I can't run Drone locally so relying on CI to confirm both fixes now...

[PVince81]

hmmm, in Drone we have mariadb targets and mysql and mysqlmb4.

Currently mysqlmb4 fails with encoding issues: https://drone.owncloud.com/owncloud/core/12401/130
Mariadb 10.2 failed as well with encoding issues: https://drone.owncloud.com/owncloud/core/12401/189
Mariadb 10.3 passed: https://drone.owncloud.com/owncloud/core/12401/150

[PVince81]

reverted the removal of workaround, just in case...

[PVince81]

I still have no clue why this PR influences mysqlmb4 aka mysql 5.7. The logic should still be the same.

Also, I discovered this: https://dba.stackexchange.com/a/125893

With MySQL 8 the setting is also removed.

[PVince81]

ouch, of course... my logic is flawed 🤕

[PVince81]

should be fixed now.

I've also added MySQL 8 in the matrix for fun and a added a check since MySQL 8 has removed the innodb vars as well.

[PVince81]

cool, all passed except MySQL 8 which has setup issues:

+ DB=mysql
+ install_cmd='maintenance:install -vvv       --database=mysql       --database-name=owncloud       --database-table-prefix=oc_       --admin-user=admin       --admin-pass=admin       --data-dir=/drone/src/data '
+ [[ mysql != \s\q\l\i\t\e ]]
+ install_cmd+=' --database-host=mysql                  --database-user=owncloud                  --database-pass=owncloud'
+ php occ maintenance:install -vvv --database=mysql --database-name=owncloud --database-table-prefix=oc_ --admin-user=admin --admin-pass=admin --data-dir=/drone/src/data --database-host=mysql --database-user=owncloud --database-pass=owncloud
Error while trying to create admin user: Failed to connect to the database: An exception occurred in driver: SQLSTATE[HY000] [2054] The server requested authentication method unknown to the client
->

need to check, maybe the default creds have changed for that docker

[PVince81]

@phil-davis thank you, generous wizard. I have now applied your powerful magic to this pull request and hoping for the best.

[PVince81]

it worked !!!

@DeepDiver1975 @phil-davis @VicDeo please review

[phil-davis]

CI scripts LGTM.
I will let someone else comment on the "real" code changes.

[VicDeo]

@PVince81 Looks good. My quickfix for MariaDB 10.2 is not needed any more as it is already supported as a Doctrine Platform in our current doctrine/dbal version.

How difficult it would be to write a test for public function supports4ByteCharset(IDBConnection $connection) {?

[VicDeo]

While considering a backport please note that stable10 still uses older doctrine/dbal and could not be updated to the release with direct MariaDB 10.2.6+ support due to PHP compatibility reasons.

[PVince81]

@VicDeo thanks for the warning. I don't think we should backport this unless MariaDB 10.2 becomes EOL soon. From what I see here https://mariadb.org/about/maintenance-policy/ MariaDB 10.2 still runs up to May 2022.

Now if all Linux distributions start shipping 10.3 by default we might need to backport anyway at some point.

[PVince81]

• add unit test for the utility method

[PVince81]

@VicDeo mocking database variables and query function ? challenge accepted: a2078b3

[PVince81]

• discuss whether to backport
  • don't backport the 10.2 workaround removals

not sure how to read these results, seems that Ubuntu still is on MariaDB 10.1 here https://packages.ubuntu.com/search?keywords=mariadb-server

also see https://mariadb.com/kb/en/library/distributions-which-include-mariadb/

[VicDeo]

Great!

[phil-davis]

Backporting as part of "drop PHP 7.0" in stable10 #35786
Hopefully that will all be good and allow mariadb 10.3 support in ownCloud10