Hi Dmitry,
thanks for your review. I have one final questions, see below. Once I go
that answered, I'll proceed and push this.
>
>> === modified file 'sql/sql_show.cc'
>> --- a/sql/sql_show.cc 2011-02-08 15:47:33 +0000
>> +++ b/sql/sql_show.cc 2011-03-04 08:41:29 +0000
>> @@ -1379,17 +1379,24 @@ int store_create_info(THD *thd, TABLE_LI
>> if (!(thd->variables.sql_mode& MODE_NO_TABLE_OPTIONS)&&
> !foreign_db_mode)
>> {
>> show_table_options= TRUE;
>> - /*
>> - Get possible table space definitions and append them
>> - to the CREATE TABLE statement
>> - */
>>
>> - if ((for_str= file->get_tablespace_name(thd,0,0)))
>> + /* TABLESPACE and STORAGE */
>> + if (share->tablespace ||
>> + share->default_storage_media != HA_SM_DEFAULT)
>> {
>> - packet->append(STRING_WITH_LEN(" /*!50100 TABLESPACE "));
>> - packet->append(for_str, strlen(for_str));
>> - packet->append(STRING_WITH_LEN(" STORAGE DISK */"));
>> - my_free(for_str);
>> + packet->append(STRING_WITH_LEN(" /*!50100"));
>> + if (share->tablespace)
>> + {
>> + packet->append(STRING_WITH_LEN(" TABLESPACE "));
>> + packet->append(share->tablespace, strlen(share->tablespace));
>> + }
>> +
>> + if (share->default_storage_media == HA_SM_DISK)
>> + packet->append(STRING_WITH_LEN(" STORAGE DISK"));
>> + if (share->default_storage_media == HA_SM_MEMORY)
>> + packet->append(STRING_WITH_LEN(" STORAGE MEMORY"));
>> +
>> + packet->append(STRING_WITH_LEN(" */"));
>> }
>>
>> /*
>
> Maybe it makes sense to also adjust store_schema_partitions_record()
> code so it will no longer use handler::get_tablespace_name()?
Yes, I can do that. But since there are then no use of
handler::get_tablespace_name(), should I remove that function? My
suggestion is to remove it.
>
> ...
>
>> === modified file 'sql/table.cc'
>> --- a/sql/table.cc 2011-01-28 12:28:15 +0000
>> +++ b/sql/table.cc 2011-03-04 08:41:29 +0000
>
> ...
>
>> @@ -1438,6 +1483,18 @@ static int open_binary_frm(THD *thd, TAB
>> error= 8;
>> goto err;
>> }
>> +
>> + if (format_section_fields)
>> + {
>> + const uchar field_flags= format_section_fields[i];
>> + const uchar field_storage= (field_flags& STORAGE_TYPE_MASK);
>> + const uchar field_column_format=
>> + ((field_flags>> COLUMN_FORMAT_SHIFT)& STORAGE_TYPE_MASK);
> ^^^^^^^^^^^^^^^^^
> I think the above should be: COLUMN_FORMAT_MASK
Yes, good catch.
>
> ...
>
>> === modified file 'sql/table.h'
>> --- a/sql/table.h 2010-12-29 00:26:31 +0000
>> +++ b/sql/table.h 2011-03-04 08:41:29 +0000
>> @@ -609,6 +609,8 @@ struct TABLE_SHARE
>> }
>> enum row_type row_type; /* How rows are stored */
>> enum tmp_table_type tmp_table;
>> + enum ha_storage_media default_storage_media;
>> + char *tablespace;
>>
>
> Please add comments describing these two new members.
>
> Also I would have put them a bit lower in the list of members
> (e.g. after cached_row_logging_check member).
>
>> uint ref_count; /* How many TABLE objects uses this */
>> uint blob_ptr_size; /* 4 or 8 */
OK
>>
>
> ...
>
> Otherwise, I am OK with your patch and think that it can be pushed
> after considering/addressing the above issues.
>
Thanks!
/ Magnus
