Hi Ingo,
Thank you for the review and the detailed comments. Pls find responses
below.
Ingo Strüwing wrote:
> Hi VN,
>
> this is a good basis for the bug fix. The code change is probably ok. I
> just like to see a little more tests. Please see below for more comments.
>
> V Narayanan, 24.07.2009 14:31:
>
> ...
>
>> 3039 V Narayanan 2009-07-24
>> Bug#45800 crash when replacing into a merge table and there is a duplicate
>>
>> A REPLACE in the MERGE engine is actually a REPLACE
>> into one (FIRST or LAST) of the underlying MyISAM
>> tables. So in effect we have the meta data of the
>> MERGE tables but we are inserting into a MyISAM table.
>>
>
>
> I (believe to) understand, what you mean with the last sentence.
> However, I would prefer to try to improve it for other readers. It might
> be better to say that the SQL layer (or "the server") works on the meta
> data of the MERGE table, while the real insert happens in the MyISAM table.
>
done!
>
>>
>> When a REPLACE into a MERGE table (and the REPLACE
>> conflicts with a duplicate in a child table) is done,
>> we try to access the duplicate key information for
>> the MERGE table. This information actually does not
>> exist since it is the child tables that have keys
>> and not the MERGE table. Hence this results in a crash.
>>
>
>
> For a reader, who is not familiar with the problem, it might be easier
> to understand, if the fact, that MERGE has no index, while MyISAM has a
> unique index, is put to the beginning of this paragraph.
>
done!
>
>>
>> The problem can be resolved by modifying the MERGE
>> engine to provide us the duplicate key information
>> directly, instead of replying on the metadata on the
>> server.
>>
>
>
> I suggest: "..., instead of just returning the MyISAM index number as
> the error key. Then the SQL layer (or "the server") does not try to
> access the key_info of the MERGE table, which does not exist.
>
done!
>
>>
>> The current patch modifies the MERGE engine to provide
>> the duplicate key information to the server.
>> @ include/myisammrg.h
>> Bug#45800 crash when replacing into a merge table and there is a
> duplicate
>>
>> Add a member to the st_mymerge_info structure that will
>> store the duplicate key offset in the MERGE table. This
>> offset will be the sum of the offset of the MyISAM table
>> and the offset into the MERGE table.
>>
>
>
> I'd say: "... the sum of the record offset of the MyISAM table within
> the MERGE table and the offset of the record within the MyISAM table."
>
> ...
>
done!
>> @ sql/handler.cc
>> Bug#45800 crash when replacing into a merge table and there is a
> duplicate
>>
>> With the current change the MERGE engine will be able to
>> return the duplicate key offset and will not use MAX_KEY
>> as a error indicator. This adversely affects the case
>> when we do a INSERT into a MERGE table that has a duplicate.
>>
>> An INSERT into a MERGE table having a duplicate causes the
>> write into the storage engine to fail. When we print an error
>> using print_keydup_error we access the key information from
>> the server, causing a crash.
>>
>> This problem can be resolved by checking for the case
>> when table->key_info is NULL.
>>
>
>
> I prefer not to have long problem descriptions in file comments. With
> the leading line ("Bug#45800 ...") one can find the revision and the
> global revision comment, which explains this sufficiently.
>
> IMHO file comments should say, what has been changed. They can add a
> short reason, but in general reasoning should be done in the global
> comment. (Or in the bug report). Again, that's IMHO.
>
> In summary, I would leave away the middle paragraph.
>
Ingo, I realized this change was not necessary. myrg_status returns the
key position on a MyISAM table. we could not have used this to index
this into table.key_info (errkey was primarily being used in the server
to derive information from table.key_info). we can set errkey = MAX_KEY
and keep using this as a sentinel value.
> ...
>
>> @ storage/myisammrg/myrg_info.c
>> Bug#45800 crash when replacing into a merge table and there is a
> duplicate
>>
>> We modify the myrg_status function to return the position of the
>> duplicate key. The duplicate key position in the MERGE table will
>> be the MyISAM file_offset and the offset within the MyISAM table
>> where the key is located.
>>
>
>
> That's correct in so far as the key is part of the record. But to be
> exact, the offsets describe the start positions of the records.
>
> ...
>
done!
I was in two minds on whether we should retain the setting of the errkey
here.
It is anyway being set to the sentinel MAX_KEY in ha_myisammrg::info.
But I decided
to let it remain for completeness and to retain the resemblance to the
way this works
for the MyISAM storage engine.
>> === modified file 'mysql-test/t/merge.test'
>> --- a/mysql-test/t/merge.test 2009-07-15 23:23:57 +0000
>> +++ b/mysql-test/t/merge.test 2009-07-24 12:31:47 +0000
>> @@ -1515,6 +1515,34 @@ insert into m1 (col1) values (1);
>>
>> drop table m1, t1;
>>
>> +--echo #
>> +--echo # Bug#45800 crash when replacing into a merge table and there is a
> duplicate
>> +--echo #
>> +
>> +--echo # Replace duplicate value in child table when merge table doesn't have
> key
>> +--echo # Create child table
>> +CREATE TABLE t1 (c1 INT PRIMARY KEY) ENGINE=MyISAM;
>> +--echo # Create Merge table
>> +CREATE TABLE m1 (c1 INT NOT NULL) ENGINE=MRG_MyISAM INSERT_METHOD=LAST
> UNION=(t1);
>> +INSERT INTO m1 VALUES (666);
>> +SELECT * FROM m1;
>> +--echo # insert the duplicate value into the merge table
>> +REPLACE INTO m1 VALUES (666);
>> +SELECT * FROM m1;
>> +DROP TABLE m1, t1;
>> +
>> +--echo # Insert... on duplicate key update (with duplicate values in the table)
>> +--echo # Create child table
>> +CREATE TABLE t1 (c1 INT PRIMARY KEY) ENGINE=MyISAM;
>> +--echo # Create Merge table
>> +CREATE TABLE m1 (c1 INT NOT NULL) ENGINE=MRG_MyISAM INSERT_METHOD=LAST
> UNION=(t1);
>> +INSERT INTO m1 VALUES (666);
>> +SELECT * FROM m1;
>> +--echo # insert the duplicate value into the merge table
>> +INSERT INTO m1 VALUES (666) ON DUPLICATE KEY UPDATE c1=c1+1;
>> +SELECT * FROM m1;
>> +DROP TABLE m1, t1;
>> +
>>
>
>
> These tests are good. However, I'd like to see these additional tests:
>
> - Do an INSERT (not REPLACE) one these tables so that a duplicate key
> error is reported. I want to see the error message in the result file.
> The sense is to verify that the key contents is correctly reported. This
> would prove that the new way to get at the duplicate key does indeed
> work. The tests above prove that the duplicate key is noticed as such,
> but not that its value is correctly retrieved. (Or tell me that we have
> that already in the test suite.)
>
This exists in the form of the test for Bug#41305 just above these set
of tests.
> - Repeat the tests on tables, where MERGE has a key, but MyISAM has more
> keys:
> CREATE TABLE t1 (c1 INT, c2 INT, UNIQUE c1, UNIQUE c2)
> CREATE TABLE m1 (c1 INT, c2 INT, UNIQUE c1)
> Provoke the duplicate key error on c2.
>
> - If it is possible to define MERGE and MyISAM with keys on different
> columns, repeat the tests on such tables too:
> CREATE TABLE t1 (c1 INT, c2 INT, UNIQUE c1)
> CREATE TABLE m1 (c1 INT, c2 INT, UNIQUE c2)
> Provoke the duplicate key error on the column that has no key in MERGE.
> If it is not possible to use such differently defined tables, have a
> test that shows, that it is not possible. (Or tell me that we have that
> already in the test suite.)
>
> ...
>
Added these tests!
>> === modified file 'storage/myisammrg/ha_myisammrg.cc'
>>
> ...
>
>> + if (flag & HA_STATUS_ERRKEY)
>> + {
>> + /*
>> + Set the index of the key and the offset of the key that caused
>> + the duplicate error. Here the mrg_info.dupp_key_pos will be the
>> + sum of the offset of the myisam file and the offset into the
>> + myisam file where the duplicate key is located.
>> + */
>> + errkey= mrg_info.errkey;
>>
>
>
> I suggest to extend the comment so that it explicitly says that "errkey"
> is set to the index number of the MyISAM tables (which must all have the
> same indexes), not of the MERGE table.
>
Since errkey is now being set to a sentinel value, I removed the
comments above
errkey.
> ...
>
> Regards
> Ingo
>
Thanks again for the detailed review Ingo!
Narayanan
