maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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)

> 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));

> +                       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).

> +  // 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?

>            (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?

> +    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?

> 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.

> +  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