List:Commits« Previous MessageNext Message »
From:Magnus Blåudd Date:March 2 2011 9:15am
Subject:Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111
View as plain text  
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

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