maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx