Hi, Vicentiu! Pretty nice. I like the how it looks now. Thanks! Few small comments below, really minor. On Feb 13, vicentiu wrote:

revision-id: b745ea489f0698c328383d9b8ec20d2f9777b1b8 (mariadb-10.2.3-197-gb745ea489f0) parent(s): 52e0b585aaedd0f8f7191e0c0768d4be3c1c3496 author: Vicențiu Ciorbaru committer: Vicențiu Ciorbaru timestamp: 2017-02-13 12:13:36 +0200 message:

MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir

PART 1 of the fix requires a bit of refactoring to not use hard-coded field indices any more. Create classes that express the grant tables structure, without exposing the underlying field indices.

Most of the code is converted to use these classes, except parts which are not directly affected by the MDEV-11170. These however are TODO items for subsequent refactoring.

--- sql/sql_acl.cc | 1457 ++++++++++++++++++++++++++++++++++++++------------------ sql/structs.h | 2 +- 2 files changed, 996 insertions(+), 463 deletions(-)

diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index b8a06c22b62..568aff9ac92 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc

+#ifdef HAVE_REPLICATION + if (lock_type >= TL_WRITE_ALLOW_WRITE && thd->slave_thread && !thd->spcont) + { + /* + GRANT and REVOKE are applied the slave in/exclusion rules as they are + some kind of updates to the mysql.% tables. + */ + // TODO(remove-after-review) Note: there was a bug here in previous + // open_grant_tables version. The linking of tables to be opened was + // done according to a mask, starting from the end of the TABLES_LIST[TABLES_MAX] + // array. rpl_filter->tables_ok(0, tables) would always start from + // TABLES_LIST[0] and lookup the chain from there. If we would not open + // the User table (first table in the array) + // (as it happened in grant_reload()), the chain would stop there, hence + // the rpl_filter check would not lookup all the tables opened

I see. Right, it's a bug.

+ // + // This is the reason for the first_table_in_list member variable. + Rpl_filter *rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; + if (rpl_filter->is_on() && + !rpl_filter->tables_ok(0, &first_table_in_list->tl)) + DBUG_RETURN(1); + } +#endif + if (open_and_lock_tables(thd, &first_table_in_list->tl, FALSE, + MYSQL_LOCK_IGNORE_TIMEOUT)) + DBUG_RETURN(-1); + + /* The privilge columns vary based on MariaDB version. Figure out + how many we have after we've opened the table. */ + if (which_tables & Table_user) + m_user_table.compute_num_privilege_cols(); + if (which_tables & Table_db) + m_db_table.compute_num_privilege_cols(); + if (which_tables & Table_tables_priv) + m_tables_priv_table.compute_num_privilege_cols(); + if (which_tables & Table_columns_priv) + m_columns_priv_table.compute_num_privilege_cols(); + if (which_tables & Table_host) + m_host_table.compute_num_privilege_cols(); + if (which_tables & Table_procs_priv) + m_procs_priv_table.compute_num_privilege_cols(); + if (which_tables & Table_proxies_priv) + m_proxies_priv_table.compute_num_privilege_cols(); + if (which_tables & Table_roles_mapping) + m_roles_mapping_table.compute_num_privilege_cols();

couldn't you just walk the list (from first_table_in_list) and do compute_num_privilege_cols() on every table? Like ((Grant_tables*)tl)->compute_num_privilege_cols();

+ DBUG_RETURN(0); + } + + inline const User_table& user_table() const + { + return m_user_table; + }

I personally would just make user_table/etc members public, instead of creating accessors like that.

+ + inline const Db_table& db_table() const + { + return m_db_table; + } + + + inline const Tables_priv_table& tables_priv_table() const + { + return m_tables_priv_table; + } + + inline const Columns_priv_table& columns_priv_table() const + { + return m_columns_priv_table; + } + + inline const Host_table& host_table() const + { + return m_host_table; + } + + inline const Procs_priv_table& procs_priv_table() const + { + return m_procs_priv_table; + } + + inline const Proxies_priv_table& proxies_priv_table() const + { + return m_proxies_priv_table; + } + + inline const Roles_mapping_table& roles_mapping_table() const + { + return m_roles_mapping_table; + } + + private: + User_table m_user_table; + Db_table m_db_table; + Tables_priv_table m_tables_priv_table; + Columns_priv_table m_columns_priv_table; + Host_table m_host_table; + Procs_priv_table m_procs_priv_table; + Proxies_priv_table m_proxies_priv_table; + Roles_mapping_table m_roles_mapping_table; + + /* Bitmap of which tables we want to open. */ + const int which_tables;

you don't need it if you walk the list in open_and_lock()

+ /* Type of lock we want to acquire for the tables we are oppening. */ + const enum thr_lock_type lock_type;

not needed here, it's stored in every TABLE_LIST anyway

+ /* The grant tables are set-up in a linked list. We keep the head of it. */ + Grant_table_base *first_table_in_list; + /** + Chain two grant tables' TABLE_LIST members. + */ + static void link_tables(Grant_table_base *from, Grant_table_base *to) + { + DBUG_ASSERT(from); + if (to) + from->tl.next_local= from->tl.next_global= &to->tl; + else + from->tl.next_local= from->tl.next_global= NULL; + } +};

Regards, Sergei Chief Architect MariaDB and security@mariadb.org