Hi Sergey,
when you explain in more detail, what was wrong, and how the patch
influences the situation, I'll probably approve it. Please see below for
more detailed questions.
Sergey Vojtovich, 29.01.2009 12:51:
> #At file:///home/svoj/devel/bzr-mysql/mysql-5.0-bugteam-bug32047/
Just FYI: seemingly you don't have the current version of the mysql
plugin. It continues this line like "based on revid:...". Not a problem
to me, I just wanted to note it.
>
> 2733 Sergey Vojtovich 2009-01-29
> BUG#32047 - 'Spurious' errors while opening MERGE tables
>
> Accessing well defined MERGE table may return an error
> stating that the merge table is incorrectly defined. This
> happens if MERGE child tables were accessed before and we
> failed to open another incorrectly defined MERGE table in
> this connection.
>
> The problem was incorrect usage of my_errno.
I wish you had continued this comment with an explanation, why it was
wrong, and how it was possible that a later statement inherited my_errno
from the former one. Even from your patch it is not obvious to me. You
don't clear my_errno at the beginning of myrg_open() as suggested by
Dmitri. But I believe you that your change fixes the problem too. But
then Dmitri's analyze must be wrong. So I'd like to learn what really
happened.
> modified:
> myisammrg/myrg_open.c
> mysql-test/r/merge.result
> mysql-test/t/merge.test
>
> per-file messages:
> myisammrg/myrg_open.c
> Setting my_error and attempting to check it's value later in a high-level
> function is generally a bad idea. Because there may be lower level function
> calls, which may set my_errno, in between.
Hm. I guess you mean "take its value as an indicator for failure"?
Otherwise it's common practice to check my_errno for a detailed error
code when a function returns non-zero. But if the caller of myrg_open()
made the mistake to take my_errno as an error indicator, I would have
expected this to be fixed in the patch too...
Formally, I suggest to move this comment to the global revision comment.
I personally like to see *what* was changed in the file comments and
have all explanations in the global comment.
...
BTW. The merge code looks pretty different in 5.1 and up. If the
problems exists there too, a different patch might be required.
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
