maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Diwas,

On Wed, Aug 05, 2015 at 08:47:28PM +0530, Diwas Joshi wrote:
> I have removed the errors from previous patch, I only got those errors when
> i compiled with valgrind. Everything seems to be working fine now. Also I
> have fixed the test case for select * from mysql.proc. Also added a
> function that removes the tables after they have been used. I have added
> the errors that we discussed about and also test cases corresponding to
> these errors. Please review.
> 

The big problems in the patch are:

1. check_and_process_table_functions() is at the wrong place.  I debug a query,
"SELECT * FROM tbl" , where 'tbl' is a regular table.

I see check_and_process_table_functions() invoke sp_find_routine() for
"test.tbl".  Why does this happen? 
The query makes it clear that the table is not a table function.
This is not acceptable.


2. Temporary table

2.1 The temporary table that is created uses InnoDB storage engine. This
doesn't follow any other kind of temporary table.  When execution engine
creates temporary tables (e.g. for VIEWs, or GROUP BY), these temporary 
tables are either Aria or MyISAM tables, depending on the server settings.
We should follow that.

2.2 Temporary table is created by building the

"CREATE TEMPORARY TABLE  (...)" 

string and passing it to the parser. Did you try looking into creating the
temp. table directly from the data strucutres you have?

No other code has to build "CREATE TABLE" statements and then parse them back.
I suspect Table Functions don't need to do this either.

The rest of the feedback is below:

> diff --git a/mysql-test/t/sp-error.test b/mysql-test/t/sp-error.test
> index ccf5b98..2ad4144 100644
> --- a/mysql-test/t/sp-error.test
> +++ b/mysql-test/t/sp-error.test
> @@ -3846,7 +3846,23 @@ DROP FUNCTION f1;
>  DROP FUNCTION f2;
>  DROP TABLE t1;
>  
> ---error 1235
> -SELECT * FROM f1('xyz');
> ---error 1064
> -SELECT * FROM t1, f1('xyz',t1.a);
> +delimiter |;
> +CREATE FUNCTION f1() RETURNS INTEGER
> +BEGIN
> +  RETURN 1;
> +END|
> +delimiter ;|
> +--error 1979
Please use error names, not error numbers. 
you can write
--error ER_SP_SCALAR_VALUE_EXPECTED.

> +SELECT * FROM f1();
> +DROP FUNCTION f1;
> +
> +delimiter |;
> +CREATE FUNCTION f1(a INT, b VARCHAR(11))
> +RETURNS TABLE t1(id INT, name VARCHAR(11))
> +BEGIN
> +INSERT INTO t1 SELECT id, name FROM t2 WHERE id = a;
> +END|
> +delimiter ;|
> +--error 1978
Same as above.

> +SELECT f1(1,'AA');
> +DROP FUNCTION f1;
> diff --git a/mysql-test/t/sp.test b/mysql-test/t/sp.test
> index 5c42df0..45db6a3 100644
> --- a/mysql-test/t/sp.test
> +++ b/mysql-test/t/sp.test
> @@ -9398,6 +9398,6 @@ END|
>  DELIMITER ;|
>  
>  --vertical_results
> -SELECT * FROM mysql.proc WHERE db = database() and name = 'f1';
> -
> +SELECT db, name, type, specific_name, language, sql_data_access, is_deterministic, security_type, param_list, returns, body, definer, sql_mode, comment, character_set_client, collation_connection, db_collation, body_utf8 FROM mysql.proc WHERE db = database() and name = 'f1';
The change is ok. Please also format the query so that its width 
is limited 80 chars.

> +SELECT * FROM f1(10,'XYZ');
>  DROP FUNCTION f1;
> diff --git a/sql/item_func.cc b/sql/item_func.cc
> index 3e78be8..ad18e2e 100644
> --- a/sql/item_func.cc
> +++ b/sql/item_func.cc
> @@ -6574,7 +6574,12 @@ void my_missing_function_error(const LEX_STRING &token, const char *func_name)
>      context->process_error(thd);
>      DBUG_RETURN(TRUE);
>    }
> -
> +  
> +  if (m_sp->m_type == TYPE_ENUM_TABLE){
> +          my_error(ER_SP_SCALAR_VALUE_EXPECTED, MYF(0));
> +          context->process_error(thd);
> +          DBUG_RETURN(TRUE);
> +  }
>    /*
>       A Field need to be attached to a Table.
>       Below we "create" a dummy table by initializing 
> @@ -6588,7 +6593,7 @@ void my_missing_function_error(const LEX_STRING &token, const char *func_name)
>    dummy_table->copy_blobs= TRUE;
>    share->table_cache_key = empty_name;
>    share->table_name = empty_name;
> -
> +  
Please don't make dummy changes like this.
>    if (!(sp_result_field= m_sp->create_result_field(max_length, name,
>                                                     dummy_table)))
>    {
> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index 2998972..6f94720 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -2467,6 +2467,8 @@ static bool do_execute_sp(THD *thd, sp_head *sp)
>    /* should be assigned after making first tables same */
>    all_tables= lex->query_tables;
>    /* set context for commands which do not use setup_tables */
> +  if (lex->sql_command == SQLCOM_SELECT)
> +    check_and_process_table_functions(thd, all_tables);

Why limit table function uses to SELECTs ? A user can write an INSERT.. SELECT
statement

INSERT INTO table SELECT * FROM table_func('argument');

or a multi-table UPDATE statement:

UPDATE regular_table, table_func('arg') SET regular_table.col= ...;

In general, there should not be a limit.  

I think, it's ok to introduce the limit within the scope of this MDEV entry.
In that case, you need to add a comment with 'TODO-DiwasJoshi:...' saying that the
limitation is temporary.

