Code review comment for lp:~akopytov/percona-xtrabackup/read-server-options-with-show-variables

Hi Sergei,

On Tue, Oct 07 2014 13:05:13 +0400, Sergei Glushchenko wrote:

> Hi Alexey,
>
> The patch itself looks good, but
>
> I am concerned of "Bug #1343722: Too easy to backup wrong datadir with
> multiple instances". Not only datadir but every other important option
> can be different if we read options from my.cnf which doesn't belong
> to MySQL instance.
>
> This patch makes following change:
>
> innobackupex reads options from running MySQL.
> xtrabackup reads options from my.cnf.
>
> Now instead of single source we have two sources which can be out of
> sync. We making sure that xtrabackup and innobackupex agree about
> "datadir" but we don't take proper care of other options. Wouldn't it
> be better to make both innobackupex and xtrabackup to use runtime
> options (innobackupex could simply pass all sensitive options to
> xtrabackup or even dump them into tmp-my.cnf).

Well, the patch went through multiple iterations. In one of them, it did
exactly that: it verified all options rather than just datadir. A few
problems had been revealed in testing:

- boolean values like innodb_fast_checksum can only be to 0/1 (or not
  have a value) in the defaults file. That’s how ‘xtrabackup
  --print-param’ prints them. But their values in SHOW VARIABLES are
  ON/OFF. So this must be taken into account when validating all
  options.

- path options like innodb_data_home_dir, innodb_log_group_home_dir,
  etc. They can be specified in an unnormalized way (with multiple
  slashes, “.”, “..”, etc.) in configuration file, but for some of them
  the server normalizes the paths as shown in SHOW VARIABLES, and for
  some of them no normalization is done. This could be solved with
  realpath() as we currently do with datadir. But on top of that, those
  paths can be relative to datadir, and that would require even more
  complex validation logic than just using realpath()

- sometimes we deliberately want to override some options for xtrabackup
  in the [xtrabackup] section of my.cnf, so that the values would be
  deliberately different between SHOW VARIABLES and the ones printed by
  ‘xtrabackup --print-param’.

After some considerations, I decided to only validate datadir, as
everything else would be a bit too risky for a point release. The patch
is rather invasive for a point release even without those changes.

As to passing all sensitive options from innobackupex to xtrabackup,
that’s what we do now for some of them (namely, datadir,
innodb_log_file_size and innodb_data_file_path), because those variables
have different defaults across major server versions, so using runtime
values is crucial.

Passing other value would work, and would eliminate possibility (which
is rather low in practice) of cases when ‘datadir’ is the same in SHOW
VARIABLES and my.cnf, but other (sensitive for XtraBackup) options are
different. But it comes with its own bunch of drawbacks and further
complications and corner cases in the code.

I decided to keep the patch minimal to solve current issues
for a point release, and scheduled
https://blueprints.launchpad.net/percona-xtrabackup/+spec/rewrite-innobackupex-in-c
for the next major release. Seriously, we should do it ASAP, but
obviously, not in a point release.