Hello Magnus!
Here are my comments about your patch:
* Magnus Blåudd <magnus.blaudd@stripped> [11/03/04 11:52]:
> #At file:///data0/magnus/mysql/format_section/5.5/ based on
> revid:jorgen.loland@stripped
>
> 3353 Magnus Blåudd 2011-03-04
> Bug#60111 storage type for table not saved in .frm
> - Add new "format section" in extra data segment with additional table and
> column properties. This was originally introduced in 5.1.20 based MySQL
> Cluster
> - Remove hardcoded STORAGE DISK for table and instead
> output the real storage format used. Keep both TABLESPACE
> and STORAGE inside same version guard.
> - Implement default version of handler::get_tablespace_name() since
> tablespace
> is now available in share and it's unnecessary for each handler to
> implement.
> (the function could actually be removed totally now).
> - Add test for combinations of TABLESPACE and STORAGE with CREATE TABLE
> and ALTER TABLE
> - Add test to show that 5.5 now can read a .frm file created by MySQL Cluster
> 7.0.22. Although it does not yet show the column level attributes, they are
> read.
...
> === added file 'mysql-test/r/tablespace.result'
> --- a/mysql-test/r/tablespace.result 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/r/tablespace.result 2011-03-04 08:41:29 +0000
> @@ -0,0 +1,112 @@
> +CREATE TABLE t1(a int) TABLESPACE ts ENGINE=MyISAM;
> +SHOW CREATE TABLE t1;
> +Table Create Table
> +t1 CREATE TABLE `t1` (
> + `a` int(11) DEFAULT NULL
> +) /*!50100 TABLESPACE ts */ ENGINE=MyISAM DEFAULT CHARSET=latin1
...
> === added file 'mysql-test/t/tablespace.test'
> --- a/mysql-test/t/tablespace.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/t/tablespace.test 2011-03-04 08:41:29 +0000
> @@ -0,0 +1,122 @@
> +#
> +# BUG#60111 storage type for table not saved in .frm
> +#
> +
> +#
> +# Check that the table options for TABLESPACE and STORAGE
> +# are printed in SHOW CREATE TABLE
> +#
> +
> +# TABLESPACE only
> +CREATE TABLE t1(a int) TABLESPACE ts ENGINE=MyISAM;
> +SHOW CREATE TABLE t1;
> +DROP TABLE t1;
This is, in my opinion, the most controversial part of your patch.
The fact that we no longer simply ignore TABLESPACE and STORAGE
clauses for non-NDB tables (but store them) is going to be confusing
for users, I think.
But I see that MySQL Cluster already does this, so I guess you have
considered this already and found new behavior acceptable.
OK.
> === 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()?
...
> === 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
...
> === 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 */
>
...
Otherwise, I am OK with your patch and think that it can be pushed
after considering/addressing the above issues.
--
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com
Are you MySQL certified? http://www.mysql.com/certification
