Code review comment for lp:~gl-az/percona-xtrabackup/BT-32889-history-on-server-2.2

Hey there Alexey...

On 9/24/2013 4:18 AM, Alexey Kopytov wrote:
> - let’s also rename xtrabackup_history to xtrabackup_info. It
> contains meta information about the backup where it’s stored, not
> the history. I think we can even deprecate xtrabackup_checkpoints
> with xtrabackup_info in future.
>> Do you want to rename the entire feature and all options (--no-info,
>> --info=<name>, --incremental-info-name, --incremental-info-uuid) as well
>> or just the table name?
> No, sorry I should have been more specific. I was referring to the file
> created in the backup directory. 'xtrabackup_info' reflects its content
> better than 'xtrabackup_history' IMO.
>
>>> - it also appears that xtrabackup_history will not be created if
>>> --history is not passed to innobackupex. I think it should always
>>> be created. I.e. update_history() should collect all info
>>> unconditionally, but just avoid executing the actual INSERT if
>>> $option_history if false.
>> Correct. There were some XB test case failures that would need to be
>> fixed up and permissions manipulated if we always created the
>> schema/filled the record. At that point I thought too that it may not be
>> a great idea to change the default assumption that XB was read only on a
>> server database by now having it start to write stuff back into it,
>> possibly requiring a user to change their backup user privileges. So, I
>> changed it to be entirely optional and any upgrades would be compatible
>> with any existing backup user privileges without having to explicitly
>> disable it or alter backup user privileges. I should have checked that
>> out w/ you first. That being said, if you do still want it on/creating
>> schema and filling the table and generating the file by default, I can
>> change it back and go fix up whatever failing tests occur as a result
>> (just adding --no-history/--no-info) and make sure that I give Hrvoje
>> enough info doc wise to try to avoid confusion.
> Sorry, I should have been more specific here to. I just meant to say
> that we have to create the xtrabackup_history (or xtrabackup_info) file
> regardless of whether --history is enabled.
>
> The reason we are implementing that file is
> https://bugs.launchpad.net/percona-xtrabackup/2.2/+bug/1133017
Ahh, OK, Now that makes a lot more sense. Yes, I can easily do this,
just have to shuffle around the means of data collection and writing in
update_history with the new DBD prepare and bind_param stuff so there
are not 20 if (!defined($option_history)).
>>> - here’s a more elegant and correct scrub_option() implementation:
>>>
>>> sub scrub_option {
>>> my $scrub = shift;
>>> my $command = shift;
>>>
>>> return $command =~ s/--$scrub=[^ ]+ /--$scrub=... /g
>>>
>>> }
>> Nice.
> Actually that won't work (I didn't verify it before posting). The
> following will work:
>
> sub scrub_option {
> my $scrub = shift;
> my $command = shift;
>
> $command =~ s/--$scrub=[^ ]+/--$scrub=.../g;
> return $command;
> }
>
>
>
Yup, I figured that out yesterday and fixed it myself. Actually, I just
removed the whole scrub_option call and just used the string replacement
since it is now a single line per scrubbed option:
$history_tool_command =~ s/--password=[^ ]+ /--password=... /g;
$history_tool_command =~ s/--encrypt-key=[^ ]+ /--encrypt-key=... /g;

You should be seeing another MP getting posted later on today with all
of the changes.

Thanks!

--
George O. Lorch III
Software Engineer, Percona
+1-888-401-3401 x542 US/Arizona (GMT -7)
skype: george.ormond.lorch.iii