Skip to content

Fix #18204 - Update getInnodbPluginVersion for MariaDB compatibility#20220

Open
gs71 wants to merge 1 commit intophpmyadmin:QA_5_2from
gs71:patch-1
Open

Fix #18204 - Update getInnodbPluginVersion for MariaDB compatibility#20220
gs71 wants to merge 1 commit intophpmyadmin:QA_5_2from
gs71:patch-1

Conversation

@gs71
Copy link
Copy Markdown
Contributor

@gs71 gs71 commented Mar 7, 2026

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:

  • Make sure you have read our CONTRIBUTING.md document.
  • Make sure you are making a pull request against the correct branch. For example, for bug fixes in a released version use the corresponding QA branch and for new features use the master branch. If you have a doubt, you can ask as a comment in the bug report or on the mailing list.
  • Every commit has proper Signed-off-by line as described in our DCO. This ensures that the work you're submitting is your own creation.
  • Every commit has a descriptive commit message.
  • Every commit is needed on its own, if you have just minor fixes to previous commits, you can squash them.
  • Any new functionality is covered by tests.

Signed-off-by: Guido Selva guido.selva@gmail.com

@gs71 gs71 changed the title Update getInnodbPluginVersion for MariaDB compatibility Fix #18204 - Update getInnodbPluginVersion for MariaDB compatibility Mar 7, 2026
global $dbi;

return $dbi->fetchValue('SELECT @@innodb_version;') ?: '';
return ($dbi->isMariaDB() && $dbi->getVersion() >= 101000) ? '' : ($dbi->fetchValue('SELECT @@innodb_version') ? : '');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
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') ? : '');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if you return empty it will break

$innodb_file_format = $innodbEnginePlugin->getInnodbFileFormat() ?? '';

was this variable also removed ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@gs71 gs71 Mar 17, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It depends if it useless since before the minimum MySQL version for phpMyAdmin or not
They say we can use @@version instead

Copy link
Copy Markdown
Contributor Author

@gs71 gs71 Mar 17, 2026

Choose a reason for hiding this comment

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

OK, then we can juste replace :

return $dbi->fetchValue('SELECT @@innodb_version;') ?: '';

With:

return $dbi->fetchValue('SELECT @@version;') ?: '';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably yes, but it needs to be checked on the lowest version supported

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@gs71 gs71 Mar 17, 2026

Choose a reason for hiding this comment

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

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;");
    }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indeed, please apply this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants