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.
>
> 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.
>
> 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.
>
> 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."
...
> @ 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.
...
> @ 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.
...
> === 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.)
- 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.)
...
> === 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.
...
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
