Code review comment for lp:~sergei.glushchenko/percona-xtrabackup/BT-26901-2.0

Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

> Sergei,
>
> A few of comments:
>
> - what if innodb_doublewrite_file is not specified in my.cnf? As I understand,
> --print-param will either crash or print "innodb_doublewrite_file = (null)"
> (on Linux), and then innobackupex will assume we have a doublewrite full with
> "(null)" as its name.
>

Yes, i fixed it.

> - why don't we write stripped innodb_doublewrite_file (i.e. with path removed)
> to backup-my.cnf? Less code in innobackupex to strip it on --copy-back
>

The reason was to avoid hacking in testcase. Also we modified some values from my.cnf when put them to backup-my.cnf before. Currently we don't. But I agree that full original path is not needed in backup-my.cnf for xtrabackup to operate. So I rewrite to do path strip before writing to backup-my.cnf.

> - why don't we make 2.0 and 2.1 consistent and just modify
> trx_sys_sys_space(id) to include the #ifdef and avoid having a separate macro?
> Less code changes in 2.0, less conflicts when merging 2.0->2.1 in the future.
>

Yep. The reason was I did not see it before. Then see in 2.1 and fix and not yet synced 2.0 with it.

> - please use one of MYSQLD_VARDIR/MYSQLD_DATADIR/MYSQLD_TMPDIR instead of
> hardcoded paths as in "${TEST_BASEDIR}/var1..."
>

It isn't possible due to unavailability of MYSQLD_* before server startup.

> - s/.idb/.ibd/

OK.

« Back to merge proposal