Re: [Maria-developers] [Commits] 822aa21672b: MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir
Hi, Vicentiu! Looks fine, thanks. Few comments below. On Feb 13, vicentiu wrote:
revision-id: 822aa21672ba6650a31a3ec05ab5c1bce7a80c1e (mariadb-10.2.3-198-g822aa21672b) parent(s): b745ea489f0698c328383d9b8ec20d2f9777b1b8 author: Vicențiu Ciorbaru committer: Vicențiu Ciorbaru timestamp: 2017-02-13 14:44:00 +0200 message:
MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir
PART 2 of the fix adds the logic of not using password column, unless it exists. If password column is missing we attempt to use plugin && authentication_string columns.
diff --git a/mysql-test/r/no_password_column-mdev-11170.result b/mysql-test/r/no_password_column-mdev-11170.result new file mode 100644 index 00000000000..c3920a1b530 --- /dev/null +++ b/mysql-test/r/no_password_column-mdev-11170.result @@ -0,0 +1,170 @@ ... +# Reset to final original state. +# +drop table mysql.user; +create table mysql.user like backup_user; +insert into mysql.user select * from backup_user;
you could RENAME TABLE backup_user to mysql.user.
+drop table backup_user; +flush privileges; diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 568aff9ac92..7e03253ca98 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -2071,7 +2103,7 @@ static bool acl_load(THD *thd, const Grant_tables& tables) 8, 8, MYF(0));
/* check default role, if any */ - if (!is_role && user_table.num_fields() > DEFAULT_ROLE_COLUMN_IDX) + if (!is_role && user_table.default_role())
all these changes could've went into the first patch...
{ user.default_rolename.str= get_field(&acl_memroot, user_table.default_role()); @@ -4030,11 +4092,16 @@ static int replace_user_table(THD *thd, const User_table &user_table,
rights= user_table.get_access();
- DBUG_PRINT("info",("table fields: %d",user_table.num_fields())); - if (combo.pwhash.str[0]) + DBUG_PRINT("info",("table fields: %d", user_table.num_fields())); + /* If we don't have a password column, we'll use the authentication_string + column later. */ + if (combo.pwhash.str[0] && user_table.password()) user_table.password()->store(combo.pwhash.str, combo.pwhash.length, system_charset_info); - if (user_table.num_fields() >= 31) /* From 4.0.0 we have more fields */ + /* We either have the password column, the plugin column, or both. Otherwise + we have a corrupt user table. */ + DBUG_ASSERT(user_table.password() || user_table.plugin());
you cannot assert the table structure. ALTER TABLE shouldn't crash the server.
+ if (user_table.ssl_type()) /* From 4.0.0 we have more fields */ { /* We write down SSL related ACL stuff */ switch (lex->ssl_type) {
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei! Thanks for the review. I'll wait for buildbot to test it out and I'll pushj afterwards. On Mon, 13 Feb 2017 at 17:25 Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Vicentiu!
Looks fine, thanks. Few comments below.
On Feb 13, vicentiu wrote:
revision-id: 822aa21672ba6650a31a3ec05ab5c1bce7a80c1e (mariadb-10.2.3-198-g822aa21672b) parent(s): b745ea489f0698c328383d9b8ec20d2f9777b1b8 author: Vicențiu Ciorbaru committer: Vicențiu Ciorbaru timestamp: 2017-02-13 14:44:00 +0200 message:
MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir
PART 2 of the fix adds the logic of not using password column, unless it exists. If password column is missing we attempt to use plugin && authentication_string columns.
diff --git a/mysql-test/r/no_password_column-mdev-11170.result b/mysql-test/r/no_password_column-mdev-11170.result new file mode 100644 index 00000000000..c3920a1b530 --- /dev/null +++ b/mysql-test/r/no_password_column-mdev-11170.result @@ -0,0 +1,170 @@ ... +# Reset to final original state. +# +drop table mysql.user; +create table mysql.user like backup_user; +insert into mysql.user select * from backup_user;
you could RENAME TABLE backup_user to mysql.user.
Done.
+drop table backup_user; +flush privileges; diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 568aff9ac92..7e03253ca98 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -2071,7 +2103,7 @@ static bool acl_load(THD *thd, const Grant_tables& tables) 8, 8, MYF(0));
/* check default role, if any */ - if (!is_role && user_table.num_fields() > DEFAULT_ROLE_COLUMN_IDX) + if (!is_role && user_table.default_role())
all these changes could've went into the first patch...
The reason why this patch contains those extra checks is that I wanted to mess as little as possible with the current logic in the first patch, and only introduce the Grant_table class.
{ user.default_rolename.str= get_field(&acl_memroot, user_table.default_role()); @@ -4030,11 +4092,16 @@ static int replace_user_table(THD *thd, const
User_table &user_table,
rights= user_table.get_access();
- DBUG_PRINT("info",("table fields: %d",user_table.num_fields())); - if (combo.pwhash.str[0]) + DBUG_PRINT("info",("table fields: %d", user_table.num_fields())); + /* If we don't have a password column, we'll use the
authentication_string
+ column later. */ + if (combo.pwhash.str[0] && user_table.password()) user_table.password()->store(combo.pwhash.str, combo.pwhash.length, system_charset_info); - if (user_table.num_fields() >= 31) /* From 4.0.0 we have more fields */ + /* We either have the password column, the plugin column, or both. Otherwise + we have a corrupt user table. */ + DBUG_ASSERT(user_table.password() || user_table.plugin());
you cannot assert the table structure. ALTER TABLE shouldn't crash the server.
Kept it like this, as discussed.
participants (2)
-
Sergei Golubchik -
Vicențiu Ciorbaru