Fix #18204 - Update getInnodbPluginVersion for MariaDB compatibility#20220
Fix #18204 - Update getInnodbPluginVersion for MariaDB compatibility#20220gs71 wants to merge 1 commit intophpmyadmin:QA_5_2from
Conversation
| global $dbi; | ||
|
|
||
| return $dbi->fetchValue('SELECT @@innodb_version;') ?: ''; | ||
| return ($dbi->isMariaDB() && $dbi->getVersion() >= 101000) ? '' : ($dbi->fetchValue('SELECT @@innodb_version') ? : ''); |
There was a problem hiding this comment.
| return ($dbi->isMariaDB() && $dbi->getVersion() >= 101000) ? '' : ($dbi->fetchValue('SELECT @@innodb_version') ? : ''); | |
| return ($dbi->isMariaDB() && $dbi->getVersion() >= 101000) ? '' : ($dbi->fetchValue('SELECT @@innodb_version;') ? : ''); |
| global $dbi; | ||
|
|
||
| return $dbi->fetchValue('SELECT @@innodb_version;') ?: ''; | ||
| return ($dbi->isMariaDB() && $dbi->getVersion() >= 101000) ? '' : ($dbi->fetchValue('SELECT @@innodb_version') ? : ''); |
There was a problem hiding this comment.
if you return empty it will break
phpmyadmin/libraries/classes/Operations.php
Line 525 in 6f66b76
was this variable also removed ?
There was a problem hiding this comment.
Yes, it was removed in MariaDB 10.10 : https://mariadb.com/docs/server/server-usage/storage-engines/innodb/innodb-system-variables
There was a problem hiding this comment.
Would you mind refactoring this so we have a clear path that the two variables are only displayed and fetched if the database version is the right one ?
Like you did in the other PR
There was a problem hiding this comment.
I just noticed that even in MySQL this variable is quite useless:
https://dev.mysql.com/doc/refman/8.4/en/innodb-parameters.html#sysvar_innodb_version
Maybe it's better to remove it completely?
There was a problem hiding this comment.
It depends if it useless since before the minimum MySQL version for phpMyAdmin or not
They say we can use @@version instead
There was a problem hiding this comment.
OK, then we can juste replace :
return $dbi->fetchValue('SELECT @@innodb_version;') ?: '';
With:
return $dbi->fetchValue('SELECT @@version;') ?: '';
There was a problem hiding this comment.
Probably yes, but it needs to be checked on the lowest version supported
There was a problem hiding this comment.
If you just replace it then it becomes useless, right? I think the point was to check whether this setting exists and is not empty.
There was a problem hiding this comment.
I think that the function getInnodbPluginVersion() is useless, since it is only used in libraries/classes/Operations.php as follows:
$innodbPluginVersion = $innodbEnginePlugin->getInnodbPluginVersion();
$innodb_file_format = '';
if (! empty($innodbPluginVersion)) {
$innodb_file_format = $innodbEnginePlugin->getInnodbFileFormat() ?? '';
}This block could just be replaced by:
$innodb_file_format = $innodbEnginePlugin->getInnodbFileFormat() ?? '';
And the function getInnodbFileFormat() could be optimized as:
public function getInnodbFileFormat(): ?string
{
global $dbi;
return (
($dbi->isMariaDB() && $dbi->getVersion() >= 100600)
|| ($dbi->isMySql() && $dbi->getVersion() >= 80000)
) ? '' : $dbi->fetchValue("SELECT @@innodb_file_format;");
}
Description
Starting from version 10.10 MariaDB removed the system variable 'innodb_version' (https://jira.mariadb.org/browse/MDEV-28554), so clicking on the 'Operations' link produces an error in the SQL errors log (if enabled):
ERROR 1193: Unknown system variable 'innodb_version' : SELECT @@innodb_version
Fixes #18204
Before submitting pull request, please review the following checklist:
Signed-off-byline as described in our DCO. This ensures that the work you're submitting is your own creation.Signed-off-by: Guido Selva guido.selva@gmail.com