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

George,

   - I propose we rename ‘mysql_version’ to ‘server_version’, since we
     support not only MySQL.

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

   - the table does not define the character set as we discussed
     previously. This means the character set will depend on
     server/connection settings.

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

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

   - 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)";

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

    - system("rm -f $file") is equivalent to unlink($file)

    - do you really want to scrub --encrypt and --encrypt-key-file?

    - 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

}

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

   - I guess built-in innobackupex help needs some updates (still
     mentions --history-name, --no-history, etc.)

   - 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