>    select_lex->
>      context.resolve_in_table_list_only(select_lex->
>                                         table_list.first);
> @@ -5697,7 +5699,8 @@ static bool do_execute_sp(THD *thd, sp_head *sp)
>      thd->mdl_context.release_transactional_locks();
>    }
>  #endif /* WITH_WSREP */
> -
> +  if (lex->sql_command == SQLCOM_SELECT)
> +   clear_table_functions(thd, all_tables);

This looks like an odd place to do cleanups. Let's get back to this once other
issues are resolved.

>    DBUG_RETURN(res || thd->is_error());
>  }
>  
> @@ -9222,3 +9225,185 @@ 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->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;
> +
> +    if (!err && (sp = sp_find_routine(thd,TYPE_ENUM_FUNCTION, name,
> +                                      &thd->sp_proc_cache, FALSE))) //check if the table is a table-valued function.
> +    {
> +      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;
> +      ulonglong sql_mode= 0 , saved_mode= thd->variables.sql_mode;
> +      String defstr; //query in defstr will be parsed to create temporary table.
> +      defstr.append(STRING_WITH_LEN("CREATE TEMPORARY TABLE "));
> +      defstr.append(tables->table_name);
> +      defstr.append(STRING_WITH_LEN(" ("));
> +      List_iterator_fast<Create_field> it(sp->m_cols_list);
> +
> +      TABLE table;
> +      TABLE_SHARE share;
> +      bzero((char*) &table, sizeof(table));
> +      bzero((char*) &share, sizeof(share));
> +      table.in_use= thd;
> +      table.s = &share;
> +      Create_field *cols_field;
> +      cols_field= it++;
> +      while (cols_field)
> +      {
> +        uint unused1= 0;
> +        sp_prepare_create_field(thd, cols_field);
> +        prepare_create_field(cols_field, &unused1, HA_CAN_GEOMETRY);
> +        uint field_length;
> +        Field *field;
> +        String tmp_result(512);
> +        field_length= cols_field->length;
> +        cols_field->flags= 0;
> +        field= make_field(table.s,                     /* TABLE_SHARE ptr */
> +                            (uchar*) 0,                   /* field ptr */
> +                            field_length,                 /* field [max] length */
> +                            (uchar*) "",                  /* null ptr */
> +                            0,                            /* null bit */
> +                            cols_field->pack_flag,
> +                            cols_field->sql_type,
> +                            cols_field->charset,
> +                            cols_field->geom_type, cols_field->srid,
> +                            Field::NONE,                  /* unreg check */
> +                            cols_field->interval,
> +                            cols_field->field_name);
> +        field->vcol_info= cols_field->vcol_info;
> +        field->stored_in_db= cols_field->stored_in_db;
> +        if (field)
> +          field->init(&table);
> +        defstr.append(cols_field->field_name);
> +        defstr.append(STRING_WITH_LEN(" "));
> +        field->sql_type(tmp_result);
> +        defstr.append(tmp_result);
> +        cols_field= it++;
> +        if(cols_field)
> +          defstr.append(STRING_WITH_LEN(", "));
> +        delete field;
> +      }
> +      defstr.append(STRING_WITH_LEN(")"));
> +      newlex.create_info.DDL_options_st::init();
> +      newlex.current_select= NULL;
> +      Parser_state parser_state;
> +      thd->variables.sql_mode= sql_mode;
> +      err= parser_state.init(thd, defstr.c_ptr_safe(), defstr.length());
> +      if (!err) {
> +        lex_start(thd);
> +        err= parse_sql(thd, &parser_state, NULL, TRUE) || thd->lex == NULL;
> +      }
> +      if (!err)
> +      {
> +        Table_specification_st create_info(thd->lex->create_info);
> +        Alter_info alter_info(thd->lex->alter_info, thd->mem_root);
> +        thd->lex->create_info.default_table_charset= NULL;
> +        SELECT_LEX *select_lex= &thd->lex->select_lex;
> +        TABLE_LIST *create_table= select_lex->table_list.first;
> +        if (append_file_to_dir(thd, &create_info.data_file_name,
> +                   create_table->table_name) ||
> +            append_file_to_dir(thd, &create_info.index_file_name,
> +                   create_table->table_name))
> +        {
> +          err= 1;
> +        }
> +        if (!(create_info.used_fields & HA_CREATE_USED_ENGINE))
> +           create_info.use_default_db_type(thd);
> +
> +        err= mysql_create_table(thd, create_table, &create_info, &alter_info);
> +        thd->variables.option_bits|= OPTION_KEEP_LOG;
> +      }
> +      thd->variables.sql_mode= saved_mode;
> +      lex_end(thd->lex);
> +      thd->lex= old_lex;
> +    }
> +    tables = tables->next_local;
> +  }
> +  if (!err)
> +    close_thread_tables(thd);
> +  return err;
> +}
> +
> +/*
> +  @brief
> +  Drop tables created for table functions.
> +
> +  @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 clear_table_functions(THD *thd, TABLE_LIST *table_list)
> +{
> +  TABLE_LIST *tables = table_list;
> +  int err= 0;
> +  while(tables)
> +  {
> +    if (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;
> +    if (!err && (sp = sp_find_routine(thd,TYPE_ENUM_FUNCTION, name,
> +                                      &thd->sp_proc_cache, FALSE)) &&

This function is supposed to just close the temporary tables. Why does it need
to search for SPs? 

> +        sp->m_type== TYPE_ENUM_TABLE) //check if the table is a table-valued function.
> +    {
> +      if(tables->table)
> +        close_temporary_table(thd,tables->table,TRUE,TRUE);
> +      if (thd->is_error())
> +        err= 1;
> +      tables->table= NULL;
> +    }
> +    tables= tables->next_local;
> +  }
> +  return err;
> +}


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