maria-developers team mailing list archive

[jerome brauge <j.brauge@xxxxxxxxxxx>]

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@xxxxxxxxxxx]
> Envoyé : jeudi 11 janvier 2018 23:17
> À : jerome brauge
> Cc : bar@xxxxxxxxxxx
> 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@xxxxxxxxxxx

Attachment: mdev13417.diff
Description: mdev13417.diff