RE: bzr commit into mysql-6.0-backup branch (cbell:2638) Bug#33364, Bug#35230, WL#4296

[Chuck Bell]

Hi,

I plan to do a new, restructured implementation of this worklog. Alas, but
this is one of my works that has fallen victim to many plagues including
diverging development trees. :|

I have supplied some answers to your queries in the interim.

Chuck 

>  > +FLUSH BACKUP LOGS;
>  > +--file_exists $MYSQLTEST_VARDIR/master-data/backup_history.log
>  > +--file_exists $MYSQLTEST_VARDIR/master-data/backup_progress.log
> 
> Oystein: So files will exists even if no logging is specified?
> If so, I think it should be checked that the files are empty.

Yes, it is a side effect of the existing server logging code. I can check
filesise.

>  > -/// Return global id of the backup/restore operation.
>  > +/// Return global history data of the backup/restore operation.
> 
> Oystein: I do not understand why the comment is changed.  The 
> method is 
> still
> returning just an ID isn't it?

Hmmm...will fix that.

>  >  inline
>  >  ulonglong Backup_restore_ctx::op_id() const
>  >  {
>  >

...

>  > +void Logger::report_engine(const char *engine_name)
>  > +{
>  > +  String str;    // engines string
>  > +  DBUG_ASSERT(m_state == READY || m_state == RUNNING);
>  > +
>  > +  str.length(0);
>  > +  if (m_op_hist->engine_name &&
>  > +      (strlen(m_op_hist->engine_name) > 0) &&
>  > +      (strlen(engine_name) > 0))
>  > +    str.append(", ");
>  > +  if (strlen(engine_name) > 0)
>  > +    str.append(engine_name);
>  > +  if (str.length() > 0)
>  > +  {
>  > +    if (m_op_hist->engine_name)
>  > +      delete m_op_hist->engine_name;
>  > +    m_op_hist->engine_name = new char[str.length()+1];
>  > +    strcpy(m_op_hist->engine_name, str.ptr());
>  > +  }
>  > +}
> 
> Oystein: I do not think the method above works as intended. 
> It seems the old
> content of m_op_hist->engine_name is overwritten, not appended to.
> Did you intend to copy old name into str at the beginning?

Actually, it does work as intended and engine (driver) names are appended.

...

>  > +  as the starting backup_id.
>  > +
>  > +  @todo Do we need a mutex to protect this call internally?
>  > +*/
> 
> Oystein: Why use a file and not the database when persisting 
> backup id? 
>   Seems
> like we should eat our own dog food here.

It was decided that a file would be easier to work with -- faster, no
locking to contend with, etc..

...

> Oystein: Irregular indentation.

Left over from old code -- not much I can do except run it through a filter.
I will try to remove them.

...

> Oystein: Mixing tabs and spaces for indentation here.  That should be
> avoided.

Same as above.

...

> Oystein: Tab on above line should be replaced with spaces.

Same as above

...

>  > +    /*
>  > +    Free memory for strings used
>  > +    */
>  > +    if (m_op_hist->backup_file)
>  > +      delete m_op_hist->backup_file;
>  > +    if (m_op_hist->binlog_file)
>  > +      delete m_op_hist->binlog_file;
>  > +    if (m_op_hist->command)
>  > +      delete m_op_hist->command;
>  > +    if (m_op_hist->engine_name)
>  > +      delete m_op_hist->engine_name;
>  > +    if (m_op_hist->user_comment)
>  > +      delete m_op_hist->user_comment;
> 
> Oystein: Why if-tests?  I thought delete on a zero pointer 
> would have no
> effect.

One would think so but in this case not all compilers deal well with that
premise. The check is a safety measure only and harmless IMHO.

...

>  > +void Logger::report_error_to_log(THD *thd,
>  > +                                    int error_num)
>  > +{
>  > +  m_op_hist->error_num= error_num;
>  >  }
> 
> Oystein: Why do all these functions have a THD parameter when 
> it is not
> used?

I was attempting to minimize changes to existing code. This code was copied
and not modified beyond that. Since I am restructuring now at the behest of
Rafal, I can take care of this too.

...

>  > +  m_op_hist->operation= BACKUP ? OP_BACKUP : OP_RESTORE;
> 
> Oystein: I was not able to determine what BACKUP is.  Please, explain.

BACKUP is defined as an enum as m_type. I think it should read:

m_op_hist->operation= m_type ? OP_BACKUP : OP_RESTORE;

Good catch!

> 
>  > +  m_op_hist->error_num= 0;
>  > +
>  > +  if (!m_op_hist->backup_file && (path.length > 0))
>  > +  {
>  > +    m_op_hist->backup_file= new char[path.length+1];
>  > +    strcpy(m_op_hist->backup_file, path.str);
>  > +  }
>  > +  if (!m_op_hist->command && (strlen(query) > 0))
>  > +  {
>  > +    m_op_hist->command= new char[strlen(query)+1];
>  > +    strcpy(m_op_hist->command, query);
>  > +  }
> 
> Oystein: backup_file and command was set to 0 just above.  
> Testing for it
> here seems a bit unecessary.

Agreed. Force of habit.

>  > +  pthread_mutex_lock(&LOCK_backupid);
>  > +  get_next_backup_id();
>  > +  m_op_hist->backup_id= m_next_id;
>  > +  pthread_mutex_unlock(&LOCK_backupid);
> 
> Oystein: I think it would be better if the mutex was handled within
> get_next_backup_id().  I see no reason to put that responsibility on
> the caller.

Curious. That would be ok I think. Will have to look into it.

...

>  > +bool MYSQL_BACKUP_LOG::open(const char *log_name,
>  > +                            enum_log_type log_type_arg,
>  > +                            const char *new_name,
>  > +                            enum cache_type io_cache_type_arg,
>  > +                            bool history)
>  > +{
> 
> Oystein: Lot's of cut&paste here.  Did you consider making a helper
> function that could be shared with MYSQL_LOG::open?

Yes, but it didn't fit. I will consider this when I refactor the code.

...

>  > +  return logger.backup_history_log_write(thd, history_data);
>  > +}
> 
> Oystein: When trying to understand the code, these global functions
> confused me a bit.  (It tooke me some time to understand that they
> were global).  I think it would be a bit clearer if they belonged to a
> class or namespace or something.

Yes, but this is the logger code lifting its ugly head above the slime. I
will consider this in the refactor.

...

>  > +class MYSQL_BACKUP_LOG: public MYSQL_LOG
> 
> Oystein: What does this inheritance actually give that could not be
> achieved by encapsulation?  As far as I can see, MYSQL_BACKUP_LOG does
> not override any methods from MYSQL_LOG.

It keeps the backup-specific code out of the MYSQL_LOG class which is
something else entirely. I sacrificed some clarity for separation.

Chuck