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

[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.)

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?)

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