From: Dmitry Lenev
Date: March 10 2011 3:33pm
Subject: Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111
List-Archive: http://lists.mysql.com/commits/132774
Message-Id: <20110310153335.GA1933@bandersnatch>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable

Hello Magnus!

Here are my comments about your patch:

* Magnus Bl=E5udd <magnus.blaudd@stripped> [11/03/04 11:52]:
> #At file:///data0/magnus/mysql/format_section/5.5/ based on revid:jorge=
n.loland@stripped
>=20
>  3353 Magnus Bl=E5udd	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 ba=
sed 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() si=
nce tablespace
>          is now available in share and it's unnecessary for each handle=
r  to implement.
>          (the function could actually be removed totally now).
>        - Add test for combinations of TABLESPACE  and STORAGE with CREA=
TE 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 attribu=
tes, they are read.

...

> =3D=3D=3D 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=3DMyISAM;
> +SHOW CREATE TABLE t1;
> +Table	Create Table
> +t1	CREATE TABLE `t1` (
> +  `a` int(11) DEFAULT NULL
> +) /*!50100 TABLESPACE ts */ ENGINE=3DMyISAM DEFAULT CHARSET=3Dlatin1

...

> =3D=3D=3D 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=3DMyISAM;
> +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.

> =3D=3D=3D 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_d=
b_mode)
>    {
>      show_table_options=3D TRUE;
> -    /*
> -      Get possible table space definitions and append them
> -      to the CREATE TABLE statement
> -    */
> =20
> -    if ((for_str=3D file->get_tablespace_name(thd,0,0)))
> +    /* TABLESPACE and STORAGE */
> +    if (share->tablespace ||
> +        share->default_storage_media !=3D 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 =3D=3D HA_SM_DISK)
> +        packet->append(STRING_WITH_LEN(" STORAGE DISK"));
> +      if (share->default_storage_media =3D=3D HA_SM_MEMORY)
> +        packet->append(STRING_WITH_LEN(" STORAGE MEMORY"));
> +
> +      packet->append(STRING_WITH_LEN(" */"));
>      }
> =20
>      /*

Maybe it makes sense to also adjust store_schema_partitions_record()
code so it will no longer use handler::get_tablespace_name()?

...

> =3D=3D=3D 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=3D 8;=20
>          goto err;
>        }
> +
> +    if (format_section_fields)
> +    {
> +      const uchar field_flags=3D format_section_fields[i];
> +      const uchar field_storage=3D (field_flags & STORAGE_TYPE_MASK);
> +      const uchar field_column_format=3D
> +        ((field_flags >> COLUMN_FORMAT_SHIFT)& STORAGE_TYPE_MASK);
                                                  ^^^^^^^^^^^^^^^^^
I think the above should be:                      COLUMN_FORMAT_MASK

...

> =3D=3D=3D 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;
>=20

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 */
>=20

...

Otherwise, I am OK with your patch and think that it can be pushed
after considering/addressing the above issues.

--=20
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification