Re: bk commit into 5.1 tree (mattiasj:1.2568) BUG#30695

[Mikael Ronström]

Hi,

16 okt 2007 kl. 10.29 skrev Mattias Jonsson:

> Hi Sergey and Mikael,
>
> I understand the change in changeset comment, and the smaller  
> testcase request (will fix that depending of additional changes in  
> table.cc).
>
> I can also understand the comment about testing the tmp-variable,  
> but since it already was tested up to three times and I could not  
> find have any obvious place to make the change, i just added a test  
> before the failing statement (just to make the change as small as  
> possible).
>
>
> Mikael, can you please help me understand the fix_partition_func?  
> (since I believe that is somehow using the "arena", but I can not  
> find where.)
>

You don't need to worry about the fix_partition_func since it's  
already safe-guarded against being called if there was an error
in the parser. What you can do is:

   if (!tmp)
   {
      outparam->part_info->is_auto_part...
      DBUG_PRIINT("info", ("auto....
      if (!work_part_info_used)
        tmp= fix_partition_func(...
   }


> Depending of this it is possible to rearrange the code to have  
> lesser tests.
>
> the affected code are in (mysql-5.1) table.cc around lines 1778-1815.
>
> Sergey, the two rows that you ask about in the testcase makes the  
> frm-file look like if it have a comment made in previous versions  
> of the server, and makes it possible to see what happens when  
> loading a "bad" frm-file (unescaped quotings in comment), since  
> this is what triggers the crash. I have commented these two rows  
> out, since it will probably not work on all operating systems. (Are  
> there any way to do this kind of tests, previous version->current  
> version, in the test framework?)
>

Not really as part of the standard framework but I know that Magnus  
Svensson is working on features like this so ask him what is  
available and what will be added in the
future.

Rgrds Mikael

> regards
> Mattias
>
>
> Sergey Vojtovich wrote:
>> Hi Mattias,
>> the patch is almost ok to push, but there are still some points to  
>> improve.
>> On Thu, Oct 11, 2007 at 01:34:47PM +0200, mattiasj@stripped wrote:
>>> Below is the list of changes that have just been committed into a  
>>> local
>>> 5.1 repository of mattiasj. When mattiasj does a push these  
>>> changes will
>>> be propagated to the main repository and, within 24 hours after the
>>> push, to the public repository.
>>> For information on how to access the public repository
>>> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>>>
>>> ChangeSet@stripped, 2007-10-11 13:34:41+02:00, mattiasj@mattiasj- 
>>> laptop.(none) +4 -0
>>>   Bug #30695: An apostrophe ' in the comment of the ADD PARTITION
>>>     causes the Server to crash.
>>>
>>>   Crashed because of the comment in the partition clause was  
>>> saved in
>>>   the frm file without escaping, causing the server to crash when  
>>> it was
>>>   read/parsed again.
>> I'd suggest to rephrase this statement:
>> "Accessing partitioned table with an apostrophe in partition  
>> options like
>> DATA DIRECTORY, INDEX DIRECTORY or COMMENT causes server crash.
>> Partition options were saved in .frm file without escaping. When  
>> accessing
>> such table it is not possible to properly restore partition  
>> information.
>> Crashed because there was no check for partition info parser  
>> failure."
>> Main point here is to note that DATA|INDEX DIRECTORY are affected  
>> as well.
>>>   Fixed by escaping quoted text in the partition info when  
>>> writing it to
>>>   the frm-file and added a check that it was able to parse the  
>>> partition
>>>   info before using it
>> Ok.
>>> --- a/mysql-test/t/partition.test	2007-07-02 20:11:52 +02:00
>>> +++ b/mysql-test/t/partition.test	2007-10-11 13:34:39 +02:00
>>> @@ -10,6 +10,32 @@ drop table if exists t1;
>>>  --enable_warnings
>>>   #
>>> +# Bug #30695: An apostrophe ' in the comment of the ADD  
>>> PARTITION causes the Server to crash.
>>> +#
>>> +# To verify the fix for crashing (on unix-type OS)
>>> +# uncomment the exec and error rows!
>> Usually I'm trying to make tests as small as possible. This helps to
>> understand in future what was this bug fix about. Following lines are
>> sufficient to repeat this problem:
>> CREATE TABLE t1(a INT) PARTITION BY LIST(c1)
>>   (PARTITION p1 VALUES IN(1) COMMENT '\'');
>> SELECT * FROM t1;
>> DROP TABLE t1;
>> But feel free to keep this test.
>>> +
>>> +CREATE TABLE t1 (
>>> +    d DATE NOT NULL
>>> +)
>>> +PARTITION BY RANGE( YEAR(d) ) (
>>> +    PARTITION p0 VALUES LESS THAN (1960),
>>> +    PARTITION p1 VALUES LESS THAN (1970),
>>> +    PARTITION p2 VALUES LESS THAN (1980),
>>> +    PARTITION p3 VALUES LESS THAN (1990)
>>> +);
>>> +
>>> +ALTER TABLE t1 ADD PARTITION (
>>> +PARTITION `p5` VALUES LESS THAN (2010)
>>> +COMMENT 'APSTART \' APEND'
>>> +);
>>> +#--exec sed 's/APSTART \\/APSTART  /' var/master-data/test/ 
>>> t1.frm > tmpt1.frm && mv tmpt1.frm var/master-data/test/t1.frm
>>> +#--error 1064
>> What are the above two lines about?
>>> --- a/sql/table.cc	2007-08-31 11:55:53 +02:00
>>> +++ b/sql/table.cc	2007-10-11 13:34:39 +02:00
>>> @@ -1782,7 +1782,8 @@ int open_table_from_share(THD *thd, TABL
>>>                                  outparam, is_create_table,
>>>                                  share->default_part_db_type,
>>>                                  &work_part_info_used);
>>> -    outparam->part_info->is_auto_partitioned= share- 
>>> >auto_partitioned;
>>> +    if (!tmp)
>>> +      outparam->part_info->is_auto_partitioned= share- 
>>> >auto_partitioned;
>>>      DBUG_PRINT("info", ("autopartitioned: %u", share- 
>>> >auto_partitioned));
>>>      /* we should perform the fix_partition_func in either local or
>>>         caller's arena depending on work_part_info_used value
>> Generally I agree with this fix, but this is against our coding  
>> standart -
>> we should never check variable value more than once if possible. I  
>> mean a
>> few lines after we have another if (!tmp && ...). Could you please  
>> check
>> with Mikael if it is possible to make this code better?
>> Regards,
>> Sergey
>
> -- 
> Mattias Jonsson, Software Engineer
> MySQL AB, www.mysql.com
>
> Are you MySQL certified?  www.mysql.com/certification