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(<ime, 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