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
