List:Commits« Previous MessageNext Message »
From:Rafal Somla Date:March 20 2008 10:38am
Subject:Re: bk commit into 6.0 tree (cbell:1.2600) BUG#34858
View as plain text  
Chuck,

Thanks for explanations. I approve the patch now. I made a suggestion to make 
report_ob_progress() an inline function. Make the change before push if you like.

Chuck Bell wrote:
> 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.
>

If you are "reasonably certain" then I trust your judgement (which means the 
responsibility is on you ;)). I just wanted to bring this point into your attention.

>>> +  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.
> 

Since WL#4296 is coming it is OK to leave the code as is if you prefer that.

>>> +      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.
> 

OK.

>>> +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.
>

OK, if it makes things easier for the coming WL#4296 code... But in that case I 
suggest that you move init_ob_progress to the header file and make it inline 
function.


>>> 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.

OK, that is a very good reason to have this code here.

> 
> 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 
> 

Rafal
Thread
bk commit into 6.0 tree (cbell:1.2600) BUG#34858cbell19 Mar
  • Re: bk commit into 6.0 tree (cbell:1.2600) BUG#34858Rafal Somla19 Mar
    • RE: bk commit into 6.0 tree (cbell:1.2600) BUG#34858Chuck Bell20 Mar
      • Re: bk commit into 6.0 tree (cbell:1.2600) BUG#34858Rafal Somla20 Mar