maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Diwas,

On Thu, Jul 02, 2015 at 04:26:58PM +0530, Diwas Joshi wrote:
> for the return type i am storing a string containing TABLE(<col
> definitions>). eg. forCREATE FUNCTION f1(a INT, b VARCHAR(11))
> RETURNS TABLE t1(id INT, name VARCHAR(11)) BEGIN set @a=3; END|
> we store
> TABLE t1(id INT, name VARCHAR(11))

Ok. 

> for the type of function we can store it as TYPE_ENUM_FUNCTION only and
> later see if it creates a problem, although we can always differentiate
> between the functions based on the return type stored above.

Yes, let's keep it this way for now.

> 
> I included the table alias with the return type string as you suggested.
> There is a warning that i am getting, which i am failing to figure the
> reason for but I am still trying.
> Please review.
>

The first thing: please add a testcase that checks what the code code does.
I can think of doing this:

CREATE table_function .... < whatever here> 
select * from mysql.proc where  name='table_function';

This will show what was stored in the system table.

This will also make it clear if/what warning gets produced.

Since you've changed CREATE FUNCTION table_function ... not to return error, I
guess some of testcases you previously added will need to be modified, too.

Overall the patch seems to be ok but I would like to look at the final version
of it before giving the approval.

> diff --git a/sql/sp.cc b/sql/sp.cc
> index 9fd17b0..3413ba1 100644
> --- a/sql/sp.cc
> +++ b/sql/sp.cc
> @@ -925,21 +925,66 @@ class Bad_db_error_handler : public Internal_error_handler
>    bzero((char*) &share, sizeof(share));
>    table.in_use= thd;
>    table.s = &share;
> -  field= sp->create_result_field(0, 0, &table);
> -  field->sql_type(result);
> -
> -  if (field->has_charset())
> +  if (sp->m_type == TYPE_ENUM_FUNCTION)
>    {
> -    result.append(STRING_WITH_LEN(" CHARSET "));
> -    result.append(field->charset()->csname);
> -    if (!(field->charset()->state & MY_CS_PRIMARY))
> +    field= sp->create_result_field(0, 0, &table);
> +    field->sql_type(result);
> +
> +    if (field->has_charset())
>      {
> -      result.append(STRING_WITH_LEN(" COLLATE "));
> -      result.append(field->charset()->name);
> +      result.append(STRING_WITH_LEN(" CHARSET "));
> +      result.append(field->charset()->csname);
> +      if (!(field->charset()->state & MY_CS_PRIMARY))
> +      {
> +        result.append(STRING_WITH_LEN(" COLLATE "));
> +        result.append(field->charset()->name);
> +      }
>      }
> +    delete field;
> +  }
> +  else if (sp->m_type == TYPE_ENUM_TABLE)
> +  {
> +    List_iterator_fast<Create_field> it(sp->m_cols_list);
> +    Create_field *cols_field;
> +    cols_field= it++;
> +    result.append(STRING_WITH_LEN("TABLE "));
> +    result.append(sp->m_table_alias.str);
> +    result.append(STRING_WITH_LEN("("));
> +    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(64);
> +      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);
> +      field->sql_type(tmp_result);
> +      result.append(tmp_result);
> +      cols_field= it++;
> +      if(cols_field)
> +        result.append(STRING_WITH_LEN(", "));
> +      delete field;
> +    }
> +    result.append(STRING_WITH_LEN(")"));
>    }
> -
> -  delete field;
>  }
>  
>  
> @@ -1020,7 +1065,8 @@ class Bad_db_error_handler : public Internal_error_handler
>    char definer_buf[USER_HOST_BUFF_SIZE];
>    LEX_STRING definer;
>    ulonglong saved_mode= thd->variables.sql_mode;
> -  MDL_key::enum_mdl_namespace mdl_type= type == TYPE_ENUM_FUNCTION ?
> +  MDL_key::enum_mdl_namespace mdl_type= type == (type == TYPE_ENUM_FUNCTION ||
> +                                                 type == TYPE_ENUM_TABLE)  ?
>                                          MDL_key::FUNCTION : MDL_key::PROCEDURE;
Looks reasonable.

>  
>    CHARSET_INFO *db_cs= get_default_db_collation(thd, sp->m_db.str);
> @@ -1036,7 +1082,8 @@ class Bad_db_error_handler : public Internal_error_handler
>    retstr.set_charset(system_charset_info);
>  
>    DBUG_ASSERT(type == TYPE_ENUM_PROCEDURE ||
> -              type == TYPE_ENUM_FUNCTION);
> +              type == TYPE_ENUM_FUNCTION ||
> +              type == TYPE_ENUM_TABLE);
>  
>    /* Grab an exclusive MDL lock. */
>    if (lock_object_name(thd, mdl_type, sp->m_db.str, sp->m_name.str))
> @@ -1117,7 +1164,8 @@ class Bad_db_error_handler : public Internal_error_handler
>  
>      store_failed= store_failed ||
>        table->field[MYSQL_PROC_MYSQL_TYPE]->
> -        store((longlong)type, TRUE);
> +        store((longlong)(type == TYPE_ENUM_TABLE ?
> +                         TYPE_ENUM_FUNCTION : type), TRUE);
>  
>      store_failed= store_failed ||
>        table->field[MYSQL_PROC_FIELD_SPECIFIC_NAME]->
> @@ -1145,7 +1193,7 @@ class Bad_db_error_handler : public Internal_error_handler
>        table->field[MYSQL_PROC_FIELD_PARAM_LIST]->
>          store(sp->m_params.str, sp->m_params.length, system_charset_info);
>  
> -    if (sp->m_type == TYPE_ENUM_FUNCTION)
> +    if (sp->m_type == TYPE_ENUM_FUNCTION || sp->m_type == TYPE_ENUM_TABLE)
>      {
>        sp_returns_type(thd, retstr, sp);
>  
> @@ -1153,7 +1201,7 @@ class Bad_db_error_handler : public Internal_error_handler
>          table->field[MYSQL_PROC_FIELD_RETURNS]->
>            store(retstr.ptr(), retstr.length(), system_charset_info);
>      }
> -
> +    String body_with_alias(64);
What is this for? It's not used anywhere.

>      store_failed= store_failed ||
>        table->field[MYSQL_PROC_FIELD_BODY]->
>          store(sp->m_body.str, sp->m_body.length, system_charset_info);
> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> index b53d939..d7d7d7b 100644
> --- a/sql/sql_yacc.yy
> +++ b/sql/sql_yacc.yy
> @@ -16392,12 +16392,6 @@ sf_tail:
>              Lex_input_stream *lip= YYLIP;
>  
>              lex->sphead->set_body_start(thd, lip->get_cpp_tok_start());
> -            
> -            if (Lex->sphead->m_type== TYPE_ENUM_TABLE) //to be removed later.
> -            {
> -              my_error(ER_NOT_SUPPORTED_YET, MYF(0), "Table functions");
> -              MYSQL_YYABORT;
> -            }
>            }
>            sp_proc_stmt /* $15 */
>            {

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