Hi Satya,
Good work. Almost ok to push. I'll check my reviewer's box so that no
further review is required by me. It would be great if you could
consider my below suggestions anyway.
Satya B, 11.11.2008 15:25:
...
> 2902 Satya B 2008-11-11
> Fix for Bug#36573 myisampack --join does not create destination
> table .frm file
...
> === 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-11 14:25:37 +0000
...
> +--exec $MYISAMPACK --join=$MYSQLTEST_VARDIR/master-data/test/t3
> $MYSQLTEST_VARDIR/master-data/test/t1 $MYSQLTEST_VARDIR/master-data/test/t2 2>&1
> +--error 0
> +--echo #Tests the myisampack join operation with an existing destination .frm file,
>
> +--echo #the command should return correct exit status(0) and
> +--echo #we should be able to query the table.
> +
> +SELECT COUNT(a) FROM t3;
What is the purpose of --error 0 ? It means "the next statement must not
fail". This is default anyway. Since you have some --echo between
--error and the next statement (SELECT), I'm not sure if it does apply
to it anyway. Usually --error is placed immediately before the affected
statement.
My suggestion is to remove --error 0 here and at other places to avoid
confusion.
...
> +--error S42S02
> +SELECT COUNT(a) FROM t3;
Using error numbers like S42S02 is a bit uncommon. I was not able to
find, what error this is. If you had used a numeric error, like 1146,
one can use the 'perror' utility to find its meaning. But usually we use
the ER_* symbols, which give a hint, what error is expected. For example
ER_NO_SUCH_TABLE.
P.S. I learned that the leading 'S' is only for mysqltest to distinguish
an SQLSTATE from an error number. Searching for 42S02 was successful.
So this means, it is not wrong to use such error codes, but still
uncommon in the test suite. I found a total of 7 occurrences in 3 of
>1000 test cases.
...
> +static int create_dest_frm(char *source_table, char *dest_table)
> +{
> + char source_name[FN_REFLEN], dest_name[FN_REFLEN];
> + int error;
> +
> + DBUG_ENTER("create_dest_frm");
> +
> + (void) fn_format(source_name, source_table,
> + "", FRM_EXT, MY_UNPACK_FILENAME|MY_RESOLVE_SYMLINKS);
> + (void) fn_format(dest_name, dest_table,
> + "", FRM_EXT, MY_UNPACK_FILENAME|MY_RESOLVE_SYMLINKS);
> +
> + (void) my_copy(source_name, dest_name, MYF(MY_DONT_OVERWRITE_FILE));
> +
> + return 0;
> +}
Lately you mentioned that your implementation does already accept the
situation that no source .frm file is present. I was curious, how you
did it. Now I notice, what I did overlook before. You suppress any error
reporting from my_copy(). That's good for "source file doesn't exist"
and "destination file exists". But you suppress all possible other
problems too. I agree that it makes sense here as we just want to try to
make the user's life easier. Copying of the .frm file is not vital for
the join operation. So that's ok. But it would be great if you could add
a comment before my_copy(), telling all these considerations so that a
reader understands that it is wanted behavior and not an oversight.
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
