On 03/02/2011 09:39 AM, Sergey Vojtovich wrote:
> Hi Magnus,
>
> I have no major objections against this patch, so you have
> my approval. Though there're a couple of minor suggestions
> inline, which are up to you.
Thank you.
>
> On Wed, Feb 23, 2011 at 01:15:12PM -0000, Magnus Blåudd wrote:
>> #At file:///home/msvensson/mysql/5.5-format-section/ based on
> revid:jorgen.loland@stripped
>>
>> 3353 Magnus Blåudd 2011-02-23
>> 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).
> This assumption (handler::get_tablespace_name() could be removed)
> makes me thoughtful. If it still makes sense...
>
> In 6.0 partitions may go to different tablespaces, storage engines
> kept tablespace names. This patch suggests that tablespace names
> are to be kept by SQL. It shouldn't be a problem assuming
> partitioning handles it as well. Just a different concept from
> what we had in 6.0.
>
> Also I believe there is some progress wrt tablespaces in InnoDB.
>
> All in all no objections from my side. But interested parties should
> be informed, thus I CC Kevin and Mattias.
Good. But we don't need to think about how this would work in 6.0 since
that source it not used anymore.
>
> ...skip...
>> === 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-02-23 13:15:08 +0000
>> @@ -0,0 +1,112 @@
>
> ...skip...
>> +CREATE TABLE t1(a int) TABLESPACE ts STORAGE DISK ENGINE=MyISAM;
>> +ALTER TABLE t1 ADD COLUMN b int;
>> +SHOW CREATE TABLE t1;
>> +Table Create Table
>> +t1 CREATE TABLE `t1` (
>> + `a` int(11) DEFAULT NULL,
>> + `b` int(11) DEFAULT NULL
>> +) /*!50100 TABLESPACE ts */ ENGINE=MyISAM DEFAULT CHARSET=latin1
>> +DROP TABLE t1;
> Why no STORAGE DISK?
Yes, seems like it should be there. Will check why not.
>
> ...skip...
>> +CREATE TABLE t1(a int) ENGINE=MyISAM;
>> +ALTER TABLE t1 STORAGE MEMORY TABLESPACE ts;
>> +SHOW CREATE TABLE t1;
>> +Table Create Table
>> +t1 CREATE TABLE `t1` (
>> + `a` int(11) DEFAULT NULL
>> +) /*!50100 TABLESPACE ts STORAGE MEMORY */ ENGINE=MyISAM DEFAULT CHARSET=latin1
>> +ALTER TABLE t1 STORAGE DISK tablespace ts2;
> "tablespace" keyword is upper case in this test, but not here.
No, it's in lowercase...
+ALTER TABLE t1 STORAGE DISK tablespace ts2;
+SHOW CREATE TABLE t1;
>
>> === modified file 'sql/table.cc'
>> --- a/sql/table.cc 2011-01-28 12:28:15 +0000
>> +++ b/sql/table.cc 2011-02-23 13:15:08 +0000
>> @@ -750,6 +750,9 @@ static int open_binary_frm(THD *thd, TAB
>> const char **interval_array;
>> enum legacy_db_type legacy_db_type;
>> my_bitmap_map *bitmaps;
>> + uchar* extra_segment_buff= 0;
>> + const uint format_section_header_size= 8;
>> + uchar *format_section_fields= 0;
> Ehm, "uchar* " vs "uchar *" - who wins? There are a couple more of
> coding style violations in this patch - it would be nice to have
> them fixed.
Of course, will fix.
>
> ...skip...
>> + /* header */
>> + const uint format_section_length= uint2korr(next_chunk);
>> + const uint format_section_flags= uint4korr(next_chunk+2);
>> + /* 2 bytes unused */
> 2 bytes reserved in format section of extra data segment of
> meta-data file. It makes me think something is not OK with
> our DD.
DD = design document?
Yes, this is how it was implemented originally a couple of years ago,
and I want to keep compatibility with that.
On the other hand it's "only" two bytes for the whole table.
>
> ...skip...
>> @@ -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);
>> + DBUG_PRINT("debug", ("field flags: %u, storage: %u, column_format: %u",
>> + field_flags, field_storage, field_column_format));
>> + (void)field_storage; /* Used in MySQL Cluster */
>> + (void)field_column_format; /* Used in MySQL Cluster */
>> + }
> I assume the above code is blank. Why not to ifdef it out? What's the
> purpose to have it here at all?
1) Since MySQL Cluster based on 5.5 use the field_storage and
fiel_column_format values, writing it this way it's a minmial diff for
us in 5.5-cluster(we have enough diffs anyway).
2) It's a way to "reserve" those bits, anyone working with 5.5 or trunk
would see that the value are used in MySQL Cluster.
I should probably change the comment to say "Reserved by MySQL Cluster"
>
> ...skip...
>
> Regards,
> Sergey
