maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Diwas,

Please find part #1 of the review below. I'll provide more soon.


> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index 2998972..a9dc162 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -2466,6 +2466,8 @@ static bool do_execute_sp(THD *thd, sp_head *sp)
>    lex->first_lists_tables_same();
>    /* should be assigned after making first tables same */
>    all_tables= lex->query_tables;
> +
> +  check_and_process_table_functions(thd, all_tables);
>    /* set context for commands which do not use setup_tables */
>    select_lex->
>      context.resolve_in_table_list_only(select_lex->
> @@ -5581,6 +5583,7 @@ static bool do_execute_sp(THD *thd, sp_head *sp)
>    }
>    THD_STAGE_INFO(thd, stage_query_end);
>    thd->update_stats();
> +  //clear_table_functions(thd, all_tables);

Please remove the comment.

>  
>    goto finish;
>  
> @@ -5697,7 +5700,7 @@ static bool do_execute_sp(THD *thd, sp_head *sp)
>      thd->mdl_context.release_transactional_locks();
>    }
>  #endif /* WITH_WSREP */
> -
> +  clear_table_functions(thd, all_tables);
>    DBUG_RETURN(res || thd->is_error());
>  }
>  
> @@ -5709,7 +5712,7 @@ static bool execute_sqlcom_select(THD *thd, TABLE_LIST *all_tables)
>    bool res;
>    /* assign global limit variable if limit is not given */
>    {
> -    SELECT_LEX *param= lex->unit.global_parameters();
> +      SELECT_LEX *param= lex->unit.global_parameters();

No need for this dummy change.

>      if (!param->explicit_limit)
>        param->select_limit=
>          new (thd->mem_root) Item_int((ulonglong) thd->variables.select_limit);
> @@ -9222,3 +9225,182 @@ CHARSET_INFO *find_bin_collation(CHARSET_INFO *cs)
>    }
>    return cs;
>  }
> +
> +/*
> +  @brief
> +  Check for table functions and create corresponding tables to be used in query.
> +
> +  @param  thd          Thread handle
> +  @param  table_list  A list of tables being processed in the query.
> +
> +  @return
> +    False  on success
> +  @retval
> +    TRUE   on error
> +*/
> +bool check_and_process_table_functions(THD *thd, TABLE_LIST *table_list)
> +{
> +  TABLE_LIST *tables = table_list;
> +  int err= 0;
> +  while(tables)
> +  {
> +    if (!tables->is_table_function || tables->table_name == NULL)
> +    {
> +      tables = tables->next_local;
> +      continue;
> +    }
> +    sp_head *sp;
> +    sp_name *name;
> +    LEX_STRING lex_db;
> +    LEX_STRING lex_name;
> +    lex_db.length= strlen(tables->db);
> +    lex_name.length= strlen(tables->table_name);
> +    lex_db.str= thd->strmake(tables->db, lex_db.length);
> +    lex_name.str= thd->strmake(tables->table_name, lex_name.length);
> +    name= new sp_name(lex_db, lex_name, true);
> +    name->init_qname(thd);
> +    int err=0;
Please switch err from int to bool.

> +
> +    if (!err && (sp = sp_find_routine(thd,TYPE_ENUM_FUNCTION, name,
Why assign 'err' and then immediately check it? The code looks redundant.

> +                                      &thd->sp_proc_cache, FALSE))) //check if the table is a table-valued function.

Please move the comment a few lines down, where the check is actually done.

> +    {
> +      if (sp->m_type == TYPE_ENUM_FUNCTION)
> +        my_error(ER_SP_TABLE_VALUE_EXPECTED, MYF(0));
> +      LEX *old_lex= thd->lex, newlex;
> +      thd->lex= &newlex;
> +      lex_start(thd);
> +      ulonglong saved_mode= thd->variables.sql_mode;
> +      Table_specification_st create_info;
> +      Alter_info alter_info;
> +      create_info.init();
> +      alter_info.reset();
> +      create_info.default_table_charset= NULL;
> +      TABLE_LIST *create_table;
> +      create_info.options|= HA_LEX_CREATE_TMP_TABLE;
> +      LEX_STRING tablename,dbname;
> +      if (strlen(sp->m_table_alias.str) > 0) {
> +        tablename.str= sp->m_table_alias.str;
> +        tablename.length= strlen(sp->m_table_alias.str);
> +      }
sp->m_table_alias is the alias of the return table of the table function.
When can it have zero length? Please add a comment about this here, and 
also add a testcase in the .test file

> +      else {
> +        tablename.str= tables->table_name;
> +        tablename.length= strlen(tables->table_name); 
> +      }
> +      dbname.str= tables->db;
> +      dbname.length= strlen(tables->db);
> +      Table_ident *new_ident= new Table_ident(thd, dbname, tablename, 0);
> +      if (!thd->lex->select_lex.add_table_to_list(thd, new_ident, NULL,
> +                                                  TL_OPTION_UPDATING,
> +                                                  TL_WRITE, MDL_EXCLUSIVE))
> +        err= 1;
> +      create_info.use_default_db_type(thd);
> +      thd->lex->query_tables->open_strategy= TABLE_LIST::OPEN_STUB;
> +      thd->lex->create_info.default_table_charset= NULL;
> +      thd->lex->name= null_lex_str;
> +      create_table= thd->lex->create_last_non_select_table= thd->lex->last_table();
> +      thd->lex->current_select= &thd->lex->select_lex;
> +      create_table_set_open_action_and_adjust_tables(thd->lex);
> +      alter_info.create_list= sp->m_cols_list;
> +      err= mysql_create_table(thd, create_table, &create_info, &alter_info);

Ok, so now it is using mysql_create_table instead of parsing the table
definition. This is a step forward.

However, I still think that mysql_create_table() does too much. It is a
function that used for regular CREATE TABLE statements. 

When I trace a query, I still see that the created temporary table uses InnoDB
storage engine.

I think, more effort is needed to make temporary table creation work in a more 
similar to how other parts of code create temporary tables.

I think I have also found a problem with how name resolution process works.
I'll elaborate in a separate email.

> +      thd->variables.option_bits|= OPTION_KEEP_LOG;

What is this for? please add a comment.

> +      if (!err)
> +        close_thread_tables(thd);
> +      execute_table_function(thd,sp,tables->table_func_params);
> +      thd->variables.sql_mode= saved_mode;
> +      lex_end(thd->lex);
> +      thd->lex= old_lex;
> +      if (strlen(sp->m_table_alias.str) > 0) {
> +        tables->table_name= sp->m_table_alias.str;
> +        tables->mdl_request.init(tables->mdl_request.key.mdl_namespace(),
> +                                 tables->db,sp->m_table_alias.str,
> +                                 tables->mdl_request.type,tables->mdl_request.duration);
> +      }
What does the above statement do? Please add a comment.

> +    }
> +    tables = tables->next_local;
> +  }
> +  close_thread_tables(thd);
> +  return err;
> +}

