Hi Mats,
Mats Kindahl wrote:
>> +
>> +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.
Sure, can do that.
>> +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?
Hmmm, I don't think so. That would remove the binlogs. If the test
fails, it will be difficult analyze without the binlogs. Any test case
executing next and using replication is supposed to source
include/master-slave.inc anyway, which will reset master and slave.
>> +[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?
Right, well spotted.
>> 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).
OK
> [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?
Yes, I maintained the property that the N'th insert in the file inserts
the value N. This makes it easier to see where the test fails, if it
fails. It also makes it easier to see that the result file is as expected.
> I think that you are again placing yourself for a difficult
> merge, or causing a buggy merge.
Since the test is essentially rewritten, I think it is *good* if we get
a conflict in the (unlikely) case this file is concurrently edited by
someone else. An auto-merge would be dangerous.
--
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com
