Code review comment for lp:~akopytov/percona-xtrabackup/bug1079700-2.0

Hi Laurynas,

On Wed, 08 May 2013 14:48:26 -0000, Laurynas Biveinis wrote:
> Review: Needs Information
>
> - The MLOG_FILE_DELETE bits need reconcilation and/or explanation
> on why the already existing hashed log rec handling in the
> patches for buf_page_read_low() is not enough. Maybe it's
> possible to tweak buf_page_read_low() patch instead of adding
> more patches to log0recv.c? Please see
> http://bugs.mysql.com/bug.php?id=43948 and
> https://bugs.launchpad.net/percona-xtrabackup/+bug/1121072
> which I have closed as Invalid due to this MP.
>

The explanation is that you did not port the patch for bug #43948 to
innodb56.patch. Now that I have patched recv_apply_hashed_log_recs(),
the fix for bug 43948 in PS and innodb51.patch/innodb55.patch is redundant.

But since the goal is to get rid of all those patches, they will be
"reconciled" naturally.

> - Will the following work:
> - XtraBackup creates a list of tablespaces;
> - CREATE TABLE foo is issued (optionally maybe even CREATE
> DATABASE first);
> - XtraBackup proceeds with backup;
> - Incremental backup is taken. Will it contain foo? It is
> possible that some of the foo writes will be captured in the
> full backup redo log backup.
> I guess this question boils down to "is fil_system snapshot
> taking atomic with the rest of backup state." CREATE TABLE
> specifically because other scenarios seem to be covered by this
> fix. I am not implying that I suspect it will or will not, I
> don't know and am curious.
>

fil_op_log_parse_or_replay() does nothing if it has to replay
MLOG_FILE_CREATE and the tablespace already exists. So if the tablespace
makes it into fil_system at the backup stage, XtraBackup will ignore
MLOG_FILE_CREATE. Otherwise it will replay it. Not sure what difference
a subsequent incremental backup would make.

> - This might be outside the scope of the current fix, but since
> you are backporting the mysqldump backup verifier, I am
> mentioning it here. If there's a bug/blueprint open for this,
> please point me there: mysqldump verification probably would miss
> secondary index tree corruptions, wouldn't it?
>

Both mysqldump and CHECKSUM TABLE verification are identical for InnoDB.
CHECKSUM TABLE basically does "SELECT * FROM table" internally and
calculates checksums for all values in all rows. So corruption in
secondary index pages will also go unnoticed.

> - What are the s/AM_CONFIG_HEADER/AC_CONFIG_HEADERS bits for?
> Some bug fixed? If yes, is there a # ref?
>

It is a backport of the fix for bug #1164958 to older PS/MySQL 5.1 versions.

> - Likewise for configure.cmake patch bits?
>

Likewise, backports of the fix for
http://bugs.mysql.com/bug.php?id=65050 to older PS/MySQL 5.5 versions.

> - I recall seeing utils/build.sh patch elsewhere, yet there is
> only one unmerged revision in this MP. Is this intended?
>

That was for 2.1.