maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,

Thanks for review.

I have a couple of questions before I can send a new version.
See the questions and other comments below.


On 06/24/2015 08:32 PM, Sergei Golubchik wrote:
> Hi, Alexander!
>
> > I rewrote the patch slightly, so now we don't need to remember
> > all_default_are_checked or write_set_defaults_are_checked.
> >
> > The default values are now checked before the query execution
> > and before the first restore_record() call. They are now
> > checked directly in table->s->default_values.
> >
> > I hope it's now faster and clearer comparing to the original patch
> > version from MySQL-5.6.
>
> Agree! It's much better now.
>
> See my comments and questions below.
>
> > diff --git a/sql/field.h b/sql/field.h
> > index 4ca493e..2b430cb 100644
> > --- a/sql/field.h
> > +++ b/sql/field.h
>
> This is ok, but then, please, see what other places in the code you can
> change to use your new helpers. In a separate commit, of course. I
> suspect there's a lot of code that's doing, like
>
>        /* Get the value from default_values */
>        diff= (my_ptrdiff_t) (orig_field->table->s->default_values-
>                              orig_field->table->record[0]);
>        orig_field->move_field_offset(diff);      // Points now at default_values
>        if (orig_field->is_real_null())
>          field->set_null();
>        else
>        {
>          field->set_notnull();
>          memcpy(field->ptr, orig_field->ptr, field->pack_length());
>        }
>        orig_field->move_field_offset(-diff);     // Back to record[0]
>
> (this is from create_tmp_table() in sql_select.cc)

Sure, I have created a task for this, not to forget:

MDEV-8372 Use helper methods introduced in MDEV-7824 all around the code


> > diff --git a/sql/field.cc b/sql/field.cc
> > index ba6d4ff..4a854a3 100644
> > --- a/sql/field.cc
> > +++ b/sql/field.cc
> > @@ -5980,12 +5993,22 @@ String *Field_newdate::val_str(String *val_buffer,
> >   bool Field_newdate::get_date(MYSQL_TIME *ltime,ulonglong fuzzydate)
> >   {
> >     uint32 tmp=(uint32) uint3korr(ptr);
> > -  ltime->day=   tmp & 31;
> > -  ltime->month= (tmp >> 5) & 15;
> > +  ltime->day=   num_to_day(tmp);
> > +  ltime->month= num_to_month(tmp);
> >     ltime->year=  (tmp >> 9);
>
> may be num_to_year() ? just for consistency.
>
> >     ltime->time_type= MYSQL_TIMESTAMP_DATE;
> >     ltime->hour= ltime->minute= ltime->second= ltime->second_part= ltime->neg= 0;
> > -  return validate_for_get_date(tmp, ltime, fuzzydate);
> > +  return validate_MMDD(tmp, ltime->month, ltime->day, fuzzydate);
> > +}
> > +
> > +
> > +bool
> > +Field_newdate::validate_value_in_record(THD *thd, const uchar *record) const
> > +{
> > +  DBUG_ASSERT(!is_null_in_record(record));
> > +  uint32 num= (uint32) uint3korr(ptr_in_record(record));
> > +  return validate_MMDD(num, num_to_month(num), num_to_day(num),
>
> on the other hand, I'd rather use get_date() here, instead of
> duplicating its logic. This applies to all other
> validate_value_in_record() methods too. May be like
>
>     MYSQL_TIME ltime;
>     get_date_from_ptr(&ltime, ptr_in_record(record), sql_mode_for_dates(thd));

get_date_from_ptr() will generally need to do more than
it's needed for validation. For example, for DATETIME, it will spend
CPU to initialize the TIME part unnecessarily.

But Field_datetimef does full initialization of MYSQL_TIME anyway,
as I didn't add a faster validation function in compat56.cc.

Okey, as validation is done only once per query, I'll agree that
it's better to have less duplicate code here than a super-optimized code.

I'll try your idea with get_date_from_ptr().



> > +                       sql_mode_for_dates(thd));
> >   }
> >
> >
> > @@ -10268,3 +10332,29 @@ void Field::set_explicit_default(Item *value)
> >       return;
> >     set_has_explicit_value();
> >   }
> > +
> > +
> > +bool Field::validate_value_in_record_with_warn(THD *thd, const uchar *record)
> > +{
> > +  if (!validate_value_in_record(thd, record))
> > +    return false;
> > +  /*
> > +    Only write_set[field_index] is set for "this",
> > +    while read_set[field_index] is not guaranteed to be set.
> > +    Set table->read_set to NULL,
> > +    to make ASSERT_COLUMN_MARKED_FOR_READ in val_str() happy.
> > +  */
> > +  MY_BITMAP *read_set_backup= table->read_set;
> > +  table->read_set= 0;
>
> Normally one uses dbug_tmp_use_all_columns() for that (without comment).

Thanks. Will use this.


> > +  // Get val_str() for the DEFAULT value
> > +  StringBuffer<MAX_FIELD_WIDTH> tmp;
> > +  val_str(&tmp, ptr_in_record(record));
> > +  // Convert val_str() result to a printable error parameter
> > +  ErrConvString err(&tmp);
> > +  push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
> > +                      ER_INVALID_DEFAULT_VALUE_FOR_FIELD,
> > +                      ER(ER_INVALID_DEFAULT_VALUE_FOR_FIELD),
> > +                      err.ptr(), field_name);
> > +  table->read_set= read_set_backup;
> > +  return true;
> > +}
> > diff --git a/sql/field_conv.cc b/sql/field_conv.cc
> > index e31f7c5..14f2947 100644
> > --- a/sql/field_conv.cc
> > +++ b/sql/field_conv.cc
> > @@ -859,7 +859,8 @@ bool memcpy_field_possible(Field *to,Field *from)
> >             from->charset() == to->charset() &&
> >             (!sql_mode_for_dates(to->table->in_use) ||
> >              (from->type()!= MYSQL_TYPE_DATE &&
> > -            from->type()!= MYSQL_TYPE_DATETIME)) &&
> > +            from->type()!= MYSQL_TYPE_DATETIME &&
> > +            from->type()!= MYSQL_TYPE_TIMESTAMP)) &&
>
> what is this for - to apply TIME_NO_ZERO_DATE to timestamps?

