Code review comment for lp:~sergei.glushchenko/percona-xtrabackup/bug711166_part-1.6

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.