List:Commits« Previous MessageNext Message »
From:Magnus Blåudd Date:March 2 2011 12:32pm
Subject:Re: bzr commit into mysql-5.5 branch (magnus.blaudd:3353) Bug#60111
View as plain text  
On 03/02/2011 12:35 PM, Sergey Vojtovich wrote:
> Magnus,
>
> On Wed, Mar 02, 2011 at 10:15:09AM +0100, Magnus Blåudd wrote:
>
> ...skip...
>>>>         - 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.
> The point here is not 6.0 codebase as such. The point is that we
> did the same thing before. And we did it slightly different.
> Nevertheless it is just something to be aware of, no action needed.

We also discovered that "partitions may go to different tablespaces, 
storage engines kept tablespace names" didn't work.

The main problem is some form of ALTER that Martin or Dmitry can tell 
more about.


>
>>> ...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;
> Well, I meant you use "TABLESPACE" (upper case) everywhere, except for
> this particular query, where you use "tablespace" (lower case).

OK

>
>>> ...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?
> data dictionary

Yes, and I originally suggested to put this new information in other 
places - but this solution was still preferred.

At least the bytes are zeroed so we could for example use them as 
"version" or flag bits if we need to extend this new "format section".

>
>> 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.
> Yes, I understand. Just constating the fact that it looks... ehm...
> interesting.
>
>>> ...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"
> My concern was: we execute commands that do not do anything
> useful. If it would make Cluster guys life easier, I'm Ok about
> this code hunk. But if I were not in the loop, I would be very
> surprised to see active code that does nothing.

They will certainly be optimized away.

Thanks!
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