Mats, good day!
The patch you were looking needed merging with the current 5.0 code
base which includes bug#27417 refactoring.
That was marked with lines like
+ // TODO: merge with bug#27417,23333
That's why there are some extra questions I am answering anyway along
the lines of this old patch.
A new commit has been done.
I spoke to Joro about adding a two-liner fix for
Bug #30763 Multi-table UPDATE with transaction + non-transactional table assertion failure
which went in the way of my testing.
I hope you can review those fixes within the common changeset. A test
for bug#29136 covers this new bug#30763 regression.
cheers,
Andrei
>
> Andrei Elkin wrote:
>> Below is the list of changes that have just been committed into a local
>> 5.0 repository of elkin. When elkin does a push these changes will
>> be propagated to the main repository and, within 24 hours after the
>> push, to the public repository.
>> For information on how to access the public repository
>> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>>
>> ChangeSet@stripped, 2007-06-23 16:33:39+03:00,
> aelkin@stripped +7 -0
>> Bug #29136 erred multi-delete on trans table does not rollback the statement
>> similar to bug_27716, but it was stressed on in the synopsis on
>> that there is another
>> side of the artifact affecting behaviour in transaction.
>> Fixed with deploying multi_delete::send_error() - otherwise
>> never called - and refining its logic
>> to perform binlogging job if needed.
>> A small refinement for the related bug's fix
>>
>> mysql-test/r/innodb.result@stripped, 2007-06-23 16:33:33+03:00,
> aelkin@stripped +18 -5
>> results changed
>>
>> mysql-test/r/multi_update.result@stripped, 2007-06-23 16:33:33+03:00,
> aelkin@stripped +15 -0
>> results changed
>>
>> mysql-test/t/innodb.test@stripped, 2007-06-23 16:33:33+03:00,
> aelkin@stripped +32 -0
>> trans engine specific test added
>>
>> mysql-test/t/multi_update.test@stripped, 2007-06-23 16:33:33+03:00,
> aelkin@stripped +32 -0
>> the schedule: send_eof() (binloggin there), send_error (early return)
>>
>> sql/sql_delete.cc@stripped, 2007-06-23 16:33:33+03:00,
> aelkin@stripped +29 -5
>> expanding multi_delete::send_error to 1. early return if
>> binlogged == t
>> 2. binlog locally if there were non-trans table modified side effect
>>
>> sql/sql_parse.cc@stripped, 2007-06-23 16:33:34+03:00,
> aelkin@stripped +7 -0
>> adding multi_update::send_error which can perform binlogging and rollback job
> in needed
>>
>> sql/sql_update.cc@stripped, 2007-06-23 16:33:34+03:00,
> aelkin@stripped +2 -2
>> a minor issue relating to bug_27716 with zeroing inside of binlogging branch.
> Moved outside to avoid double work in ::send_error()
>> when binlog is off.
>>
>> # This is a BitKeeper patch. What follows are the unified diffs for the
>> # set of deltas contained in the patch. The rest of the patch, the part
>> # that BitKeeper cares about, is below these diffs.
>> # User: aelkin
>> # Host: dsl-hkibras1-ff5dc300-70.dhcp.inet.fi
>> # Root: /home/elkin/MySQL/TEAM/FIXES/5.0/bug29136-mdelete
>>
>> --- 1.198/sql/sql_delete.cc 2007-04-20 13:30:47 +03:00
>> +++ 1.199/sql/sql_delete.cc 2007-06-23 16:33:33 +03:00
>> @@ -678,13 +678,16 @@ void multi_delete::send_error(uint errco
>> /* First send error what ever it is ... */
>> my_message(errcode, err, MYF(0));
>> -
>> - /* If nothing deleted return */
>> - if (!deleted)
>> + // TODO: merge with bug#27417,23333
>> + //thd->no_trans_update.top |= thd->no_trans_update.stmt;
>> + /* If nothing deleted and no side effects return */
>> + //if ((!thd->no_trans_update.top && !deleted) || binlogged)
>> + if (deleted == 0)
>> DBUG_VOID_RETURN;
>> /* Something already deleted so we have to invalidate cache */
>> - query_cache_invalidate3(thd, delete_tables, 1);
>> + if (deleted)
>> + query_cache_invalidate3(thd, delete_tables, 1);
>> /*
>> If rows from the first table only has been deleted and it is
>> @@ -703,8 +706,22 @@ void multi_delete::send_error(uint errco
>> error log
>> */
>> error= 1;
>> - send_eof();
>> + send_eof(); // deleted sets to zero
>> }
>> + //TODO the merge: if (thd->no_trans_update.top)
>> + if (deleted != 0 && normal_tables)
>> + {
>> + /* if there is side effect - binlog with the error */
>> + if (mysql_bin_log.is_open())
>> + {
>> + Query_log_event qinfo(thd, thd->query, thd->query_length,
>> + transactional_tables, FALSE);
>> + mysql_bin_log.write(&qinfo);
>> + }
>> + if (thd->no_trans_update.stmt)
>> + thd->no_trans_update.all= TRUE;
>>
>
> This kind of solution worries me (it's not your change, bug the code
> it is based on). The basic property is that no_trans_update.stmt ==
> TRUE implies no_trans_update.all == TRUE, but what if it is forgotten
> to update the .all part? Wouldn't it be better to implement the
> structure so that .all is automatically updated whenever .stmt is
> updated? That way, it is impossible to forget to set the .all flag
> when .stmt is set.
>
Your note is reasonble. Moreover, this particular aspect was discussed
along bug#27417. The latest commit mail comments
http://lists.mysql.com/commits/31827 has the summary of a refactiring
that was done with the greatest help of Serg and Kostja.
> I'm not suggesting that you implement this now, but that you add a
> note somewhere that we should change this.
>
>> + }
>> + DBUG_ASSERT(!normal_tables || !deleted || thd->no_trans_update.stmt);
>> DBUG_VOID_RETURN;
>> }
>> @@ -819,9 +836,16 @@ bool multi_delete::send_eof()
>> if (mysql_bin_log.write(&qinfo) && !normal_tables)
>> local_error=1; // Log write failed: roll back the SQL statement
>> }
>> + /*
>> + flag the fact so that there will be the early return from
>> + send_error() if one will be called
>> + */
>> if (!transactional_tables)
>> thd->no_trans_update.all= TRUE;
>>
>
> Does this mean that the no_trans_update.all will be set to TRUE if and
> only if there are no transactional tables? Shouldn't it be that that
> no_trans_update.all should be set to TRUE if there is at least one
> non-transactional table?
To answer your question, the flag your are about is called since
bug#27417 as
thd->transaction.all.modified_non_trans_table
and hopefully the name says it all: whenever a non-transactional table
got modified within the scope of the current top level transaction
the flag is set to true.
The value for that flag is set true basing on
thd->transaction.stmt.modified_non_trans_table
which gets true first actually. The life time of it is the current
statement life time.
The transactional level flag is to be merged with the statement level at the
end of a statement by the rules of bug#27417 fixes.
The above hunk needed merging with bug#27417 refactoring.
Also multi_update,delete calls send_error() after send_eof() so that
we have to avoid binlogging, rollback job in the latter when it has
already been done in send_eof(). That's why I added a new member for
two classes - invite you to find in the cset.
>
>> }
>> + if (local_error)
>> + deleted= 0; // to force early leave from ::send_error()
>> + /* Commit or rollback the current SQL statement */
>> if (transactional_tables)
>> if (ha_autocommit_or_rollback(thd,local_error > 0))
>>
>> --- 1.623/sql/sql_parse.cc 2007-06-05 18:28:42 +03:00
>> +++ 1.624/sql/sql_parse.cc 2007-06-23 16:33:34 +03:00
>> @@ -3684,6 +3684,13 @@ end_with_restore_list:
>> SELECT_NO_JOIN_CACHE | SELECT_NO_UNLOCK |
>> OPTION_SETUP_TABLES_DONE,
>> del_result, unit, select_lex);
>> + res|= thd->net.report_error;
>> + if (unlikely(res))
>> + {
>> + /* If we had a another error reported earlier then this will be ignored
> */
>> + del_result->send_error(ER_UNKNOWN_ERROR, ER(ER_UNKNOWN_ERROR));
>> + del_result->abort();
>>
>
> Could you please use something more informative, at least as error
> message. I know there are tons of similar constructions in all kinds
> of places, but that it really not a good reason to continue that bad
> practice.
Okay.
I changed the text of the error message argument to say essentially
what the comments do.
>
>> + }
>> delete del_result;
>> }
>> else
>>
>> --- 1.220/sql/sql_update.cc 2007-06-05 02:14:07 +03:00
>> +++ 1.221/sql/sql_update.cc 2007-06-23 16:33:34 +03:00
>> @@ -1739,8 +1739,6 @@ bool multi_update::send_eof()
>> {
>> if (local_error == 0)
>> thd->clear_error();
>> - else
>> - updated= 0; /* if there's an error binlog it here not in ::send_error
> */
>> Query_log_event qinfo(thd, thd->query, thd->query_length,
>> transactional_tables, FALSE);
>> if (mysql_bin_log.write(&qinfo) && trans_safe)
>> @@ -1749,6 +1747,8 @@ bool multi_update::send_eof()
>> if (!trans_safe)
>> thd->no_trans_update.all= TRUE;
>> }
>> + if (local_error)
>> + updated= 0; // to force early leave from ::send_error()
>> if (transactional_tables)
>> {
>>
>> --- 1.168/mysql-test/r/innodb.result 2007-06-01 13:22:47 +03:00
>> +++ 1.169/mysql-test/r/innodb.result 2007-06-23 16:33:33 +03:00
>> @@ -1119,6 +1119,19 @@ show master status /* there must be no U
>> File Position Binlog_Do_DB Binlog_Ignore_DB
>> master-bin.000001 98
>> drop table t1, t2;
>> +drop table if exists t1, t2;
>> +CREATE TABLE t1 (a int, PRIMARY KEY (a));
>> +CREATE TABLE t2 (a int, PRIMARY KEY (a)) ENGINE=InnoDB;
>> +create trigger trg_del_t2 after delete on t2 for each row
>> +insert into t1 values (1);
>> +insert into t1 values (1);
>> +insert into t2 values (1),(2);
>> +delete t2 from t2;
>> +ERROR 23000: Duplicate entry '1' for key 1
>> +select count(*) from t2 /* must be 2 as restored after rollback caused by the
> error */;
>> +count(*)
>> +2
>> +drop table t1, t2;
>> create table t1 (a int, b int) engine=innodb;
>> insert into t1 values(20,null);
>> select t2.b, ifnull(t2.b,"this is null") from t1 as t2 left join t1 as t3 on
>> @@ -1675,14 +1688,14 @@ t2 CREATE TABLE `t2` (
>> drop table t2, t1;
>> show status like "binlog_cache_use";
>> Variable_name Value
>> -Binlog_cache_use 158
>> +Binlog_cache_use 159
>> show status like "binlog_cache_disk_use";
>> Variable_name Value
>> Binlog_cache_disk_use 0
>> create table t1 (a int) engine=innodb;
>> show status like "binlog_cache_use";
>> Variable_name Value
>> -Binlog_cache_use 159
>> +Binlog_cache_use 160
>> show status like "binlog_cache_disk_use";
>> Variable_name Value
>> Binlog_cache_disk_use 1
>> @@ -1691,7 +1704,7 @@ delete from t1;
>> commit;
>> show status like "binlog_cache_use";
>> Variable_name Value
>> -Binlog_cache_use 160
>> +Binlog_cache_use 161
>> show status like "binlog_cache_disk_use";
>> Variable_name Value
>> Binlog_cache_disk_use 1
>> @@ -1815,10 +1828,10 @@ Variable_name Value
>> Innodb_page_size 16384
>> show status like "Innodb_rows_deleted";
>> Variable_name Value
>> -Innodb_rows_deleted 2072
>> +Innodb_rows_deleted 2073
>> show status like "Innodb_rows_inserted";
>> Variable_name Value
>> -Innodb_rows_inserted 31732
>> +Innodb_rows_inserted 31734
>> show status like "Innodb_rows_updated";
>> Variable_name Value
>> Innodb_rows_updated 29532
>>
>> --- 1.141/mysql-test/t/innodb.test 2007-06-01 13:22:48 +03:00
>> +++ 1.142/mysql-test/t/innodb.test 2007-06-23 16:33:33 +03:00
>> @@ -793,6 +793,38 @@ show master status /* there must be no U
>> drop table t1, t2;
>> #
>> +# Bug #29136 erred multi-delete on trans table does not
>> rollback +#
>> +
>> +# prepare
>> +--disable_warnings
>> +drop table if exists t1, t2;
>> +--enable_warnings
>> +CREATE TABLE t1 (a int, PRIMARY KEY (a));
>> +CREATE TABLE t2 (a int, PRIMARY KEY (a)) ENGINE=InnoDB;
>> +create trigger trg_del_t2 after delete on t2 for each row
>> + insert into t1 values (1);
>> +insert into t1 values (1);
>> +insert into t2 values (1),(2);
>> +
>> +
>> +# exec cases A, B - see multi_update.test
>> +
>> +# A. send_error() w/o send_eof() previously branch
>> +
>> +--error ER_DUP_ENTRY
>> +delete t2 from t2;
>> +
>> +# check
>> +
>> +select count(*) from t2 /* must be 2 as restored after rollback caused by the
> error */;
>> +
>> +# cleanup bug#29136
>> +
>> +drop table t1, t2;
>> +
>> +
>> +#
>> # Testing of IFNULL
>> #
>> create table t1 (a int, b int) engine=innodb;
>>
>> --- 1.46/mysql-test/r/multi_update.result 2007-06-01 11:14:01 +03:00
>> +++ 1.47/mysql-test/r/multi_update.result 2007-06-23 16:33:33 +03:00
>> @@ -557,4 +557,19 @@ show master status /* there must be the File
>> Position Binlog_Do_DB Binlog_Ignore_DB
>> master-bin.000001 204
>> drop table t1, t2;
>> +drop table if exists t1, t2, t3;
>> +CREATE TABLE t1 (a int, PRIMARY KEY (a));
>> +CREATE TABLE t2 (a int, PRIMARY KEY (a));
>> +CREATE TABLE t3 (a int, PRIMARY KEY (a)) ENGINE=MyISAM;
>> +create trigger trg_del_t3 after delete on t3 for each row
>> +insert into t1 values (1);
>> +insert into t2 values (1),(2);
>> +insert into t3 values (1),(2);
>> +reset master;
>> +delete t3.* from t2,t3 where t2.a=t3.a;
>> +ERROR 23000: Duplicate entry '1' for key 1
>> +show master status /* the query must be in binlog */;
>> +File Position Binlog_Do_DB Binlog_Ignore_DB
>> +master-bin.000001 199
>> +drop table t1, t2, t3;
>> end of tests
>>
>> --- 1.50/mysql-test/t/multi_update.test 2007-06-01 11:14:01 +03:00
>> +++ 1.51/mysql-test/t/multi_update.test 2007-06-23 16:33:33 +03:00
>> @@ -574,4 +574,36 @@ show master status /* there must be the #
>> cleanup bug#27716
>> drop table t1, t2;
>> +#
>> +# Bug #29136 erred multi-delete on trans table does not
>> rollback +#
>> +
>> +# prepare
>> +--disable_warnings
>> +drop table if exists t1, t2, t3;
>> +--enable_warnings
>> +CREATE TABLE t1 (a int, PRIMARY KEY (a));
>> +CREATE TABLE t2 (a int, PRIMARY KEY (a));
>> +CREATE TABLE t3 (a int, PRIMARY KEY (a)) ENGINE=MyISAM;
>> +create trigger trg_del_t3 after delete on t3 for each row
>> +insert into t1 values (1);
>> +insert into t2 values (1),(2);
>> +insert into t3 values (1),(2);
>> +reset master;
>> +
>> +# exec cases B, A - see innodb.test
>> +
>> +# B. send_eof() and send_error() afterward
>> +
>> +--error ER_DUP_ENTRY
>> +delete t3.* from t2,t3 where t2.a=t3.a;
>> +
>> +# check
>> +
>> +show master status /* the query must be in binlog */;
>> +
>> +# cleanup bug#29136
>> +drop table t1, t2, t3;
>> +
>> +
>> --echo end of tests
>>
>
> Could you please add a test that demonstrates that the behavior is
> correct even for replication cases? Pick a test case involving a mix
> of transactional and non-transactional tables, as well as cases with
> only one or the other.
>
Okay.
I added the mixture of ta and not-ta tables tests for both
multi-update and multi-detete. On the way of doing that I hit the new
bug so that I found it's better to be fixed right
away.
There has been pure myisam (multi_update.test) and pure innodb (innodb.test)
multi tests in the older patch.
