Hi Chuck,
Status: not approved.
Requests:
[1] Please remember that the revision comment is the main source for the
docs team to make a changelog entry. It should tell 1. what was the
problem (so that a user can decide if he needs this bug fix), and 2. how
was it fixed (this does also often tell the user if it does really
address his problem).
[2] During my work on myisam_keycache_coverage.test I learned that this
turns off debugging unconditionally. It is better to save and restore
the original setting of the debug variable. That way you influence a
possibly active debugging session as little as possible. (two occurrences)
[3] Please add if (thd->killed) here too. If a kill comes in during
open_performance_schema_table(), we don't want to clear it. (two
occurrences)
Suggestions:
[4] I suggest to omit "the" here. (two occurrences)
Details:
Chuck Bell, 01.05.2009 02:16:
...
> 2703 Chuck Bell 2009-04-30
> BUG#39924 : Not possible to use backup logs after BACKUP has been interrupted
>
> This patch corrects the problem of not being able to write
> to the backup logs when a backup or restore is killed.
[1] Please remember that the revision comment is the main source for the
docs team to make a changelog entry. It should tell 1. what was the
problem (so that a user can decide if he needs this bug fix), and 2. how
was it fixed (this does also often tell the user if it does really
address his problem).
Your sentence might be taken as a breeze of a hint for 1. But it doesn't
say, who wasn't able to write to the log, nor how long it was blocked
(for one statement only? Or until end of session?). So the dimension of
the problem is not obvious.
...
> === modified file 'mysql-test/suite/backup/r/backup_logs.result'
...
> +Turn off debugging session.
> +SET SESSION debug="-d";
[2] During my work on myisam_keycache_coverage.test I learned that this
turns off debug unconditionally. It is better to save and restore the
original setting of the debug variable. That way you influence a
possibly active debugging session as little as possible. (two occurrences)
...
> === modified file 'mysql-test/suite/backup/t/backup_logs.test'
...
> +SET SESSION debug="+d,kill_backup";
[2a] At the top of the test case, please have:
let $debug= `SELECT @@debug`;
...
> +--echo Turn off debugging session.
> +SET SESSION debug="-d";
[2b] Here please have:
--echo # Reset debug variable to its original value.
--disable_query_log
eval SET debug= '$debug';
--enable_query_log
...
> +--echo Turn off debugging session.
> +SET SESSION debug="-d";
[2b] Here please have:
--echo # Reset debug variable to its original value.
--disable_query_log
eval SET debug= '$debug';
--enable_query_log
...
> === modified file 'sql/log.cc'
...
> @@ -812,10 +813,22 @@ bool Log_to_csv_event_handler::
...
> + /*
> + We need to override the check for a killed thread in the
[4] I suggest to omit "the" here. (two occurrences)
> + open_performance_schema_table() to allow the backup log
> + to be written even if backup is killed in the middle of
> + execution.
> + */
> + saved_killed_state= thd->killed;
> + if (thd->killed)
> + thd->killed= THD::NOT_KILLED;
> +
> if (!(table= open_performance_schema_table(thd, & table_list,
> & open_tables_backup)))
> goto err;
>
> + thd->killed= saved_killed_state;
> +
[3] Please add if (thd->killed) here too. If a kill comes in during
open_performance_schema_table(), we don't want to clear it. (two
occurrences)
...
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
