Sven Sandberg wrote:
> Hi Mats,
>
> There is something strange with this patch - see comments below.
>
> Moreover, the new test case fails for me. See output after the patch.
>
>
> Mats Kindahl wrote:
> [...]
>> diff -Nrup a/sql/slave.cc b/sql/slave.cc
>> --- a/sql/slave.cc 2007-10-13 14:58:06 +02:00
>> +++ b/sql/slave.cc 2007-10-20 20:16:08 +02:00
> [...]
>> @@ -1905,7 +1915,8 @@ static int exec_relay_log_event(THD* thd
>> }
>> if (slave_trans_retries)
>> {
>> - if (exec_res && has_temporary_error(thd))
>> + int temp_err;
>> + if (exec_res && (temp_err= has_temporary_error(thd)))
>> {
>> const char *errmsg;
>> /*
>> @@ -1953,15 +1964,19 @@ static int exec_relay_log_event(THD* thd
>> "the slave_transaction_retries variable.",
>> slave_trans_retries);
>> }
>> - else if (!((thd->options & OPTION_BEGIN) &&
>> opt_using_transactions))
>> + else if (exec_res && !temp_err ||
>> + (opt_using_transactions &&
>> + rli->group_relay_log_pos == rli->event_relay_log_pos))
>
> This looks fishy: the condition of the `else if ()' does not depend on
> the truth value of `(opt_using_transactions &&
> rli->group_relay_log_pos == rli->event_relay_log_pos)'. In fact,
> `(opt_using_transactions ...)' will never be evaluated due to shortcut
> evaluation. Consider the following case analysis:
>
> 1. If exec_res==0 before the `if()', then the
> condition of the `else if()' is never true.
False. The evaluation of the else if() will be true if exec_res == 0,
opt_using_transactions != 0 , and rli->group_relay_log_pos ==
rli->event_relay_log_pos. Hence the else if() can be true even when
exec_res == 0.
> 2. If exec_res!=0 before the `if()', there are two cases:
> 2.1 if tmp_err!=0 before the `if()', then the condition of
> the if() is true, and hence the `else if()' condition
> is not evaluated.
False. First, the tmp_err is set as part of the evaluation of the if()
condition, so the statement is not sensible. Assuming that you mean that
has_temporary_error() evaluate to true (instead of "tmp_err is true
before the statement"), then if exec_res != 0 and has_temporary_error()
is false, then the else if() condition is evaluated.
> 2.2 if tmp_err==0 before the `if()', then the `else if()'
> condition holds always due to the !temp_err condition,
> and the (opt_using_transactions&&...) is not evaluated.
Since tmp_err cannot be set before the if(), this is not a sensible
statement. However, it is correct that the "(opt_using_transactions &&
..." is not evaluated when exec_res != 0 and tmp_err != 0, which is the
intention.
>
>
> ## BEGIN OUTPUT #################################################
> Logging: ./mtr rpl_temporary_errors
> MySQL Version 5.1.23
> Using binlog format 'mixed'
> Skipping ndbcluster, mysqld not compiled with ndbcluster
> Skipping SSL, mysqld not compiled with SSL
> Binaries are debug compiled
> Using MTR_BUILD_THREAD = 0
> Using MASTER_MYPORT = 9306
> Using MASTER_MYPORT1 = 9307
> Using SLAVE_MYPORT = 9308
> Using SLAVE_MYPORT1 = 9309
> Using SLAVE_MYPORT2 = 9310
> Using IM_PORT = 9312
> Using IM_MYSQLD1_PORT = 9313
> Using IM_MYSQLD2_PORT = 9314
> Killing Possible Leftover Processes
> Removing Stale Files
> Creating Directories
> Installing Master Database
> Installing Slave1 Database
> =======================================================
>
> TEST RESULT TIME (ms)
> -------------------------------------------------------
>
> rpl.rpl_temporary_errors [ fail ]
>
> mysqltest: In included file
> "./include/wait_for_slave_sql_to_stop.inc": At line 19: Error running
> query ' SHOW SLAVE STATUS': 2013 Lost connection to MySQL server
> during query
>
> The result from queries just before the failure was:
> < snip >
> **** On Slave ****
> SHOW STATUS LIKE 'Slave_retried_transactions';
> Variable_name Value
> Slave_retried_transactions 0
> UPDATE t1 SET a = 5, b = 47 WHERE a = 1;
> SELECT * FROM t1;
> a b
> 5 47
> 2 2
> 3 3
> 4 4
> **** On Master ****
> UPDATE t1 SET a = 5, b = 5 WHERE a = 1;
> SELECT * FROM t1;
> a b
> 5 5
> 2 2
> 3 3
> 4 4
> **** On Slave ****
>
> More results from queries before failure can be found in
>
> /home/sven/bk/b24860-5.1-new-rpl-trans_error_infinite_loop/mysql-test/var/log/rpl_temporary_errors.log
>
>
>
> Aborting: rpl.rpl_temporary_errors failed in default mode.
> To continue, re-run with '--force'.
> Stopping All Servers
> ## END OUTPUT ###################################################
>
>
Yes, this is because the test case triggered BUG#31702, which I have
fixed and got a review of. You need to apply the patch for this before
you can execute the test case for this bug.
Just my few cents,
Mats Kindahl
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
