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 secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
