Skip to content

Send proper MariaDB database version to the stats server #25281

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 23, 2019
Merged

Send proper MariaDB database version to the stats server #25281

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion plugins/system/stats/stats.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,14 +324,22 @@ protected function getRenderer($layoutId = 'default')
*/
private function getStatsData()
{
return array(
$data = array(
'unique_id' => $this->getUniqueId(),
'php_version' => PHP_VERSION,
'db_type' => $this->db->name,
'db_version' => $this->db->getVersion(),
'cms_version' => JVERSION,
'server_os' => php_uname('s') . ' ' . php_uname('r')
);

// Check if we have a MariaDB version string and extract the proper version from it
if (preg_match('/^(?:5\.5\.5-)?(mariadb-)?(?P<major>\d+)\.(?P<minor>\d+)\.(?P<patch>\d+)/i', $data['db_version'], $versionParts))
Copy link
Member

@richard67 richard67 Jun 20, 2019

Choose a reason for hiding this comment

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

This regex and the one from doctrine care for string like "5.5.5-Mariadb-10.0.8-xenial", as the function documentation at doctrine sais. This kind of strings seems to come from a database select for the versio number. But in Joomla framework database package, the mysqli::$server_info is used, which gives string like e.g. "5.5.5-10.1.29-MariaDB", see e.g. here: https://www.php.net/manual/de/mysqli.get-server-info.php#118822. Also in issue #25245 @Quy reported a string like that. So that regex works, because "mariadb-" may appear zero or one time, and the rest after the real version number is ignored, but it might be at least formally not correct and confusing when reading. But this is just my impression, it seems to work so it is ok.

Copy link
Contributor Author

@mbabker mbabker Jun 20, 2019

Choose a reason for hiding this comment

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

It's going to be fine. If my regex reading skills are OK, it's going to match if there is a "5.5.5-" prefix, a "mariadb-" prefix, or a "5.5.5-mariadb-" prefix, then the version number that is found in the first group after that is extracted into the $versionParts array. What we want is "10.0.8"; "5.5.5-Mariadb-10.0.8-xenial", "5.5.5-10.0.8-MariaDB", and "5.5.5-10.0.8-xenial" will all end up that way.

The only other way to do it "right" is going to result in a leaky abstraction (either the plugin has to issue a RDBMS specific query to get the version that way, or the $connection resource has to be made available).

Copy link
Member

@richard67 richard67 Jun 20, 2019

Choose a reason for hiding this comment

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

Right, it will work and return the right version so or so.
Unfortunately I don't have a MariaDB set up yet, so I could test only with code review. Would that be enough?

Copy link
Contributor Author

@mbabker mbabker Jun 20, 2019

Choose a reason for hiding this comment

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

I did it blind taking battle tested code from another framework and adapting it, I have exactly one MariaDB server at my disposal and it's on the one client server whose WHM was built with MariaDB enabled by default instead of MySQL (and they don't use Joomla, so clearly I can't abuse that server for testing).

Copy link
Member

@richard67 richard67 Jun 20, 2019

Choose a reason for hiding this comment

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

Let's wait and see, if we don't have a 2nd tester tomorrow I can set up a MariaDB. What keeps me from doing it is that I would ha eto specify a special port so it does not use the same as my MySQL currently used. So or so, code review looks good, I am sure it works asd desired.

Copy link
Contributor

@Quy Quy Jun 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Live server on Linux 3.10.0-957.10.1.el7.x86_64:

System Information: 5.5.60-MariaDB
Stats plugin: 5.5.60

Copy link
Contributor

@Quy Quy Jun 20, 2019

Choose a reason for hiding this comment

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

See this comment for possible MariaDB combinations: #25283 (comment)

Copy link
Member

@richard67 richard67 Jun 20, 2019

Choose a reason for hiding this comment

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

Until some version 5.5.x, MariaDB used the same numbering scheme as MySQL. Later they changed to 10.x.y. For the J4 installation that should be ok, because "5.5.60-MariaDB" is older than minimum version. For this PR here in staging I am not sure, it might be in fact confusing having 5.w.x and also 10.x.y in the statistics. But assuming that we will not allow installation of staging on MariaDB and not allow such old 5.x.y on J4, it should be practically ok. At the end we will maybe increase requirement to 10.4 anyway, as discussed in the other issue.

Copy link
Contributor Author

@mbabker mbabker Jun 20, 2019

Choose a reason for hiding this comment

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

For this PR here in staging I am not sure, it might be in fact confusing having 5.w.x and also 10.x.y in the statistics.

Having MySQL with 10.x (purely MariaDB) and 5.x (mixed MariaDB and MySQL) and 8.x (purely MySQL) version numbers isn't an issue. Actually, the 5.5.5 string is the worst bit of it all (which accounts for a not-so-insignificant number of the total MySQL stats) because we haven't the slightest idea what MariaDB backend is in use (whereas at least the 10.x ones we can tell the difference, there won't be a problem until MySQL gets to 10.x).

In the case of 5.5.60-MariaDB, there isn't a sane way to distinguish that one with the current architecture (neither in the stats plugin here or at the server level, "mariadb" just isn't an allowed thing right now because there isn't a "mariadb" database driver). It's one of those things we just live with.

Copy link
Member

@richard67 richard67 Jun 20, 2019

Choose a reason for hiding this comment

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

I fully agree.

{
$data['db_version'] = $versionParts['major'] . '.' . $versionParts['minor'] . '.' . $versionParts['patch'];
}

return $data;
}

/**
Expand Down