STATUS
------
I have no requests, but several suggestions.
REQUESTS
--------
None
COMMENTARY
----------
Patch looks fine. I still have some of the same comments that I had
last time around, but I will not put up a fight.
I still not understand why it is supposed to be better to handle
backup specific issues in the server code than to add knowledge about
the server in the backup code. To me it seems like knowledge of
backup log formats and stuff like that should be an backup-internal
issue.
Another question I have that is really not particular to this patch,
is why all this inlining? It does not seem to be restricted to
particularly performance-critical code. Is there some guidelines for
when to inline functions or not? (The main disadvantage of inline
functions IMO is that it increases compile time since same code will
be compiled multiple times.)
SUGGESTIONS
-----------
(Numbers in brackets) refer to inline comments.
1. Logger.h: Two asserts for backup_log in same method? [1-2]
2. Still much cut&paste in log.cc. It would be great if some of the
copied code could be factored out.
3. Lines should be limited to 80 chars. [3-5]
4. Unclear intention of comment [6]
5. Of all the backup specific things in log.cc, I feel that
get_next_backup_id() is the function that least belong there.
There is nothing logging specific at all with that code, as far as
I can see. I think that this should be part of the backup module,
and the ID provided to the logging module as a parameter to the
logging functions.
6. Include si_logs.h in log.cc instead of log.h [7].
7. I like the si_logs abstraction, but since it is a backup specific
interface, I feel it should reside in the backup directory (maybe
in a files called backup_log.{h,cc}). Why do we need to hide the
LOGGER interface from the backup module? The two methods that are
used seems like a clean interface to me.
8. I think si_logs.h needs to either include or forward declare some
types (e.g., THD. LEX_STRING, time_t) to be self-contained. Or is
it OK to rely on that includers also include other header files,
and in the right sequence?
9. Fix typo [8].
DETAILS
-------
See inline. Comments/questions numbered [1] through [8].
Chuck Bell wrote:
...
> @@ -224,12 +188,14 @@ inline
> void Logger::report_start(time_t when)
> {
> DBUG_ASSERT(m_state == READY);
> + DBUG_ASSERT(backup_log);
> m_state= RUNNING;
>
> report_error(log_level::INFO, m_type == BACKUP ? ER_BACKUP_BACKUP_START
> : ER_BACKUP_RESTORE_START);
> - report_ob_time(m_thd, m_op_id, when, 0);
> - report_state(BUP_RUNNING);
> + DBUG_ASSERT(backup_log);
> + backup_log->start(when);
> + backup_log->state(BUP_RUNNING);
> }
>
[1] Why assert twice?
> /**
> @@ -244,19 +210,22 @@ void Logger::report_stop(time_t when, bo
> return;
>
> DBUG_ASSERT(m_state == RUNNING);
> + DBUG_ASSERT(backup_log);
>
> report_error(log_level::INFO, m_type == BACKUP ? ER_BACKUP_BACKUP_DONE
> : ER_BACKUP_RESTORE_DONE);
> - report_ob_time(m_thd, m_op_id, 0, when);
> - report_state(success ? BUP_COMPLETE : BUP_ERRORS);
> +
> + DBUG_ASSERT(backup_log);
> + backup_log->stop(when);
> + backup_log->state(success ? BUP_COMPLETE : BUP_ERRORS);
> + backup_log->write_history();
> m_state= DONE;
> }
[2] Why assert twice?
...
> +
> + /*
> + Fill in the data.
> + */
> + table->field[ET_OBH_FIELD_BACKUP_ID]->store(history_data->backup_id,
> TRUE);
> + table->field[ET_OBH_FIELD_BACKUP_ID]->set_notnull();
> + table->field[ET_OBH_FIELD_PROCESS_ID]->store(history_data->process_id,
> TRUE);
> + table->field[ET_OBH_FIELD_PROCESS_ID]->set_notnull();
> + table->field[ET_OBH_FIELD_BINLOG_POS]->store(history_data->binlog_pos,
> TRUE);
> + table->field[ET_OBH_FIELD_BINLOG_POS]->set_notnull();
> +
> + if (history_data->binlog_file)
> + {
> + if
> (table->field[ET_OBH_FIELD_BINLOG_FILE]->store(history_data->binlog_file,
> +
> strlen(history_data->binlog_file),
[3] Long line
> + system_charset_info))
> + goto err;
> + table->field[ET_OBH_FIELD_BINLOG_FILE]->set_notnull();
> + }
> +
> + table->field[ET_OBH_FIELD_BACKUP_STATE]->store(history_data->state,
> TRUE);
> + table->field[ET_OBH_FIELD_BACKUP_STATE]->set_notnull();
> + table->field[ET_OBH_FIELD_OPER]->store(history_data->operation, TRUE);
> + table->field[ET_OBH_FIELD_OPER]->set_notnull();
> + table->field[ET_OBH_FIELD_ERROR_NUM]->store(history_data->error_num,
> TRUE);
> + table->field[ET_OBH_FIELD_ERROR_NUM]->set_notnull();
> + table->field[ET_OBH_FIELD_NUM_OBJ]->store(history_data->num_objects,
> TRUE);
> + table->field[ET_OBH_FIELD_NUM_OBJ]->set_notnull();
> + table->field[ET_OBH_FIELD_TOTAL_BYTES]->store(history_data->size, TRUE);
> + table->field[ET_OBH_FIELD_TOTAL_BYTES]->set_notnull();
> +
> + if (history_data->vp_time)
> + {
> + MYSQL_TIME time;
> + my_tz_OFFSET0->gmt_sec_to_TIME(&time,
> (my_time_t)history_data->vp_time);
> +
> + table->field[ET_OBH_FIELD_VP]->set_notnull();
> + table->field[ET_OBH_FIELD_VP]->store_time(&time,
> MYSQL_TIMESTAMP_DATETIME);
> + }
> +
> + if (history_data->start)
> + {
> + MYSQL_TIME time;
> + my_tz_OFFSET0->gmt_sec_to_TIME(&time,
> (my_time_t)history_data->start);
> +
> + table->field[ET_OBH_FIELD_START_TIME]->set_notnull();
> + table->field[ET_OBH_FIELD_START_TIME]->store_time(&time,
> MYSQL_TIMESTAMP_DATETIME);
[4] Long line
> + }
> +
> + if (history_data->stop)
> + {
> + MYSQL_TIME time;
> + my_tz_OFFSET0->gmt_sec_to_TIME(&time, (my_time_t)history_data->stop);
> +
> + table->field[ET_OBH_FIELD_STOP_TIME]->set_notnull();
> + table->field[ET_OBH_FIELD_STOP_TIME]->store_time(&time,
> MYSQL_TIMESTAMP_DATETIME);
[5] Long line
> + }
> +
> + if (host)
> + {
> + if(table->field[ET_OBH_FIELD_HOST_OR_SERVER]->store(host,
> + strlen(host), system_charset_info))
> + goto err;
> + table->field[ET_OBH_FIELD_HOST_OR_SERVER]->set_notnull();
> + }
> +
> + if (user)
> + {
> + if (table->field[ET_OBH_FIELD_USERNAME]->store(user,
> + strlen(user), system_charset_info))
> + goto err;
> + table->field[ET_OBH_FIELD_USERNAME]->set_notnull();
> + }
> +
> + if (history_data->backup_file)
> + {
> + if (table->field[ET_OBH_FIELD_BACKUP_FILE]->store(
> + history_data->backup_file,
> + strlen(history_data->backup_file), system_charset_info))
> + goto err;
> + table->field[ET_OBH_FIELD_BACKUP_FILE]->set_notnull();
> + }
> +
> + if (history_data->user_comment)
> + {
> + if
> (table->field[ET_OBH_FIELD_COMMENT]->store(history_data->user_comment,
> + strlen(history_data->user_comment), system_charset_info))
> + goto err;
> + table->field[ET_OBH_FIELD_COMMENT]->set_notnull();
> + }
> +
> + if (history_data->command)
> + {
> + if (table->field[ET_OBH_FIELD_COMMAND]->store(history_data->command,
> + strlen(history_data->command), system_charset_info))
> + goto err;
> + table->field[ET_OBH_FIELD_COMMAND]->set_notnull();
> + }
> +
> + if (history_data->driver_name.length())
> + {
> + if (table->field[ET_OBH_FIELD_DRIVERS]->store(
> + history_data->driver_name.c_ptr(),
> + history_data->driver_name.length(), system_charset_info))
> + goto err;
> + table->field[ET_OBH_FIELD_DRIVERS]->set_notnull();
> + }
> +
> + /* log table entries are not replicated */
[6] I do not understand the purpose of this comment. Would another
function than ha_write_row be called if it was replicated?
> + if (table->file->ha_write_row(table->record[0]))
> + goto err;
...
>
> === modified file 'sql/log.h'
> --- a/sql/log.h 2008-06-27 20:56:54 +0000
> +++ b/sql/log.h 2008-08-21 15:52:16 +0000
> @@ -16,6 +16,50 @@
> #ifndef LOG_H
> #define LOG_H
>
> +#include "si_logs.h"
[7] You do not really need si_logs.h here. All you need is to forward
declare st_backup_history. Then log.cc could include si_logs.h
instead.
...
> +
> +/**
> + Structure for holding backup history data.
> +
> + This structure is used to collect the information needed to write a
> + single row of information to the backup_history log.
> +*/
> +struct st_backup_history
> +{
> + ulonglong backup_id; ///< the id for this row in the log
> + int process_id; ///< the process id of the backup/restore
> + enum_backup_state state; ///< current state of the operaiont
[8] Typo: operaiont
