Re: bzr commit into mysql-6.0 branch (sven:2662) Bug#37373

[Andrei Elkin]

Sven, hello.

I am answering your last responce on my review and commenting on your
latest patch.

> Hi Andrei,
>
> Thanks for a thorough review! I agree to most of your suggestions,
> except I think the test has to be split into two files, and they
> should be in the rpl_ndb suite and the rpl suite, respectively. Also,
> we can't assume that ndb modifies its system tables at any particular
> point - 
> that happens nondeterministically. See details below.
>
> Andrei Elkin wrote:
>> Sven, hello.
>>
>> Here are some comments before I have read the newest patch that I
>> could not find.
>> We can improve the test in some ways, I think, but perhaps a bit
>> differently than your patch offers.
>>
>>> #At file:///home/sven/bzr/b37373-rpl_ndb_transaction/6.0-rpl/
>>>
>>>  2662 Sven Sandberg	2008-06-17
>>>       BUG#37373: rpl_ndb_transaction fails sporadically in pb on
> sol10-amd64-a and sol10-sparc-a
>>>       Problem: rpl_ndb_transaction fails because it assumes nothing
>>>       is written to the binlog at a certain point. However, ndb may
>>>       write updates to ndb system tables to the binlog at a
>>
>> did not you mean
>> write updates *from* ndb system tables to the binlog ...?
>>
>>>       nondeterministic time point after an ndb table update has
>>>       been committed.
>>
>> A straightforward fix, as it stands to me, could be to wait on master
>> till the event on ndb table will show up in the binlog.
>
> The problem is that the event shows up at a nondeterministic point in
> the binlog. Sometimes there is an event, sometimes not. 

I could agree that an event can emerge from ndb somehow unpredictably,
before or after events from other engines.
It should not just get lost, correct?

But, indeed, it's not clear how to find out whether the event has come to
binlog or not in such a case...
the size of the master binlog might not be guaranteed to be a
constant, and what else to use i personally don't see.

so i am okay with it.

> So it is
> difficult to come up with a condition to wait for. I also think it
> makes sense to split the test into two parts, since they are quite
> different.
>
>>>       Fix: break the test into two. rpl_ndb_transaction still does
>>>       the ndb updates needed by the first half of the test. The new
>>>       test case rpl_bug26395 includes the part that assumes nothing
>>>       more will be written to the binlog.
>>>       Also improved test cases a bit.
>>> added:
>>>   mysql-test/suite/rpl/r/rpl_bug26395.result
>>>   mysql-test/suite/rpl/t/rpl_bug26395-master.opt
>>>   mysql-test/suite/rpl/t/rpl_bug26395-slave.opt
>>>   mysql-test/suite/rpl/t/rpl_bug26395.test
>>
>> I'd rather to agree with Mats who thinks we are better to collect
>> bugXYZ tests in the bugs suite.
>
> I have discussed this with Magnus and subsequently with Mats. The
> problem is that different suites have different server options. E.g.,
> tests in the rpl suite start up a slave server, whereas tests in
> binlog do not, but they run with --have-binlog, etc. Therefore, it
> does not make sense to have a specific bugs suite, at least with the
> way it works now. Mats agreed on IRC to let the test stay in rpl. If
> we later change so that the tests don't rely on per-suite server
> options, we can move the test to the bug suite at that time.

ok

>
>> However, in the case of the 2nd part of the original
>> rpl_ndb_transaction.test a basic property is verified.
>> That makes me think the 2nd part of the test is better to remain in a
>> basic suite test.
>> I guess you will second that the name rpl_ndb_transaction.test
>> neither the suite rpl_ndb does not fit to problems of the test.
>> It is supposed to verify XA-aware and XA-ignorant egnines replication,
>> ndb is just one of them.
>>
>> So I'd rather to rename it like
>>
>>     rpl_xa_various_engine.test
>
> mtr only sets up ndb and replication for tests in the rpl_ndb
> suite. So the test has to stay in rpl_ndb.

