Hi Andrei!
Here are my comments on the patch.
Just my few cents,
Mats Kindahl
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.
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?
> }
> + 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.
> + }
> 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.
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
