Hi Thava,
Status: Undecided.
Required:
1) Extended revision comment.
3) Explanation, why no existing tests are affected.
5) Use TRUE and FALSE instead of true and false.
Options:
2) New test might be unnecessary.
4) Change function signature.
Details:
Thava Alagu, 30.10.2009 01:44:
> #At file:///home/thava/mysql/repo/backup/
>
> 2853 Thava Alagu 2009-10-30
> Bug#47994 - Interruption of BACKUP command reported multiple times.
>
> Added additional checks to catch interruption of the command earlier
> to stop the backup kernel from executing further queries.
> Removed some duplicate pushing of warnings.
1) In general, in MySQl, patches, that fix bugs, shall have this
information in the revision comment:
1. What was the problem. So that neither the docs team, nor interested
developers need to look up the bug report for it. This short description
is missing here (even though one might deduce it form the bug report
synopsis). You might want to write "If BACKUP is interrupted, this
incident could be reported as multiple warnings of the same contents".
2. How was it solved. This information is present and good
understandable. Thanks for it.
> modified:
> mysql-test/suite/backup/r/backup_intr_errors.result
> mysql-test/suite/backup/t/backup_intr_errors.test
2) It is good to add tests for a bug fix. Unless they duplicate already
present tests. If I understand correctly, the bug was reported because
existing tests showed this behavior. So it seems that it could be
sufficient to run it against existing tests. Or do your tests add
something that is not covered by other tests?
3) OTOH I wonder, why only new tests are added, and no warnings are
removed from other tests. I am sure that I added some duplicate
"interrupted" warnings to some test cases. I think that this patch
should remove them. So I doubt that the problem is really fixed.
...
> === modified file 'sql/si_objects.cc'
...
> ///////////////////////////////////////////////////////////////////////////
> -bool check_user_access(THD *thd, const String *db_name)
> +bool check_user_access(THD *thd, const String *db_name, bool *rc)
4) I would like to urge you, to exchange error and result. IMHO, it is
more MySQLish and more to my liking that a function, which can fail,
should report success/failure through its return value. Additional
information (here: access_granted) should either be compatible with the
success/error status (and thus combined), or passed by a function
argument. Here we have two booleans. It won't be natural to combine
them, though this is often done as +/0/- return. So I suggest to let the
function return success/failure and have a 'access_granted' parameter.
Fortunately, the name of the function does not trick one to think it
would return the access status.
(And, btw, the parameter name 'rc' did not transport the idea if TRUE
means success or error.)
> {
> - uint elev_count;
> - uint user_count;
> + int elev_count;
> + int user_count;
> +
> + DBUG_ASSERT(rc);
> + *rc= false; /* Reset error code. */
5) From the coding guidelines: "Use TRUE and FALSE instead of true and
false in C++ code." (multiple places affected)
...
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