Could it be renamed to reflect its purpose anyway? (only if you share
this concern).

======================================================================
Now, the newest patch:
======================================================================
...
> === added file 'mysql-test/include/save_master_pos.inc'
> --- a/mysql-test/include/save_master_pos.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/save_master_pos.inc	2008-07-03 09:28:39 +0000
> @@ -0,0 +1,16 @@
> +# ==== Purpose ====
> +#
> +# Saves the master position in the variables
> +# $saved_master_file and $saved_master_position.
> +#
> +# ==== Usage ====
> +#
> +# source include/save_master_pos.inc;
> +#
> +# Must be executed on a master.
> +#
> +# Usually followed by source include/wait_for_slave_io_to_sync.inc; or
> +# some other script that uses the saved position.
> +
> +let $saved_master_file= query_get_value("SHOW MASTER STATUS", File, 1);
> +let $saved_master_pos= query_get_value("SHOW MASTER STATUS", Position, 1);
>

it's not a bad idea. 
However, please notice that wait_for_... macros
come with one or a pair of conditions like

#    let $slave_param= Slave_SQL_Running;
#    let $slave_param_value= No;

wait_for_slave_io_to_sync.inc identifies it as member of the wait_for
by name, but it does not use the set's convention.

A way to exploit your idea, as stands to me, could be

to rename 

wait_for_slave_io_to_sync.inc -> sync_slave_io_with_master

and extend it with making 

source include/save_master_pos in its defintion.

I don't insist you have to do this though.

But there is important details.

> === added file 'mysql-test/include/wait_for_slave_io_to_sync.inc'
> --- a/mysql-test/include/wait_for_slave_io_to_sync.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/wait_for_slave_io_to_sync.inc	2008-07-03 09:28:39 +0000
> @@ -0,0 +1,40 @@
> +# ==== Purpose ====
> +#
> +# Waits until the slave IO thread has been synced, i.e., all events
> +# have been copied over to slave.  Does not care if the SQL thread is
> +# in sync.
> +#
> +#
> +# ==== Usage ====
> +#
> +# Syncs the slave to the position saved in the last call to
> +# include/save_master_pos.inc.
> +#
> +# Must be called on the slave.
> +
> +let $_error= `SELECT '$saved_master_pos' = '' OR '$saved_master_file' = ''`;
> +if ($_error)
> +{
> +  --echo ERROR IN TEST SCRIPT: you must source include/save_master_pos.inc before
> include/wait_for_slave_io_to_sync.inc
> +  exit;
> +}

sync_slave_io_with_master whould not need that.

> +
> +let $_counter= 300;
> +let $_not_done= 0;
> +
> +while ($_not_done)
> +{
> +  dec $_counter;
> +  if (!$_counter)
> +  {
> +    --echo ERROR: failed while waiting for slave io to sync. _file=$_file _pos=$_pos
> saved_master_file=$saved_master_file saved_master_pos=$saved_master_pos
> +    query_vertical show slave status;
> +    exit;
> +  }
> +
> +  sleep 0.1;
> +
> +  let $_file= query_get_value("SHOW SLAVE STATUS", Master_Log_File, 1);
> +  let $_pos= query_get_value("SHOW SLAVE STATUS", Read_Master_Log_Pos, 1);
> +  let $_not_done= `SELECT '$_file' != '$saved_master_file' OR '$_pos' !=
> '$saved_master_pos'`;
> +}

A way to define the new macro that looks to be the right one, is to
reuse wait_for_slave_param.inc

connection master;
source include/save_master_pos.inc;

connection slave;

let $slave_param= Master_Log_File;
let $slave_param_value= $saved_master_file;
include/wait_for_slave_param.inc
let $slave_param= Read_Master_Log_Pos;
let $slave_param_value= $saved_master_pos;
include/wait_for_slave_param.inc;

EOF


I am okay with the rest and am grateful that you accounted some of my
former comments.

cheers,

Andrei