Hi Thava,
STATUS
------
Not approved.
REQUIRED
--------
1. Forward to user errors/warnings (other than ER_QUERY_INTERRUPTED) which
could happen when a statement is interrupted.
2. Do not add Logger::report_state_change() method - the
Logger::report_state() one should do the same task.
SUGGESTIONS
-----------
3. Do not add new Backup_log::set/get_state() methods. Instead fix the
implementation of Backup_log::state() to follow the specification (report
only state changes) or implement logic for detecting state changes in
Logger::report_state().
COMMENT
-------
I am not convinced by your argumentation that reporting additional
errors/warnings after query interruption is not needed or even confusing. If
a user hits Ctrl+C to interrupt an operation, depending on the operation a
complex shutdown sequence might be executed. During this shutdown sequence
errors might happen or warnings be generated and I think the user should see
them. This might require more work on our side but is not impossible to
implement. E.g., instead of calling copy_warnings() do an explicit loop
iterating over the warnings in ed_connection and add only these which are
different from ER_QUERY_INTERRUPTED.
DETAILS
-------
Thava Alagu wrote:
> #At file:///home/thava/mysql/repo/backup/
>
> 2888 Thava Alagu 2009-11-06
> BUG#47994 : Interruption of BACKUP command reported multiple times.
>
> When backup session is interrupted at certain stages, multiple duplicate
> warning messages are generated.
>
> 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.
> modified:
> mysql-test/suite/backup/r/backup_errors.result
> mysql-test/suite/backup/t/backup_errors.test
> mysql-test/suite/backup_engines/r/backup_interruption.result
> sql/backup/backup_info.cc
> sql/backup/backup_kernel.h
> sql/backup/kernel.cc
> sql/backup/logger.h
> sql/share/errmsg-utf8.txt
> sql/share/errmsg.txt
> sql/si_logs.cc
> sql/si_logs.h
> sql/si_objects.cc
> sql/si_objects.h
>
...
> === modified file 'sql/backup/logger.h'
> --- a/sql/backup/logger.h 2009-10-21 13:32:24 +0000
> +++ b/sql/backup/logger.h 2009-11-06 06:36:22 +0000
> @@ -68,6 +68,7 @@ public:
> void report_completed(time_t);
> void report_aborted(time_t, bool data_changed);
> void report_state(enum_backup_state);
> + void report_state_change(enum_backup_state);
> void report_vp_time(time_t, bool);
> void report_binlog_info(const st_bstream_binlog_info&);
> void report_master_binlog_info(const st_bstream_binlog_info&);
> @@ -263,7 +264,7 @@ void Logger::report_start(time_t when)
> report_error(log_level::INFO, m_type == BACKUP ? ER_BACKUP_BACKUP_START
> : ER_BACKUP_RESTORE_START);
> backup_log->start(when);
> - backup_log->state(BUP_RUNNING);
> + backup_log->set_state(BUP_RUNNING);
> }
>
>
> @@ -329,12 +330,14 @@ void Logger::report_aborted(time_t when,
>
>
> /**
> - Report change of the state of operation
> + Report the current state of operation.
>
> For possible states see definition of @c enum_backup_state
>
[2] I think there is no need to have both report_state() and
report_state_change(). In fact, we want to report new state only if it is
different from the old one. This is why specification of report_state() was
formulated as above and it should be correctly implemented - no need to
create a new method.
If you want to rename this method to report_state_change() then I would not
object. However, I think the current name is OK.
> @todo Consider reporting state changes in the server error log (as info
> entries).
> +
> + @see @c Logger::report_state_change()
> */
>
> inline
> @@ -343,11 +346,27 @@ void Logger::report_state(enum_backup_st
> DBUG_ASSERT(m_state == RUNNING || m_state == READY);
> DBUG_ASSERT(backup_log);
>
> - backup_log->state(state);
[3] According to the specification of Backup_log::state() function, it
should report only state changes, i.e. do nothing if the same state as
before is passed. Thus this method should work as expected if
Backup_log::state() works as expected. The problem is that the latter is not
the case.
> + backup_log->set_state(state);
> }
>
>
> /**
> + Report current state of operation only when there is state change.
> + This can be called multiple times without generating duplicate messages.
> +
> + For possible states see definition of @c enum_backup_state
> +*/
> +
> +inline
> +void Logger::report_state_change(enum_backup_state state)
> +{
> + if (!backup_log)
> + return;
> + if (backup_log->get_state() != state)
> + backup_log->set_state(state);
[3] According to its specification, backup_log->state() should report only
state changes. But if instead it reports given state each time it is
invoked, regardless of whether the state is different than last time or not
(as your set_state() method does), then this method could detect state
changes and call backup_log->state() only in case the state has changed. I
don't like adding get/set methods to backup_log because it turns it into an
object which stores backup state. But the purpose of backup_log is not to
store any information (for further retrieval), but to log information to
backup logs. Hence I'd implement the logic like this, without new methods in
Backup_log class:
if (state != m_reported_state)
{
backup_log->state(state);
m_reported_state = state;
}
This should work without any changes to si_log.cc. But even better would be
to fix the implementation of Backup_log::state() as I mentioned above.
> +}
> +
> +/**
> Report validity point creation time.
>
> @param[in] when the time of validity point
> @@ -443,7 +462,7 @@ int Logger::init(enum_type type, const c
>
> m_type= type;
> backup_log = new Backup_log(m_thd, (enum_backup_operation)type, query);
> - backup_log->state(BUP_STARTING);
> + backup_log->set_state(BUP_STARTING);
> m_state= READY;
> DEBUG_SYNC(m_thd, "after_backup_log_init");
> return 0;
>
...
> === modified file 'sql/si_logs.cc'
> --- a/sql/si_logs.cc 2009-05-21 13:17:37 +0000
> +++ b/sql/si_logs.cc 2009-11-06 06:36:22 +0000
> @@ -162,13 +162,24 @@ bool Backup_log::write_master_binlog_inf
> @todo Consider reporting state changes in the server error log (as info
> entries).
> */
> -void Backup_log::state(enum_backup_state state)
> +void Backup_log::set_state(enum_backup_state state)
[3] I would not change this. Note that the intent meaning of this function
is not to "set state" of the logger, but rather to report a change of
backup/restore operation state in the logs.
> {
> m_op_hist.state= state;
> logger.backup_progress_log_write(m_thd, m_op_hist.backup_id, "backup kernel", 0,
> 0, 0, 0, 0, get_state_string(state));
[3] To be consistent with the specification of the function (which says that
only state *changes* are to be reported) I'd include here the logic for
ignoring requests with the same state:
// Do nothing if state has not changed.
if (state == m_op_hist.state)
return;
> }
>
> +/**
> + Get current state of operation.
> +
> + For possible states see definition of @c enum_backup_state
> + */
> +enum_backup_state Backup_log::get_state()
[3] Backup_log encapsulates backup logging mechanism. I don't like it being
used to keep track of the state of backup/restore operation. I think it
should be maintained in upper layers of the code (i.e., in the backup kernel).
> +{
> + return m_op_hist.state;
> +}
> +
> +
> /**
> Report validity point creation time.
>
>
> === modified file 'sql/si_logs.h'
> --- a/sql/si_logs.h 2009-05-21 13:17:37 +0000
> +++ b/sql/si_logs.h 2009-11-06 06:36:22 +0000
> @@ -130,7 +130,8 @@ public:
> the backup_history log.
> */
> ulonglong get_backup_id() { return m_op_hist.backup_id; }
> - void state(enum_backup_state);
> + void set_state(enum_backup_state);
> + enum_backup_state get_state();
[3] My suggestion is to keep a single state() method and modify it to report
only state changes.
> void error_num(int code) { m_op_hist.error_num= code; }
> void binlog_start_pos(unsigned long int pos)
> {
>
> === modified file 'sql/si_objects.cc'
> --- a/sql/si_objects.cc 2009-11-04 14:18:21 +0000
> +++ b/sql/si_objects.cc 2009-11-06 06:36:22 +0000
> @@ -236,7 +236,8 @@ run_service_interface_sql(THD *thd, Ed_c
>
> bool rc= ed_connection->execute_direct(*query);
>
> - if (get_warnings)
> + /* Skip append warnings if query was killed to avoid duplicates. */
> + if (get_warnings && !thd->killed)
[1] This way we can miss important errors/warnings which could happen during
shutdown sequence of an interrupted command. The user should be informed
about these problems.
Rafal
