Hi Guilhem,
Guilhem Bichot, 27.11.2008 21:42:
...
> Ingo Strüwing a écrit, Le 11/21/2008 05:59 PM:
...
> Normally my "could" or "can" are optional. They propose something, which
> you use if you like.
Ok. Then it's as usual. I was confused mainly about half a dozen
comments that I "could" use %p for address printing.
...
>> Guilhem Bichot, 14.11.2008 15:21:
>> ...
>>> Ingo Struewing a écrit, Le 11/13/2008 09:55 AM:
>> ...
>>>> 2730 Ingo Struewing 2008-11-13
>>>> Bug#38133 - Myisamlog test fails on Windows
...
>>>> sql/handler.cc
>>>> Bug#38133 - Myisamlog test fails on Windows
>>>> Changed abort() to DBUG_ABORT().
...
> But you did the change for a good reason (popup under Windows), and it
> is good to tell people about this reason, in case they wonder (at review
> time, or later when they study revision history); for me the file's
> revision comment is appropriate.
I learned, how to configure Windows to avoid the popup.
IMHO it is too tedious to get at the file comments. I won't expect
reasoning of changes there.
>
>> Anyway, the change was unrelated to the bug. So I reverted it.
>
> ok. It would have been fine to keep it, it's so small and unquestionable.
Not that unquestionable. You think it requires a reasoning. Or
explanation at least. And also it is not required with proper Windows
configuration.
...
> If I understand well, all the locks which you describe above happen in
> the Server when running with --external-locking?
> So you mean that the hang happens in the Server too, if using
> --external-locking? It's not a myisamlog-specific problem?
Yes. Adding --external-locking to myisamlog-master.opt makes the server
hang in my_lock() after the same locking pattern that made myisamlog to
hang.
...
> Now I'm confused. If the hang can also exist in the Server, why fix only
> myisamlog?
You know: The reviewer rules. Here again a quote from your review:
"
>>> In other words, I believe the bug could be in the user of mysys
>>> (myisamlog) more than in mysys. If the server didn't have any problem
>>> with mysys of before-the-patch, given that the server is much more used
>>> than myisamlog, I'd trust mysys more than myisamlog.
"
Personally, I'm not a friend of such arguments. For me it has
similarities to "I don't fasten my seat belt, I never had an incident."
Or "I don't need virus protection. I have never been infected". I admit
that it may be an unfair analogy. And I respect your longer experience
with MySQL. So I did just fix it in myisamlog.
OTOH I'm still having the concern that our test coverage is weak. Facing
the complexity of the system, we probably can't do much better. But
anyway, an argument like "it ran without problems for many years"
doesn't count much. The only test case that uses --external-locking is
backup_myisam1. And this is 6.0 only. And even this test doesn't run an
INSERT ... SELECT, which would show the problem. No wonder that the
problem has lived undetected for years.
...
> I had not understood that the bug was affecting the Server too.
Neither did I. My only worry was that my_lock() behaves differently on
different platforms. I thought that the main task of mysys is to
compensate differences between platforms.
mi_lock_database() tells a clear story that it expects from my_lock()
that a new lock replaces an existing lock. This isn't true on Windows.
I reported it as Bug#41124 - Server hangs on Windows with
--external-locking after INSERT...SELECT.
...
>> Ok, removed. I trust you that nobody will ever expect
>> mi_examine_log() to set my_errno on error.
>
> I believe no one should expect this, as it's not announced by the
> Doxygen comment of mi_examine_log(). For me, setting my_errno is part of
> the API. If it's not documented, it does not have to be...
It's always amazing to hear an experienced MySQL developer say that. I
have a limited comprehension for Monty's standpoint: "The source code is
the ultimate documentation". But Doxygen? How many MyISAM functions have
Doxygen comments? Not even mysys has complete Doxygen docs. At least I
don't know about them. It's often unclear, what to expect from mysys
functions. See our problem with my_lock().
...
>> At least I need to remove DBUG_ASSERT(!share->temporary). It crashed in
>> alter_table.test, mix2_myisam.test, temp_table.test, and
>> rpl_temp_table.test.
>
> It's excellent that you tested the idea. So indeed, it proves that
> F_WRLCK is used for temp table sometimes... Then could you please change
> this comment:
> if (lock_type == F_EXTRA_LCK) /* Used by TMP tables */
> by adding "sometimes" in the comment?
I'll change to "Temporary tables are usually locked with F_EXTRA_LCK.
But there are cases, where they use other lock types too."
"Sometimes used by TMP tables" might raise the wrong assumption that it
is common to lock them with other types. Or that other table types might
use that lock type too.
BTW, I wonder why _mi_decrement_open_count() needs to lock temporary
tables at all?
>
>> The updated patch: http://lists.mysql.com/commits/59568
>
> Could you please tell me against what revision id this patch is, so that
> I can apply it?
> I recently changed our commit emailer so that such information is
> automatically included in commit mails.
revision-id:jorgen.loland@stripped
But I won't expect many problems when applied on a more or less current
6.0-backup.
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
