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
