Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2740) Bug#38133

[Ingo Strüwing]

Hi Guilhem,

thanks for the review! Please see below.

Guilhem Bichot, 15.12.2008 14:23:
...
> Ingo Struewing a écrit, Le 12/10/2008 07:43 PM:
...
>>  2740 Ingo Struewing    2008-12-10
>>       Bug#38133 - Myisamlog test fails on Windows
...
>> === added file 'mysql-test/r/myisamlog_coverage.result'
>> --- a/mysql-test/r/myisamlog_coverage.result    1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/r/myisamlog_coverage.result    2008-12-10 18:43:34 +0000
> 
> It is impressive to see so much testing of error conditions.
> I remember some rule somewhere saying that all devs have to please dgcov
> (a.k.a. have good coverage of their push), my hat off to you for being
> one of the few (or maybe the only one) to honour this rule. It does give
> a feeling of confidence that you tested your work!!

Thanks. When we introduced this rule, I thought it was stupid to spend
so much time in testing every branch. After all, one catches a very
small class of problems only. But when I practiced it the first time, I
found a couple of problems, which I didn't expect to happen. This
encouraged me, to try it again. And yes, the same thing happened again.
I did even find crash paths. Adding tests that try to trigger a code
path, often reveals unexpected behavior of the code. Even when this
turns out to be correct, I do at least learn something.

So in summary, I learned that coverage testing is not a waste of time,
but pays off.

But I don't believe to be the only one doing it. If I remember
correctly, Monty was a strong advocate for this rule. So I believe, he
is doing it too.

...
>> +ERROR_INJECT("mi_examine_log_command")
>> +Unknown command 11 in logfile at position 0
>> +Trying to update MyISAM files according to log
>> 'VARDIR/master-data/myisam.log'
> 
> Here we have the stdout after stderr, unlike in previous test portions.
> This is a bit random, could you please add a fflush(stdout) before
> printing "Unknown command" to stderr?

Ok.

> 
>> === added file 'mysql-test/t/myisamlog_coverage.test'
>> --- a/mysql-test/t/myisamlog_coverage.test    1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/t/myisamlog_coverage.test    2008-12-10 18:43:34 +0000
>> @@ -0,0 +1,128 @@
...
>> +# NOTE: This test cannot run together with myisamlog.test.
...
> We had the same problem with tests reading the Maria transaction log and
...
> (for an example: see suite/maria/t/maria-no-logging.test in the latest

Ok. Thanks for the tip. I made it working.

However, I would have preferred some statement similar to FLUSH LOGS. If
myisam.log grows too big, there is no way around a server restart with
the current implementation.

After testing on Windows, it turned out that there is a flaw in the
server logging code. At least one of the tests creates a temporary
MEMORY table, which is later converted into a file based table. On
Linux, Maria is used, but on Windows MyISAM is used. MyISAM logs open of
the temporary table. When myisamlog tries to execute the log file, two
problems happen:

1. The temp file name is an absolute path and is appended to the data
directory. This was pretty easy to fix.

2. myisamlog cannot open the temporary table, because the table files
don't exist any more at this time.

So I wonder if it is correct to log temporary table actions at all? I
would happily exclude temporary tables, but this is probably beyond the
approval for this patch.

An (ugly) alternative would be to add include/not_windows to the test
case. Or even better (but still ugly) include/have_maria.

...
>> +    If one day someone notices a way how to do file lock type changes
>> +    on Windows without unlocking before taking the new lock, please
>> +    change this code accordingly to fix the race condition.
> 
> If not already done, could you please check with our Windows warriors
> (like Wlad) if they know such a way?

Done. Only a "not possible" response so far.

...
>     if (UnlockFileEx(hFile, 0, liLength.LowPart, liLength.HighPart, &ov))
>       DBUG_RETURN(0);
>     /*
>       For compatibility with fcntl implementation, ignore error,
>       if region was not locked
>     */
>     if (GetLastError() == ERROR_NOT_LOCKED)
>     {
>       SetLastError(0);
>       DBUG_RETURN(0);
>     }
> 
> You are free to choose if you want to do the same for the added
> UnlockFileEx() or not.

I changed to do the same. Though I wonder what possible errors might be
returned. But it's probably better to make the lock fail once too often,
than to risk a hang.

...
> if you enable myisamlog-coverage.test, you can remove several purecov
> markers accross the file.

I even have to. Otherwise I get gcov reports about unneeded annotations.

> 
>> @@ -219,24 +266,69 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
> 
>> +        /* purecov: begin tested */
>> +        /*
>> +          We found a closed file. It can only be closed due to a lack
>> +          of file descriptors. When a file is explicitly closed, its
>> +          information is removed from the tree and freed.
>> +          But this does not mean that there are still open files in
>> +          the tree. All other files could have been explicitly closed
>> +          meanwhile. So close a file only if there is still a lack of
>> +          file descriptors.
>> +        */
>> +        if (files_open >= mi_exl->max_files)
>> +        {
>> +          ERROR_INJECT("mi_examine_log_close_extra",
>> close_some_file(&tree););
> 
> I don't understand what you want to test with this one; it
> desynchronizes files_open with the actual number of open/closed files...?

I want close_some_file() to fail at this place. I come here only with
max_files > 0. But then files_open is also > 0. So close_some_file()
won't fail.

By testing with max_files == 1 *and* closing one file before calling
close_some_file(), I force it to fail here.

I have an error injection in close_some_file() itself. But when I use
this one together with max_files == 1, it fails earlier at another call
to close_some_file(), which I want to test too.

> 
>> +          if (close_some_file(&tree))
>> +          {
>> +            DBUG_PRINT("myisamlog", ("failed to close some file"));
>> +            goto com_err; /* No file to close */
>> +          }
>> +          files_open--;
>> +        }
> 
> Ok to push. You don't need a new review after fixing the little issues
> or answering my questions.
> Thanks for your patience in this long review cycle.
> 

Thank you! And sorry for the question about temporary tables, which
invalidates the approval, I believe.

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