Re: [Maria-developers] MDEV-8389 patch review (part#1)
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
participants (1)
-
Sergey Petrunia