Skip to content
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.

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

Possible bugs on Percona DB #16842

Closed
williamdes opened this issue Apr 20, 2021 · 5 comments
Closed

Possible bugs on Percona DB #16842

williamdes opened this issue Apr 20, 2021 · 5 comments
Assignees
Labels
Bug A problem or regression with an existing feature good first issue help wanted
Projects
Milestone

Comments

@williamdes
Copy link
Member

williamdes commented Apr 20, 2021

As a follow up of #16839 I pushed 41e0d4d to clean up the conditions. Also (d99da46 and aaaff9c are related commits)

I had to add Compatibility::isMySql and for me this is very disturbing because I did not hear that some MySQL features where not on PerconaDB

The work to do is:

  • proove the condition should allow PerconaDB
  • do the change
  • see that it works
  • make a PR

If my theory is true at the end of this process the Compatibility::isMySql should be removed

Please do all the changes on QA_5_1 using the old string comparing process, I will merge them into master afterwards

Here are the code occurrences:
image

@williamdes williamdes added the Bug A problem or regression with an existing feature label Apr 20, 2021
@williamdes williamdes added this to Needs triage in issues via automation Apr 20, 2021
@williamdes
Copy link
Member Author

cc @David-5-1

@williamdes williamdes self-assigned this Apr 29, 2021
williamdes added a commit that referenced this issue Apr 29, 2021
…sion

Signed-off-by: William Desportes <williamdes@wdes.fr>
@williamdes
Copy link
Member Author

Here is the work left

diff --git a/libraries/classes/Server/Privileges.php b/libraries/classes/Server/Privileges.php
index 778bd014c7..6ba0723886 100644
--- a/libraries/classes/Server/Privileges.php
+++ b/libraries/classes/Server/Privileges.php
@@ -863,7 +863,7 @@ class Privileges
             $host
         );
 
-        $isNew = ($serverType === 'MySQL' && $serverVersion >= 50507)
+        $isNew = ($serverType === 'MySQL' && $serverVersion >= 50507)//TODO
             || ($serverType === 'MariaDB' && $serverVersion >= 50200);
 
         $activeAuthPlugins = ['mysql_native_password' => __('Native MySQL authentication')];
@@ -2510,7 +2510,7 @@ class Privileges
                 if (! isset($row['password']) && isset($row['Password'])) {
                     $row['password'] = $row['Password'];
                 }
-                if (Util::getServerType() === 'MySQL'
+                if (Util::getServerType() === 'MySQL'//TODO
                     && $serverVersion >= 50606
                     && $serverVersion < 50706
                     && ((isset($row['authentication_string'])
@@ -3872,7 +3872,7 @@ class Privileges
 
         // Use 'SET PASSWORD' for pre-5.7.6 MySQL versions
         // and pre-5.2.0 MariaDB
-        if (($serverType === 'MySQL'
+        if (($serverType === 'MySQL'//TODO
             && $serverVersion >= 50706)
             || ($serverType === 'MariaDB'
             && $serverVersion >= 50200)
@@ -3962,9 +3962,9 @@ class Privileges
             $hostname
         );
 
-        $isNew = ($serverType === 'MySQL' && $serverVersion >= 50507)
+        $isNew = ($serverType === 'MySQL' && $serverVersion >= 50507)//TODO
             || ($serverType === 'MariaDB' && $serverVersion >= 50200);
-        $hasMoreAuthPlugins = ($serverType === 'MySQL' && $serverVersion >= 50706)
+        $hasMoreAuthPlugins = ($serverType === 'MySQL' && $serverVersion >= 50706)//TODO
             || ($this->dbi->isSuperUser() && $editOthers);
 
         $activeAuthPlugins = ['mysql_native_password' => __('Native MySQL authentication')];

@David-5-1
Copy link
Contributor

David-5-1 commented May 7, 2021

First of all, from https://www.percona.com/software/mysql-database/percona-server “Percona Server for MySQL® is a free, fully compatible, enhanced and open source drop-in replacement for any MySQL database.”, I tend to think there should be no reason to have a different behavior for matching versions of MySQL and Percona version.

FTR, all the tests I did were done using PMA 5.1.0 and one of the following Percona Server for MySQL versions:

  • 5.5.61-38.13
  • 5.6.41-84.1
  • 5.7.23-23
  • 8.0.18-9

Privileges.getHtmlForLoginInformationFields and mysql_old_password AuthPlugin (line 866)

I would add Percona as I see more diversity in plugins than just mysql_native_password in all the versions I have at hand and they all respond to the below query so $this->getActiveAuthPlugins() should be fine::

SELECT `PLUGIN_NAME`, `PLUGIN_DESCRIPTION` FROM `information_schema`.`PLUGINS`
WHERE `PLUGIN_TYPE` = "AUTHENTICATION";` so `$this->getActiveAuthPlugins()

Privileges.getDataForChangeOrCopyUser handling of MySQL 5.6.6 to 5.7.5 (line 2513)

I am not sure about what should be done for MySQL or Percona in 5.6.6-5.7.5 range…
I can tell that Password, plugin and authentication_string are all available in Percona 5.5.61 and 5.6.41 and Password is dropped in 5.7.23 and further so I wonder whether the condition should be extended to earlier MySQL releases.

Creating a user with a password through PMA in 5.6.41 results in empty auth_string, plugin set to mysql_native_password, and a non-empty Password. Failing short of understanding what this tries to get working, I tend to think Percona should be treated alike MySQL but cannot guaranty anything…

Privileges.getSqlQueriesForDisplayAndAddUser handling of MySQL 5.7.6 and later releases (line 3875)

I am not sure what this is really doing (and do not plan to investigate further tonight) but I get no error trying to create users with Percona 5.7.23 and 8.0.18.

Privileges.getFormForChangePassword handling of MySQL 5.7.6 (lines 3965 and 3967)

I also vote to add Percona as for Privileges.getDataForChangeOrCopyUser for the exact same reasons.

@williamdes
Copy link
Member Author

Thank you so much @David-5-1 !
Your response is so detailed, let us know if you find out something more

@williamdes
Copy link
Member Author

The change for getHtmlForLoginInformationFields was needed as it added other available password methods
It was needed for getFormForChangePassword for the same reasons

@williamdes williamdes added this to the 5.1.2 milestone Jul 22, 2021
williamdes added a commit that referenced this issue Jul 22, 2021
Signed-off-by: William Desportes <williamdes@wdes.fr>
williamdes added a commit that referenced this issue Jul 22, 2021
Signed-off-by: William Desportes <williamdes@wdes.fr>
issues automation moved this from Needs triage to Closed Jul 22, 2021
williamdes added a commit that referenced this issue Jul 22, 2021
…Compatibility::isMySqlOrPerconaDb should be used

Signed-off-by: William Desportes <williamdes@wdes.fr>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A problem or regression with an existing feature good first issue help wanted
Projects
issues
  
Closed
Development

No branches or pull requests

2 participants