Hello Sergei, Here is a new version of the patch following your comments. I let you decide if SET_CONSISTENCY must also be part of TRADITIONAL and ANSI. Regards, Jérôme.

-----Message d'origine----- De : Sergei Golubchik [mailto:serg@mariadb.org] Envoyé : jeudi 11 janvier 2018 23:17 À : jerome brauge Cc : bar@mariadb.org Objet : Re: Patch for MDEV-13417 - UPDATE produces wrong values if an updated column is later used as an update source

Hi, Jérôme!

Happy New Year!

Sorry for the delay.

But I've finally got around to reviewing your MDEV-13417

The patch was pretty good and with good tests too!

I had only a few minor comments, see below. If you'd like, I can apply your patch and do these changes myself.

See the review below.

On Dec 28, jerome brauge wrote:

...

Hello Sergei, Do you have time to review my patch for this mdev ? This is very important for us because I don't see any workaround (without unique key and cursor).

...

diff --git a/sql/sql_class.h b/sql/sql_class.h index f30b442..a5e2104 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -139,6 +139,7 @@ enum enum_binlog_row_image { #define MODE_HIGH_NOT_PRECEDENCE (1ULL << 29) #define MODE_NO_ENGINE_SUBSTITUTION (1ULL << 30) #define MODE_PAD_CHAR_TO_FULL_LENGTH (1ULL << 31) +#define MODE_SET_CONSISTENCY (1ULL << 32)

I don't particularly like "SET_CONSISTENCY" name, but cannot come up with a good alternative. It doesn't matter, renaming the mode is a trivial change that doesn't affect anything else. So, unless you have a strong preference for some specific mode name - just ignore this naming issue completely. And I'll ask our documentation guys if they could suggest something.

...

/* Bits for different old style modes */ #define OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE (1 << 0) diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 60247c8..debb81a 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -7952,15 +7954,41 @@ fill_record(THD *thd, TABLE *table_arg,

List<Item> &fields, List<Item> &values,

...

ER_THD(thd,

ER_WARNING_NON_DEFAULT_VALUE_FOR_VIRTUAL_COLUMN),

...

rfield->field_name.str, table->s->table_name.str); } - if (rfield->stored_in_db() && - (value->save_in_field(rfield, 0)) < 0 && !ignore_errors) + if (rfield->stored_in_db()) { - my_message(ER_UNKNOWN_ERROR, ER_THD(thd,

ER_UNKNOWN_ERROR), MYF(0));

...

- goto err; + if (value->save_in_field(rfield, 0) < 0 && !ignore_errors) + { + my_message(ER_UNKNOWN_ERROR, ER_THD(thd, ER_UNKNOWN_ERROR), MYF(0)); + goto err; + } + /* + ** In sql MODE_SET_CONSISTENCY, + ** move field pointer on value stored in record[1] + ** which contains row before update (see MDEV-13417) + */ + if (update && thd->variables.sql_mode & MODE_SET_CONSISTENCY) + rfield->move_field_offset((my_ptrdiff_t) (table->record[1] - + table->record[0]));

Cool! A simple and nice solution :)

...

} rfield->set_explicit_default(value); }

+ if (update && thd->variables.sql_mode & MODE_SET_CONSISTENCY) { + // restore fields pointers on record[0] + f.rewind(); + while ((fld= f++)) + { + rfield= fld->field_for_view_update()->field; + if (rfield->stored_in_db()) + { + table= rfield->table; + rfield->move_field_offset((my_ptrdiff_t) (table->record[0] - + table->record[1])); + } + } + } + if (!update && table_arg->default_field && table_arg->update_default_fields(0, ignore_errors)) goto err; diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 568dd40..5796b96 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -148,7 +148,25 @@ bool compare_record(const TABLE *table) we make temporary copy of Item_field, to avoid influence of changing result_field on Item_ref which refer on this field */ thd->change_item_tree(it.ref(), new (thd->mem_root) Item_field(thd,

field));

...

} + } + + if (thd->variables.sql_mode & MODE_SET_CONSISTENCY) { + // Make sure that a column is updated only once + List_iterator_fast<Item> it(items); + List<Item> ul; + ul.empty(); + while ((item= it++) && !ul.add_unique(item, &cmp_items)) ;

Dunno, it's O(N²) and item->eq() is not trivial either. perhaps it'd be faster, say, mark fields some way, like

while ((item= it++)) { Field *f= item->field_for_view_update()->field; if (f->has_explicit_value()) { my_error(ER_UPDATED_COLUMN_ONLY_ONCE, MYF(0), f->table_name, f->field_name.str); return TRUE; } f->set_has_explicit_value(); }

note that this assumes the change in the error message, %s -> %`s.%`s

...

+ + if (item) + { + // same field appear more than once in a single update statement. + my_error(ER_UPDATED_COLUMN_ONLY_ONCE, MYF(0), item- full_name()); + return TRUE; + } + } return FALSE; } diff --git a/mysql-test/r/set_consistency.result b/mysql-test/r/set_consistency.result new file mode 100644 index 0000000..747b6b3 --- /dev/null +++ b/mysql-test/r/set_consistency.result @@ -0,0 +1,185 @@ +SET

+sql_mode='STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_A UTO_CREA

...

+TE_USER,NO_ENGINE_SUBSTITUTION,SET_CONSISTENCY';

shouldn't SET_CONSISTENCY be part of TRADITIONAL, ORACLE, and ANSI?

...

+# +# MDEV-13417 UPDATE produces wrong values if an UPDATEd column is +later used as an UPDATE source # MDEV-13418 Compatibility: The order +of evaluation of SELECT..INTO assignments # CREATE TABLE t1 (c1 +INTEGER, c2 INTEGER, c3 INTEGER) ENGINE=InnoDb; INSERT INTO +t1(c1,c2,c3) VALUES (1,1,1); CREATE TABLE t2 (c1 INTEGER, c2 +INTEGER, c3 INTEGER) ENGINE=InnoDb; INSERT INTO t2(c1,c2,c3) VALUES +(1,1,1); + +Check that a column is only updated once. +

please, comment out these messages, just like the MDEV numbers above

...

+UPDATE t1 +SET c1 = 1, +c1 = 2; +ERROR HY000: The column test.t1.c1 cannot be changed more than once +in a single UPDATE statement

Regards, Sergei Chief Architect MariaDB and security@mariadb.org