maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Diwas,

On Thu, Aug 27, 2015 at 09:23:55PM +0530, Diwas Joshi wrote:
> hello sergei, I am attaching two patches with this mail.
> MDEV-8389_first is the original patch with mysql_create_table() method used
> to create tables. I have fixed the crash that was occurring with queries
> like select name from f21('aaa') where name like '%foo%'; So, this patch is
> giving the results.
> MDEV-8389_second contains use of create_tmp_table() so far the table is
> being created, but the queries can't see it so it is still giving errors
> like table not present. I guess we'll have to fix the Name Resolution for
> queries to be able to use the created table.
> 
> Also both patches have fix for MDEV-8650.

I've looked at the patch. Unfortunately, it gives little confidence that it
will work in all cases. There are a lot of weird things. To name a few:

> +      tmp_table_param->schema_table = 1;
why?

...

> +        switch (field->sql_type)
> +        {
> +          case MYSQL_TYPE_VARCHAR:
> +          case MYSQL_TYPE_VAR_STRING:
> +          case MYSQL_TYPE_STRING:
> +          case MYSQL_TYPE_SET:
> +            field->pack_flag= 0;
> +            break;
...
This big switch statement is copied from Create_field::init_for_tmp_table. Why
is it copied (and not factored out?)  If there is a need to copy the code from
that function into here, why do you not also copy this part:

  pack_flag|=
    (maybe_null ? FIELDFLAG_MAYBE_NULL : 0) |
    (is_unsigned ? 0 : FIELDFLAG_DECIMAL);



> +      TABLE *vtable= create_virtual_tmp_table(thd, sp->m_cols_list);
...
> +      if (! (table = create_tmp_table(thd, tmp_table_param, *types,
> +                              (ORDER*) 0, 1, 1,
> +                              TMP_TABLE_ALL_COLUMNS, HA_POS_ERROR, 
> +                              sp->m_table_alias.str)))
why create_virtual_tmp_table and then create_tmp_table? 

'vtable' is created and then not freed. I look at the comment near 
create_virtual_tmp_table() definition and see:

    The table is created in THD mem_root, so are the table's fields.
    Consequently, if you don't BLOB fields, you don't need to free it.

But what if the table does have blob fields?

> +      table->file->extra(HA_EXTRA_IGNORE_DUP_KEY);
What's this? I grep for HA_EXTRA_IGNORE_DUP_KEY and find this explanation:

  HA_EXTRA_IGNORE_DUP_KEY:
  HA_EXTRA_NO_IGNORE_DUP_KEY:
    Informs the handler to we will not stop the transaction if we get an
    duplicate key errors during insert/upate.

Do we expect to get duplicate key errors when inserting into the table?

> +bool execute_table_function(THD *thd, sp_head *sp, List<Item> *value_list)
> +{
> +  /*
> +    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.
How is CALL statement relevant to executing a table function?

I think at this point it is clear that this patch needs more work before it 
can be pushed.

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