Thank you for the very nice review!
The scope of the patch is to only address some simple cases for this
problem, it will not try to fix this problem completely. If we would try
to cover all cases, the code would be very complicated and would bring
considerable performance impact. So after we discussed this in the
meeting, we made the decision to only address some simple and most
So when I work on this patch, I only want to fix the following cases:
INSERT..SELECT one_table LIMIT
UNIONs and JOINs are not considered in this patch. And also in ORDER BY,
only primary keys are checked, but in fact, any non-null unique keys
should also make the statement safe. This is also ignored intentionally.
I'll check your comments carefully and to see if we can handle more
cases that are not complicated.
Thank you again for the nice and detailed review!
Sven Sandberg wrote:
> Hi Zhenxing,
> Thank you for this work. This is a good start, but it seems that too
> many warnings are suppressed. There are some minor issues in the code,
> and I think it would be good to have more coverage in the test case. So
> the patch is rejected.
> Detailed comments (references inline):
> (1) When LIMIT is part of a SELECT in a UNION, it does not give a
> warning at all now. I don't know why.
> =============== BEGIN TEST CASE =================
> --source include/have_binlog_format_statement.inc
> CREATE TABLE t1 (a INT);
> CREATE TABLE t2 (a INT);
> CREATE TABLE t3 (a INT);
> CREATE TABLE t4 (a INT);
> CREATE TABLE t5 (a INT);
> CREATE TABLE t6 (a INT);
> INSERT INTO t1 VALUES (1), (2);
> INSERT INTO t2 VALUES (3), (4);
> INSERT INTO t3 (SELECT a FROM t1 LIMIT 1) UNION (SELECT a FROM t2) LIMIT 1;
> INSERT INTO t4 (SELECT a FROM t1 LIMIT 1) UNION (SELECT a FROM t2 LIMIT 1);
> INSERT INTO t5 (SELECT a FROM t1) UNION (SELECT a FROM t2 LIMIT 1);
> INSERT INTO t6 (SELECT a FROM t1 LIMIT 1) UNION (SELECT a FROM t2);
> SELECT * FROM t3;
> SELECT * FROM t4;
> SELECT * FROM t5;
> SELECT * FROM t6;
> =============== END TEST CASE =================
> Note: the line "INSERT INTO t6" above was not marked unsafe before your
> patch either. I filed BUG#55211. Please keep that bug in mind: perhaps
> you will fix it as part of your patch.
will check that
> (2) Please don't call set_current_stmt_binlog_row_based_if_mixed(). It
> is enough to call set_stmt_unsafe(). Then decide_logging_format() will
> call set_current_stmt_binlog_row_based_if_mixed().
> (3) is_order_deterministic() seems more appropriate to place in
> sql_select.cc since it has to do with select statements.
> (4) The explicit type conversions in sql_select.cc and sql_delete.cc are
> redundant, because select_lex->table_list.first is already TABLE_LIST*
> and select_lex->order_list.first (in sql_select.cc) respectively
> order.first (in sql_delete.cc) is already ORDER*.
No, select_lex->table_list is a SQL_LIST *, not TABLE_LIST *
> (5) The explicit type conversion in
> sql_update.cc:is_order_deterministic() looks suspicious. How can we be
> sure that order->item is of type Item_field* ? If order->item is always
> of type Item_field*, then can we instead change the type to Item_field
> and avoid the explicit type conversion?
Yes, you're right
> (6) The test case misses many cases. I think the following would be good
> to test (all combinations where you pick one element from each list):
> (6.1) Statement types:
> - INSERT
> - DELETE
> - UPDATE
> (6.2) Compound statements:
> - joins
> - unions
> - subqueries in the FROM clause
> - subqueries with EXISTS/NOT EXISTS/SOME/ALL/ANY/IN
These are the cases that I want to avoid and will not try to fix.
> (6.3) LIMIT clauses:
> - LIMIT 0 (safe)
> - LIMIT 1 (unsafe)
> - LIMIT 1 ORDER BY non-pk (unsafe)
> - LIMIT 1 ORDER BY pk (safe)
> - LIMIT 1 ORDER BY pk_over_several_columns (safe)
> - LIMIT 1 ORDER BY unique_key_not_null (safe)
As I have commented before, this is left out deliberately, This is not
hard to implement, what concerns me is the possible performance impact
if user defined a lot of keys.
> (6.4) LIMIT position:
> - on top-level statement
> - on sub-statement
I think we should not try to handle this.
> If we implement this systematically, we know we have reasonable
> coverage. It also makes it clear to us which cases of redundant
> unsafeness are still there; this would help if we need to refine the
> unsafeness detection even more.
> (7) Just a hint that you need to keep in mind when merging. In trunk and
> next-mr, the warning for unsafeness includes a message that says why the
> statement is unsafe. If the statement is unsafe for more than one
> reason, then all reasons are printed. So in sql_select.cc, make sure
> that it checks unsafeness even if thd->lex->is_stmt_unsafe() is true.
> He Zhenxing wrote:
> > #At file:///media/sdb2/hezx/work/mysql/bzr/b42415/5.1-bugteam/ based on
> > 3418 He Zhenxing 2010-07-11
> > Bug #42415 UPDATE/DELETE with LIMIT clause unsafe for SBL even with ORDER
> BY PK clause