List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:March 10 2011 3:33pm
Subject:Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111
View as plain text  
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
Thread
bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111Magnus Blåudd23 Feb
Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111Magnus Blåudd2 Mar
  • Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111Sergey Vojtovich2 Mar
    • Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111Magnus Blåudd2 Mar
Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111Magnus Blåudd4 Mar
  • Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111Sergey Vojtovich4 Mar
Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111Dmitry Lenev10 Mar
  • Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111Magnus Blåudd18 Mar
    • Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111Dmitry Lenev21 Mar
      • Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111Magnus Blåudd21 Mar