...

> +bool execute_table_function(THD *thd, sp_head *sp, List<Item> *value_list)
> +{
> +  /* bits that should be cleared in thd->server_status */
> +  uint bits_to_be_cleared= 0;
> +  if (sp->m_flags & sp_head::MULTI_RESULTS)
> +  {
> +    if (!(thd->client_capabilities & CLIENT_MULTI_RESULTS))
> +    {
> +      /* The client does not support multiple result sets being sent back */
> +      my_error(ER_SP_BADSELECT, MYF(0), sp->m_qname.str);
> +      return 1;
> +    }
> +    /*
> +      If SERVER_MORE_RESULTS_EXISTS is not set,
> +      then remember that it should be cleared
> +    */
> +    bits_to_be_cleared= (~thd->server_status &
> +                         SERVER_MORE_RESULTS_EXISTS);
> +    thd->server_status|= SERVER_MORE_RESULTS_EXISTS;
> +  }

Does the above make any sense here?

This is a piece of logic from code that executes stored *procedures*. A stored
procedure may have multiple SELECT statements. Executing this stored procedure
with "CALL sp(...)" may produce multiple result sets.  So this logic checks
that the client is able to receive multiple resultsets.

A stored *function* may not emit resultsets. It must return a scalar value, 
in our case it must return a table. 

> +  ha_rows select_limit= thd->variables.select_limit;
> +  thd->variables.select_limit= HA_POS_ERROR;
> +
> +  /*
> +    We never write CALL statements into binlog:
> +     - If the mode is non-prelocked, each statement will be logged
> +       separately.
> +     - If the mode is prelocked, the invoking statement will care
> +       about writing into binlog.
> +    So just execute the statement.
> +  */
> +  int res= sp->execute_procedure(thd, value_list);
> +
> +  thd->variables.select_limit= select_limit;
> +  thd->server_status&= ~bits_to_be_cleared;
> +
> +  if (res)
> +  {
> +    DBUG_ASSERT(thd->is_error() || thd->killed);
> +    return 1;     // Substatement should already have sent error
> +  }
> +
> +  my_ok(thd, (thd->get_row_count_func() < 0) ? 0 : thd->get_row_count_func());

What is the above for?

As far as I understand, the code in this function was based on
do_execute_sp(). Which is not a problem, however, do_execute_sp() is used to 
handle "CALL stored_proc" or compound statements, which should produce "Ok" if
they have not produced a resultset.

There is no such need when using a table function.  Upper query will produce a
resultset which it will return to the client.

> +  thd->reset_query_timer();

What is this for?

> +  thd->lex->unit.cleanup();
> +  thd->mdl_context.release_transactional_locks();
> +  thd->reset_for_next_command();
> +  return 0;
> +}
> \ No newline at end of file
...
> diff --git a/sql/table.h b/sql/table.h
> index 622a3b2..5eddd3d 100644
> --- a/sql/table.h
> +++ b/sql/table.h
> @@ -2010,6 +2010,7 @@ struct TABLE_LIST
>    List<Item>    used_items;
>    /* Sublist (tail) of persistent used_items */
>    List<Item>    persistent_used_items;
> +  List<Item>    *table_func_params; //parameters used for table function.
>    Item          **materialized_items;
>  
>    /* View creation context. */
> @@ -2059,7 +2060,7 @@ struct TABLE_LIST
>      from I_S should be done by create_sort_index() or by JOIN::exec.)
>    */
>    enum enum_schema_table_state schema_table_state;
> -
> +  bool is_table_function; //Check whether the table is actually a table function.

Do I get it right that a TABLE_LIST object that denotes a Table Function
invocation always has table_func_params != NULL? 

If yes, please remove 'bool is_table_function' and add a
'bool is_table_function()' function instead which checks for
table_func_params!=NULL.


>    /* Something like a "query plan" for reading INFORMATION_SCHEMA table */
>    IS_table_read_plan *is_table_read_plan;
>  
> @@ -2277,6 +2278,18 @@ struct TABLE_LIST
>      return false;
>    } 
>    void set_lock_type(THD* thd, enum thr_lock_type lock);
> +  void fill_table_func_params(List<Item> param_list)
> +  {
> +    table_func_params= new List<Item>;
> +    List_iterator_fast<Item> it(param_list);
> +    Item *param;
> +    param= it++;
> +    while(param)
> +    {
> +      table_func_params->push_back(param);
> +      param= it++;
> +    }
> +  }
>  
>  private:
>    bool prep_check_option(THD *thd, uint8 check_opt_type);

 
BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog