Re: bzr commit into mysql-5.1 branch (sven:2618) Bug#37373

[Mats Kindahl]

Hi Sven!

I have some comments on the patch inline below.

Just my few cents,
Mats Kindahl

Sven Sandberg wrote:
> #At file:///home/sven/bzr/b37373-rpl_ndb_transaction/5.1-rpl/
> 
>  2618 Sven Sandberg	2008-06-28
>       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
>       binlog updates in ndb system tables at a nondeterministic
>       time point after an ndb table update has been committed.
>       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.
> added:
>   mysql-test/include/save_master_pos.inc
>   mysql-test/include/wait_for_slave_io_to_sync.inc
>   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
> modified:
>   mysql-test/suite/rpl_ndb/r/rpl_ndb_transaction.result
>   mysql-test/suite/rpl_ndb/t/rpl_ndb_transaction-master.opt
>   mysql-test/suite/rpl_ndb/t/rpl_ndb_transaction.test
> 
> per-file messages:
>   mysql-test/include/save_master_pos.inc
>     Like the test language's built-in save_master_pos, but saves it in a test
>     language variable so it's accessible to tests. This was needed to
>     implement wait_for_slave_io_to_sync.inc.
>   mysql-test/include/wait_for_slave_io_to_sync.inc
>     Like sync_slave_with_master, but only waits until the IO thread has
>     synced; the SQL thread may still be behind.
>   mysql-test/suite/rpl/r/rpl_bug26395.result
>     New result file.
>   mysql-test/suite/rpl/t/rpl_bug26395-master.opt
>     New opt file.
>   mysql-test/suite/rpl/t/rpl_bug26395-slave.opt
>     New opt file.
>   mysql-test/suite/rpl/t/rpl_bug26395.test
>     - Moved second half of rpl_ndb_transaction here.
>     - Improved the test case: instead of using a sleep,
>       it waits for the slave's io thread to sync up to a proper
>       position, and for the slave's sql thread to sync up to
>       another position.
>     - Added a warning that no more tests should be added at the
>       end of the file.
>   mysql-test/suite/rpl_ndb/r/rpl_ndb_transaction.result
>     Updated result file.
>   mysql-test/suite/rpl_ndb/t/rpl_ndb_transaction-master.opt
>     No need for the special debug flag any more, it was used by
>     the second part of the test which is now in rpl_bug26395.
>   mysql-test/suite/rpl_ndb/t/rpl_ndb_transaction.test
>     - Moved second half of the test to another test (rpl_bug26395)
>     - Improved comments.
>     - Extended the mixed transaction test to include also innodb.
>     - Used 'source include/diff_tables.inc' instead of listing
>       several identical tables in the result file.
> === 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-06-28 14:54:47 +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);

Seems a little excessive to create a dedicated include file for two
lines: it only saves a single line in the test case, it still requires
anybody interested in using it to open it and look in it to figure out
what the variable names are, and it introduces two "magic" variables in
the test that uses it.

[Update: I see that you use it to sync the I/O thread below, so that is
a different use case, so I accept the introduction of a new file for it.]

> === 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-06-28 14:54:47 +0000
> @@ -0,0 +1,42 @@
> +# ==== 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.

Ah, OK. Different scenario. I thought you were going to use it for
something else.

> +
> +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;
> +}
> +
> +let $_counter= 300;
> +
> +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'`;

Why not set $_not_done to 1 and force it to do the first loop? That will
allow you to not have to duplicate the code both inside and outside the
loop.

It does mean that it does a 100ms wait before actually continuing, but I
view that as a minor issue.

[snip]

> +connection slave;
> +DROP TABLE tinnodb;
> +
> +set @@debug= @old_debug;
> +
> +# Warning: do not add more tests here. The binlog is in a bad state.
> +

In that case, should you not reset the master (and the slave) to ensure
that the bad state is thrown away?

[snip]

> +==== Verify the result ====
> +SELECT * FROM tmyisam ORDER BY a;
>  a

[snip]

> +142
> +146
> +SELECT * FROM tinnodb ORDER BY a;
>  a

[snip]

> +125
> +127
>  SELECT * FROM tndb ORDER BY a;

[snip]

>  7

[snip]

> +8
> +19
> +20
> +44

[snip]

> +121
> +123
> +126
> +[on slave]
> +Comparing tables master:test.tmyisam and master:test.tmyisam
> +Comparing tables master:test.tinnodb and master:test.tinnodb
> +Comparing tables master:test.tndb and master:test.tndb

Is there any particular reason you expect test.tmyisam, test.tinnodb,
and test.tndb to be different from itself?

Assuming that it is a typo (and you want to compare the master and slave
tables), do you really need the selects on the same tables above?

> +==== Clean up ====
> +[on master]
>  DROP TABLE tmyisam, tinnodb, tndb;
> +[on slave]

Comment seems redundant.

> 
> === modified file 'mysql-test/suite/rpl_ndb/t/rpl_ndb_transaction-master.opt'
> --- a/mysql-test/suite/rpl_ndb/t/rpl_ndb_transaction-master.opt	2007-12-14 13:40:45
> +0000
> +++ b/mysql-test/suite/rpl_ndb/t/rpl_ndb_transaction-master.opt	2008-06-28 14:54:47
> +0000
> @@ -1 +1 @@

[snip]

> +# ==== Related bugs ====
> +#
> +# BUG#26395: if crash during autocommit update to transactional table on master,
> slave fails
> +
>  
>  source include/ndb_master-slave.inc;
> -source include/have_ndb.inc;
>  source include/have_innodb.inc;
> -source include/have_debug.inc;
> +source include/have_ndb.inc;

Any particular reason to move the "source include/have_ndb.inc" line?
You are just placing yourself for a difficult merge in case somebody
switches order on two other tables (you might actually trigger the
Bazaar bug that you reported for --weave).

[snip]

> +--echo ---- autocommitted ----
>  
>  SET AUTOCOMMIT = 1;
>  
> -INSERT INTO tndb VALUES (1);
> -INSERT INTO tmyisam VALUES (1);
> +INSERT INTO tmyisam VALUES (0);
> +INSERT INTO tinnodb VALUES (1);
> +INSERT INTO tndb VALUES (2);

Any particular reason to change the values inserted into tndb and
tmyisam? I think that you are again placing yourself for a difficult
merge, or causing a buggy merge.

[snip]

> +--echo ==== Verify the result ====
> +
> +SELECT * FROM tmyisam ORDER BY a;
>  SELECT * FROM tinnodb ORDER BY a;
> +SELECT * FROM tndb ORDER BY a;
> +
> +--echo [on slave]
> +--sync_slave_with_master
> +
> +let $diff_table_1=master:test.tmyisam;
> +let $diff_table_2=master:test.tmyisam;
> +source include/diff_tables.inc;

See my comment about about comparing tables with themselves.

>  
> -# Clean up. We cannot do it on master and replicate over, because
> -# master binlog is in a bad state after last test. So we do it both on
> -# master and on slave.
> ---echo --- on master ---
> +let $diff_table_1=master:test.tinnodb;
> +let $diff_table_2=master:test.tinnodb;
> +source include/diff_tables.inc;

See my comment about about comparing tables with themselves.

> +
> +let $diff_table_1=master:test.tndb;
> +let $diff_table_2=master:test.tndb;
> +source include/diff_tables.inc;

See my comment about about comparing tables with themselves.

-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com