Right, this is for CREATE TABLE AS SELECT, like in here:

set sql_mode=default;
drop table if exists t1;
create table t1 (a timestamp);
insert into t1 values (0);
set sql_mode='TRADITIONAL';
drop table if exists t2;
create table t2 as select * from t1;


The last statement should fail. It does not fail before the patch.

This is not strictly DEFAULT. But a very related thing,
when a wrong value sneaks in going around Field_xxx::store().

Would you mind if I have this change in this patch?

Btw, I forgot to add the above script into tests. Will do.


> >             (from_real_type != MYSQL_TYPE_VARCHAR ||
> >              ((Field_varstring*)from)->length_bytes ==
> >              ((Field_varstring*)to)->length_bytes));
> > diff --git a/sql/item.cc b/sql/item.cc
> > index f685242..0f2813a 100644
> > --- a/sql/item.cc
> > +++ b/sql/item.cc
> > @@ -8184,7 +8184,12 @@ int Item_default_value::save_in_field(Field *field_arg, bool no_conversions)
> >         return 1;
> >       }
> >       field_arg->set_default();
> > -    return 0;
> > +    THD *thd= current_thd;
>
> you've recently spent a lot of efforts removing current_thd, is it
> necessary to add a new one here? can field_arg->table->in_use be used here?

Right, field_arg->table->in_use should be okey.
Thanks.

> > +    return
> > +      !field_arg->is_null_in_record(field_arg->table->s->default_values) &&
> > +       field_arg->validate_value_in_record_with_warn(thd,
> > +                                       field_arg->table->s->default_values) &&
> > +       thd->is_error() ? -1 : 0;
> >     }
> >     return Item_field::save_in_field(field_arg, no_conversions);
> >   }
> > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> > index 0846740..d79fbcc 100644
> > --- a/sql/share/errmsg-utf8.txt
> > +++ b/sql/share/errmsg-utf8.txt
> > @@ -7127,3 +7127,5 @@ ER_ROLE_DROP_EXISTS
> >           eng "Can't drop role '%-.64s'; it doesn't exist"
> >   ER_CANNOT_CONVERT_CHARACTER
> >           eng "Cannot convert '%s' character 0x%-.64s to '%s'"
> > +ER_INVALID_DEFAULT_VALUE_FOR_FIELD  22007
> > +        eng "Incorrect default value '%-.128s' for column '%.192s'"
>
> Hmm, it's not like you're creating a new error condition - invalid
> defaults were happening before too. What error was used for this case
> earlier?

There is an old error which only mentions the column name,
without the value:


ER_INVALID_DEFAULT 42000 S1009
        eng "Invalid default value for '%-.192s'"

which is called from CREATE TABLE.

> SET sql_mode=TRADITIONAL;
> DROP TABLE IF EXISTS t1;
> CREATE TABLE t1 (a DATE DEFAULT '0000-00-00');
> ERROR 1067 (42000): Invalid default value for 'a'

It's okey not to mention the value in CREATE TABLE, because it's visible 
in the statement that you've just typed.


But when you get the same error in a query like this:

> SET sql_mode=TRADITIONAL;
> INSERT INTO t1 (b) VALUES ('something');
> ERROR 1067 (42000): Invalid default value for 'a'

It might look confusing, and needs the author to spend time to figure 
out what actually the invalid value is (e.g. run SHOW CREATE TABLE).

Also, ER_INVALID_DEFAULT reports DDL related SQL State and error code, 
while the new message is rather DML related and so has SQL error code
22007, which is returned by other truncation warnings and errors.


Do you agree with a new error?

> > diff --git a/sql/table.cc b/sql/table.cc
> > index 5d2c188..a027b39 100644
> > --- a/sql/table.cc
> > +++ b/sql/table.cc
> > @@ -6896,6 +6896,40 @@ bool TABLE::prepare_triggers_for_update_stmt_or_event()
> >     return FALSE;
> >   }
> >
> > +
> > +/**
> > +  Validates default value of fields which are not specified in
> > +  the column list of INSERT/LOAD statement.
> > +
> > +  @Note s->default_values should be properly populated
> > +        before calling this function.
> > +
> > +  @param thd              thread context
> > +  @param record           the record to check values in
> > +
> > +  @return
> > +    @retval false Success.
> > +    @retval true  Failure.
> > +*/
> > +
> > +bool TABLE::validate_default_values_of_unset_fields(THD *thd) const
> > +{
> > +  DBUG_ENTER("TABLE::validate_default_values_of_unset_fields");
>
> please, add
>
>      DBUG_ASSERT(!thd->is_error());
>
> here, because you're relying on it later to detect errors.

Good idea, thanks.


> > +  for (Field **fld= field; *fld; fld++)
> > +  {
> > +    if (!bitmap_is_set(write_set, (*fld)->field_index) &&
> > +        !((*fld)->flags & NO_DEFAULT_VALUE_FLAG))
> > +    {
> > +      if (!(*fld)->is_null_in_record(s->default_values) &&
> > +          (*fld)->validate_value_in_record_with_warn(thd, s->default_values) &&
> > +          thd->is_error())
> > +        DBUG_RETURN(true); // strict mode converted WARN to ERROR
> > +    }
> > +  }
> > +  DBUG_RETURN(false);
> > +}
>
> Regards,
> Sergei