Hi Satya,
excellent so far! Please add functionality and test for the case that
none of the source tables has a .frm file. Since you copy a .frm file
from the first table only, it would be sufficient to show that missing
this one does not produce an error and the joined table gets no .frm.
Please find additional comments below.
Satya B, 07.11.2008 15:48:
> #At file:///home/satya/WORK/mysql-6.0-bug-36573/
>
> 2902 Satya B 2008-11-07
> Fix for Bug#36573 myisampack --join does not create destination table .frm
> file
>
> Problem:
> ========
> Myisampack --join doesn't create the destination table .frm file. The user has
> to copy one
> of the source table .frm file as destination .frm file for mysql server to
> recognize.
> This is just 'user-friendliness' issue.
Please keep the comment lines short.
I like to keep the problem description in past tense. The patch makes
the problem go away. But this is a matter of taste.
>
> How it was solved
> =================
>
> added create_dest_frm(source table, dest table) which copies the "FIRST" tables
> frm file
> as the destination frm file.
This is good information, but it is too technical for my taste. This
technical information is better moved to the file comment. For the
revision comment I would prefer something like "After successful join
and compression we copy the frm file from the first source table."
Please keep the comment lines short.
>
> It is invoked only after the compression succeeds.(after compress(...) )
>
> Note to maintain the existing behaviour of myisampack, the create_dest_frm
> returns always
> true, which means after the compression,while creating .frm file, if it finds
> an existing
> .frm file , it doesn't overwrite it and we return command status as success.
>
> Functionality added
> ===================
> ./myisampack --join=/home/satya/WORK/mysql-bin-5.1-bug-36573/var/test/t3
> /home/satya/WORK
> /mysql-bin-5.1-bug-36573/var/test/t2
> /home/satya/WORK/mysql-bin-5.1-bug-36573/var/test/t1
> creates /home/satya/WORK/mysql-bin-5.1-bug-36573/var/test/t3.frm (which is
> bascially
> copied from first table's frm
> /home/satya/WORK/mysql-bin-5.1-bug-36573/var/test/t2)
Please keep the comment lines short. Oh, did I mention it already? ;-)
For better reading, you could replace the long path names by something
shorter, e.g. myisampack --join=/path/t3 /path/t1 /path/t2
...
> +# ===== myisampack.3 =====
> +#Tests the myisampack join operation with an existing destination .frm,.MYI,.MDI
> +#the command should fail with exit status 2
> +DROP TABLE t1,t2,t3;
What a pity that we don't see the error message here. Please see below
for a possible way to fix it.
>
> === modified file 'mysql-test/t/myisampack.test'
> --- a/mysql-test/t/myisampack.test 2007-11-07 08:55:28 +0000
> +++ b/mysql-test/t/myisampack.test 2008-11-07 14:48:28 +0000
> @@ -31,3 +31,85 @@ FLUSH TABLES;
> --exec $MYISAMCHK -s --unpack $MYSQLTEST_VARDIR/master-data/test/t1
> CHECK TABLE t1 EXTENDED;
> DROP TABLE t1;
> +#
> +# Bug#36573 myisampack --join does not create destination table .frm file
> +#
> +
> +
I perfer to have a new test separated from former tests by a blank line,
and not to use blank lines within one test. For optical separation
within a test, I use lines with just a '#' at the beginning. This is a
matter of taste though.
But please don't use two consecutive blank lines anywhere within a test.
I take the header as belonging to the test.
> +#############################################################################
> +# Testcase myisampack.1: Positive test for myisampack --join
> +# To test myisampack --join operation creates .frm file
> +# If it creates .frm file, we will be able to access from mysql
> +# server
> +#############################################################################
> +-- echo # ===== myisampack.1 =====
> +
> +-- disable_warnings
> +DROP TABLE IF EXISTS t1;
> +-- enable_warnings
> +
> +CREATE TABLE t1(a int);
> +
> +INSERT INTO t1 VALUES(20);
> +
> +let $i=9;
> +while ($i)
> +{
> + INSERT INTO t1 SELECT ROUND(a*RAND()*10) from t1;
> + dec $i;
> +}
This is ok. A tip, just for your information: In cases when you have way
more than 9 iterations, you might not want to have all of them in the
result file. You can temporarily suppress the statement printout with
--disable_query_log and re-enable it with --enable_query_log.
Other useful maysqltest commands are in the test framework manual ->
mysqltest Language Reference -> mysqltest Commands
...
> +-- echo # ===== myisampack.3 =====
> +
> +--echo #Tests the myisampack join operation with an existing destination
> .frm,.MYI,.MDI
> +--echo #the command should fail with exit status 2
> +
> +--error 2
> +--exec $MYISAMPACK --join=$MYSQLTEST_VARDIR/master-data/test/t3
> $MYSQLTEST_VARDIR/master-data/test/t1 $MYSQLTEST_VARDIR/master-data/test/t2
To get the error message into the test result, you can append " 2>&1" to
the command line. Amazingly this works even on Windows.
I suggest to append this in general. That way new or modified error
messages pop up as result difference and must be explicitly acknowledged.
...
> @@ -762,6 +768,30 @@ static int compress(PACK_MRG_INFO *mrg,c
> DBUG_RETURN(-1);
> }
>
> +/*
> + Create FRM for the destination table for --join operation
> + Copy the first table FRM as the destination table FRM file. Doing so
> + will help the mysql server to recognize the newly created table
> + See Bug#36573
> +*/
> +static int create_dest_frm(char *source_table, char *dest_table)
The formatting is much better than in the former patch. There is still a
forgotten space after comma, but I don't want to be too picky.
But the function comment is still not correct. It should be a doxygen
enabled style. You find an example in the coding guidelines.
Also there should be two empty lines between the previous function and
the function comment. And one empty line between the function comment
and the function definition. I dislike the latter myself, but it is our
style and we need to have a common style.
Otherwise good work. Thank you!
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
