Merge lp://qastaging/~sergei.glushchenko/percona-xtrabackup/bug711166_part-1.6 into lp://qastaging/percona-xtrabackup/1.6

Jenkins build:
http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-1.6-param/107/

Sergei,

Partial backups can also be created with the --include option of innobackupex (or its xtrabackup equivalent, --tables). That does not seem to be covered by the fix.

I've amended the bug report so we take that into account in the fix.

Some minor comments:

- innobackupex should be used in tests rather than IB_BIN directly

- please use --copy-back to restored when innobackupex is used to create a backup, so we also test that functionality

- you can use the checksum_table() function from common.sh

- xtrabackup.c (mostly) follows the InnoDB coding style. so the closin */ in comments should be on the last comment line, rather than on a separate one.

- typo ("Cheking")

- typo ("parttition")

Alexei,

> Sergei,
>
> Partial backups can also be created with the --include option of innobackupex
> (or its xtrabackup equivalent, --tables). That does not seem to be covered by
> the fix.
>

--include option passed directly to --tables option of xtrabackup.
Each database.table value passed to --tables option considered as regular
expression. So if we pass --tables=test.test, all partitions of database test
would be backed up because of test.test matches all files test/test#P#*
I think it's a good behavior.

Thanks for your comments! I'll do my best to fix tescases.

Sergei,

On 01.02.12 21:20, Sergei Glushchenko wrote:
> Alexei,
>
>> Sergei,
>>
>> Partial backups can also be created with the --include option of innobackupex
>> (or its xtrabackup equivalent, --tables). That does not seem to be covered by
>> the fix.
>>
>
> --include option passed directly to --tables option of xtrabackup.

Not always. It is handled in innobackupex for streaming backups.

> Each database.table value passed to --tables option considered as regular
> expression. So if we pass --tables=test.test, all partitions of database test
> would be backed up because of test.test matches all files test/test#P#*
> I think it's a good behavior.
>

Consider a case when I have tables 'test' and 'test_table'. So to be
able to backup only 'test', but not 'test_table', I have to use
--tables='test$'. But that won't work if 'test' is partitioned, right?

