RE: bk commit into 6.0 tree (cbell:1.2600) BUG#34858

[Chuck Bell]

Hi Rafal,

Thanks for the great review and for jumping on this right away. I appreciate
the teamwork! :)

I have made all of the minor changes (plus a few I found myself -- all in
the documentation bits no code) and have addressed your concerns below. 

New patch is:

http://lists.mysql.com/commits/44265

> Chuck,
> 
> I don't see any major problems with this patch. See some 
> minor comments and questions below.

...

> > +  *backup_id= table->file->insert_id_for_cur_row;
> 
> Are you sure that table->file->insert_id_for_cur_row is the 
> same as the value assigned to the auto-increment column. I 
> vaguely recall that wierd things can happen to auto-increment 
> values - for example one can change the initial value and 
> increment step with server options. Would the above code be 
> consistent also in that case?

Yes, I am reasonably certain. I have done this several times in other parts
of the code and in my own mods to mysql. I will be happy to run a test to
satisfy your concerns. Let's discuss this in the morning and I'll run a
check for insurance purposes.

> > +  bzero(& table_list, sizeof(TABLE_LIST));
> > +  table_list.alias= table_list.table_name= 
> BACKUP_HISTORY_LOG_NAME.str;
> > +  table_list.table_name_length= BACKUP_HISTORY_LOG_NAME.length;
> >  
> > -  table= locking_thd->tables_in_backup->table;
> > -  table->use_all_columns();
> > +  table_list.lock_type= TL_WRITE_CONCURRENT_INSERT;
> >  
> > -  if (find_online_backup_row(table, backup_id))
> > -  {
> > -    ret= close_backup_progress_table(locking_thd);
> > -    DBUG_RETURN(1);
> > -  }
> > +  table_list.db= MYSQL_SCHEMA_NAME.str;
> > +  table_list.db_length= MYSQL_SCHEMA_NAME.length;
> > +
> > +  if (!(table= open_performance_schema_table(thd, & table_list,
> > +                                             & 
> open_tables_backup)))
> > +    goto err;
> > +
> 
> Above code shared with previous function. Perhaps good to 
> refactor it into a 
> helper function?

That is a good point and it would be a good thing however, if we use this
code for WL#4296, the implementation changes needed to these methods make
code sharing problematic. It turns out it boils down to only the first 8
lines of code that can be reused and only with some additional code. So I
deemed it not worth the effort. If you strongly disagree we can discuss it,
but I'd prefer to leave it as is.

> > +      if (val_time)
> > +      {
> > +        MYSQL_TIME time;
> > +        my_tz_OFFSET0->gmt_sec_to_TIME(&time, (my_time_t)val_time);
> > +
> 
> If we need value of type my_time_t, why the val_time 
> parameter is not of that 
> type? In general, perhaps we should use consistently 
> my_time_t type, not time_t.
> 
> I see that this is the same code as was there before, so ok 
> to push as it is 
> now. Above remark is only for the record.

Ok. Leaving it... ;)

> > +  if (result && !thd->killed)
> > +    sql_print_error("Failed to write to mysql.online_backup.");
> 
> I wonder why you don't use internationalized error message 
> and related error 
> reporting functions?

Very good point and good catch. I've added a new error message.

> > +int report_ob_progress(THD *thd,
> > +                       ulonglong backup_id,
> >                         const char *object,
> >                         time_t start,
> >                         time_t stop,
> 
> Do you still need this function given that there is 
> backup_progress_log_write() 
> doing basically the same thing?

Not technically for this patch, no we don't. However, it will be needed for
WL#4296. I would prefer to leave it in for now to keep the report_ob_* API
the same (yes, I could just do a rename) but this leaves it cleaner for
future work.

> > diff -Nrup a/sql/table.cc b/sql/table.cc
> > --- a/sql/table.cc	2008-02-25 11:05:59 -05:00
> > +++ b/sql/table.cc	2008-03-19 12:56:42 -04:00
> > @@ -33,6 +33,9 @@ LEX_STRING GENERAL_LOG_NAME= {C_STRING_W
> >  /* SLOW_LOG name */
> >  LEX_STRING SLOW_LOG_NAME= {C_STRING_WITH_LEN("slow_log")};
> >  
> > +extern LEX_STRING BACKUP_HISTORY_LOG_NAME;
> > +extern LEX_STRING BACKUP_PROGRESS_LOG_NAME;
> > +
> >  	/* Functions defined in this file */
> >  
> >  void open_table_error(TABLE_SHARE *share, int error, int db_errno,
> > @@ -241,6 +244,22 @@ TABLE_CATEGORY get_table_category(const 
> >      if ((name->length == SLOW_LOG_NAME.length) &&
> >          (my_strcasecmp(system_charset_info,
> >                        SLOW_LOG_NAME.str,
> > +                      name->str) == 0))
> > +    {
> > +      return TABLE_CATEGORY_PERFORMANCE;
> > +    }
> > +
> > +    if ((name->length == BACKUP_HISTORY_LOG_NAME.length) &&
> > +        (my_strcasecmp(system_charset_info,
> > +                      BACKUP_HISTORY_LOG_NAME.str,
> > +                      name->str) == 0))
> > +    {
> > +      return TABLE_CATEGORY_PERFORMANCE;
> > +    }
> > +
> > +    if ((name->length == BACKUP_PROGRESS_LOG_NAME.length) &&
> > +        (my_strcasecmp(system_charset_info,
> > +                      BACKUP_PROGRESS_LOG_NAME.str,
> >                        name->str) == 0))
> >      {
> >        return TABLE_CATEGORY_PERFORMANCE;
> >
> 
> Why do we need this change? Better if the patch changes as 
> few as possible in 
> the server code.

I added this because the open_performance_schema() method has an assert for
table->s->table_category == TABLE_CATEGORY_PERFORMANCE. Which experience
tells me needs to be handled properly. The proper way for this
table_category attribute to be set is in a call to get_table_category()
which is made from open_table_def() which is called from get_table_share()
(among other places) which originates from the handler code. So I felt that
changing it here was best. 

It also prepares us for WL#4296 if we indeed implement the backup logs as
logs rather than tables as they are now. 

I could simply set the parameter in the backup_progress code but I am not
comfortable with that (hence the "small hack" I mentioned on IRC).

Chuck