On 9/23/2013 9:58 AM, Alexey Kopytov wrote:
> Review: Needs Fixing
>
> George,
>
>     - I propose we rename ‘mysql_version’ to ‘server_version’, since we
>       support not only MySQL.
Makes sense, can do.
>     - 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?
>     - the table does not define the character set as we discussed
>       previously. This means the character set will depend on
>       server/connection settings.
Very true, forgot about that. Will add utf8 specifier.
>     - update_history() is vulnerable to SQL injection attacks :) what if
>       something like this is passed to --history, for example:
>
>       --history="foo'; DROP DATABASE customers".
>
>       Please use DBI prepared statements. This way you don’t have to roll
>       your own quoting in other places.
Yes, I actually had a discussion w/ Vadim on whether or not we are 
concerned w/ SQL injection at that level and the answer was not really 
so I just stuck with the original prototype code I had that used all 
literals. I suppose it wouldn't hurt and would be more proper to use DBD 
prepare and bind values instead of all the literal bits.
>
>     - 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.
>     - for start_time / end_time it’s better to use time() and then
>       convert values it returns to TIMESTAMP with FROM_UNIXTIME():
>
>    $history_start_time = time();
>    ...
>
>    # start_time
>    $insert_vals .= ", FROM_UNIXTIME($history_start_time)";
>
>    # end_time
>    $tmp = time();
>    $insert_vals .= ", FROM_UNIXTIME($tmp)";
OK, will do.
>      - /proc is only available on Linux, so the following will fail on
>        everything else. And there are really no reasons to use it,
>        there are many portable ways to get the cmd line from a Perl script
>
> +  $tmp = sprintf("cat /proc/%d/cmdline", $$);
> +  $tmp = qx($tmp);
> +  system("cp /proc/$$/cmdline /tmp");
> +  $tmp =~ s/\0/ /g; # transform embedded nulls between args to spaces
OK. I originally searched perldocs and didn't see anything useful but 
then, I completely missed a simple and obvious solution...capture them 
to a temp string before they get parsed/shifted.
>      - system("rm -f $file") is equivalent to unlink($file)
OK, perl noobness
>
>      - do you really want to scrub --encrypt and --encrypt-key-file?
Hmm, yeah well, I guess that is a bit overkill. Removing.
>
>      - 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.
>     - in case --incremental-history-name or --incremental-history-uuid is
>       used, it might be useful to have a diagnostic message about the LSN
>       value being used and why it is being used.
Good point, should print out the 'discovered' lsn that the incremental 
will be starting from.
>
>     - I guess built-in innobackupex help needs some updates (still
>       mentions --history-name, --no-history, etc.)
Ahh, yeah, missed those, fixing.
>     - in the test case: it is not necessary to include inc/common.sh
>       anymore. it is also not necessary to use “shift” if you don’t
>       access the parameters array later in a function
>
inc/common.sh removed.
re: shift, no idea why I they are in the first two fn's in the test, 
completely useless there.

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