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:[email protected]] > Envoyé : jeudi 11 janvier 2018 23:17 > À : jerome brauge > Cc : [email protected] > 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 [email protected]
mdev13417.diff
Description: mdev13417.diff
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
