Code review comment for lp:~laurynas-biveinis/percona-xtrabackup/bug1098498-1132763-2.0

Hi Laurynas,

On Wed, 06 Mar 2013 12:45:33 -0000, Laurynas Biveinis wrote:
>> - I don't understand the reason to call free_reserved_port and
>> reset_server_variables only if the PID file exists in stop_server /
>> shutdown_server. Don't we still want to cleanup in case the PID file
>> disappeared for whatever reasons?
>
> I'll revert it as it would make for a smaller diff. But I am not sure what would be the whatever reasons for PID to disappear. If this is a "can't happen" situation, then it's probably the case that the testsuite run wouldn't be able to recover to continue running for trusted results anyway?
>

I don't know. A test case removing the PID file or its directory for
some evil server/backup torturing? Or shutting down the server on its
own with mysqladmin shutdown for the same reason?

Just don't see any sense in avoiding those calls in case of a missing
PID file. It looks less reliable this way.

>> - Was the following change really necessary?
>>
>> 128 -dd if=/dev/zero of=$mysql_datadir/sakila/rental.ibd seek=1000 count=1
>> 129 +printf '\xAA\xAA\xAA\xAA' | dd of=$mysql_datadir/sakila/rental.ibd
>> seek=16384 count=4
>
> I described it in the commit message:
>
> "2) bug766033: replace the database corruption of poking one zero byte
> to the first page with overwriting the start of the second page with
> 0xAAAAAAAA to reduce the risk of the previous corruption attempt not
> corrupting anything due to the zero byte already existing at the
> location."
>
> Replacing the shutdown with the crash and using the original corruption attempt makes that testcase pass reliably. Poking a single zero byte to a first tablespace page doesn't look like a very reliable corruption attempt to me due to the chances that that byte will be zero already, which is presumably what happens with the crashed server.
>

OK. The reason I asked was that the code is not portable (dd semantics
is different on OSX/BSD). So the test case was corrupting another page
and worked mostly by accident on anything but Linux.

But I have checked it on my laptop, and the new code still works by
accident on OSX/BSD.

>> - Was the following change really necessary?
>>
>> 215 # also test xtrabackup --stats work with --tables-file
>> 216 -COUNT=`xtrabackup --stats --tables='test.test$'
>> --datadir=$topdir/backup \
>> 217 +COUNT=`xtrabackup --stats
>> --tables='^(mysql.*|performance_schema.*|test.test)$' --datadir=$topdir/backup
>> \
>
> No, thank you.
>