Bug 711166
Partitioned tables are not correctly handled by the --databases, --include,
--tables-file options of innobackupex, and by the --tables and
--tables-file options of xtrabackup.
Solution is to remove partition suffix (#P#...) before doing filtering.
Testcases cover variants of using filtering options with MyISAM and
InnoDB tables (to test both innobackupex and xtrabackup) with either stream
mode turned on and turned off.

Since last MP request:
 added --include option handling
 added more testcases

Jenkins build:
lp:~sergei.glushchenko/percona-xtrabackup/bug711166_part-1.6

Jenkins build:
http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-1.6-param/110/

Sergei,

   - I think you can avoid some code duplication in test cases by
     putting the table definition to a common file in the inc/ directory
     and using the default_storage_engine server variable.
   - you can also reduce the test sizes considerably by not messing with
     information_schema and performance_schema. I_S is actually not a
     storage engine, so it doesn't have any files in the data dir. And
     you could just drop the test database before shutting down the
     server and restoring, rather than just remove the entire data dir,
     so you don't have to care about performance_schema and mysql DBs. I
     think that would make test cases much smaller and easier to read,
     but without any impact on test coverage.
   - you can also make tests faster by not inserting 500 rows into the
     table. Inserting that many rows one-by-one by invoking the client
     each time actually take significant time. I think the following
     would be sufficient for your purposes:
       INSERT INTO test VALUES (1), (101), (201), (301), (401);
   - Unlike the core server, InnoDB code encloses "if" blocks in braces
     even if the block consists of only 1 statement.
   - I think changes in xtrabackup.c have to much duplication and
     unnecessary work. The whole point of those transformations is to
     check whether a specific table should be skipped. Please take a
     look at my changes to that code here
     http://bazaar.launchpad.net/~akopytov/percona-xtrabackup/compact-backups/view/head:/src/xtrabackup.c#L1550
     Perhaps it even makes sense to backport check_if_skip_table() from
     compact backups branch to 1.6/trunk and base your fix on that
     function?

Alexey,

> - I think you can avoid some code duplication in test cases by
> putting the table definition to a common file in the inc/ directory
> and using the default_storage_engine server variable.

yes. You're right about that.

> - you can also reduce the test sizes considerably by not messing with
> information_schema and performance_schema. I_S is actually not a
> storage engine, so it doesn't have any files in the data dir. And
> you could just drop the test database before shutting down the
> server and restoring, rather than just remove the entire data dir,
> so you don't have to care about performance_schema and mysql DBs. I
> think that would make test cases much smaller and easier to read,
> but without any impact on test coverage.

We can't just drop database `test` and keep other databases in their places.
copy-back function expects that data directory is empty when we are
restoring backup.

> - you can also make tests faster by not inserting 500 rows into the
> table. Inserting that many rows one-by-one by invoking the client
> each time actually take significant time. I think the following
> would be sufficient for your purposes:
> INSERT INTO test VALUES (1), (101), (201), (301), (401);

OK. I'll follow your advice.

> - Unlike the core server, InnoDB code encloses "if" blocks in braces
> even if the block consists of only 1 statement.

Thanks. I should print InnoDB code guidelines somewhere and put it somewhere over my eyes.

> - I think changes in xtrabackup.c have to much duplication and
> unnecessary work. The whole point of those transformations is to
> check whether a specific table should be skipped. Please take a
> look at my changes to that code here
> http://bazaar.launchpad.net/~akopytov/percona-xtrabackup/compact-
> backups/view/head:/src/xtrabackup.c#L1550
> Perhaps it even makes sense to backport check_if_skip_table() from
> compact backups branch to 1.6/trunk and base your fix on that
> function?

As I can see check_if_skip_table does pretty much the same thing, but does
in-place patching instead of copying. I tried to avoid in-place patching,
because of it is not good from my point of view to modify data which is
not belongs to us. You should look at xtrabackup_stats_func in your
branch. It contains pretty much the same code as check_if_skip_table.

I'm surely can write something similar to check_if_skip_table, but avoiding
in-place patching. What do you think about that?

Sergei,

On 17.02.12 14:05, Sergei Glushchenko wrote:
>> - you can also reduce the test sizes considerably by not messing with
>> information_schema and performance_schema. I_S is actually not a
>> storage engine, so it doesn't have any files in the data dir. And
>> you could just drop the test database before shutting down the
>> server and restoring, rather than just remove the entire data dir,
>> so you don't have to care about performance_schema and mysql DBs. I
>> think that would make test cases much smaller and easier to read,
>> but without any impact on test coverage.
>
> We can't just drop database `test` and keep other databases in their places.
> copy-back function expects that data directory is empty when we are
> restoring backup.
>

Right, which means --copy-back is useless for partial backups. It either
creates an unusable datadir, if it was empty originally, or just fails,
if it wasn't empty.

Which in turn means users are expected to restore manually from a
partial backups. And so we should do the same in the test suite?

>
>> - Unlike the core server, InnoDB code encloses "if" blocks in braces
>> even if the block consists of only 1 statement.
>
> Thanks. I should print InnoDB code guidelines somewhere and put it somewhere over my eyes.
>

The problem is that there's no such thing as InnoDB code guidelines. If
you find them, let me know! :)

>> - I think changes in xtrabackup.c have to much duplication and
>> unnecessary work. The whole point of those transformations is to
>> check whether a specific table should be skipped. Please take a
>> look at my changes to that code here
>> http://bazaar.launchpad.net/~akopytov/percona-xtrabackup/compact-
>> backups/view/head:/src/xtrabackup.c#L1550
>> Perhaps it even makes sense to backport check_if_skip_table() from
>> compact backups branch to 1.6/trunk and base your fix on that
>> function?
>
> As I can see check_if_skip_table does pretty much the same thing, but does
> in-place patching instead of copying. I tried to avoid in-place patching,
> because of it is not good from my point of view to modify data which is
> not belongs to us. You should look at xtrabackup_stats_func in your
> branch. It contains pretty much the same code as check_if_skip_table.
>
> I'm surely can write something similar to check_if_skip_table, but avoiding
> in-place patching. What do you think about that?
>

Sure, my point was that encapsulating the entire logic to decide whether
a table should be skipped or not is better than implementing some
auxiliary functions and passing the buffers around.

In-place modifications are certainly not nice, I just didn't have time
to fix that too, as I was after something completely different, so I
left that work for better times. Such as now :)

But thanks for pointing out the same code is in xtrabackup_stats_func().
I didn't notice it, because the main object of refactoring was
xtrabackup_copy_datafile().

OTOH I think malloc()s can be avoided and replaced with a
stack-allocated bufer.

Tescases were reduced and improved a little. It allowed to find mistake in testcase and implementation. (tar command in stream mode didn't relsolve symbolic likns). check_if_skip_table was implemented instead of auxiliary functions.

Jenkins:
http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-1.6-param/119/

Updated with current 1.6 trunk.
lp:~sergei.glushchenko/percona-xtrabackup/bug711166_part-1.6

http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-1.6-param/147/

Sergei,

As discussed previously, we will not be fixing this bug in 1.6. Please create a 2.0 MP.