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

Hi Sergei, This is a new version. Please also see some comments below. Thanks. On 06/25/2015 01:00 PM, Sergei Golubchik wrote:

Hi, Alexander!

On Jun 25, Alexander Barkov wrote:

...

Hi Sergei,

...

...

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?

I created a separate MDEV-8373 for CREATE AS SELECT.

better not.

it's trivial to put it in a separate commit with "git citool" if it'd be difficult to extract it - then yes, but mention is explicitly in the commit comment ("also fixes the case....").

I am adding tests for both bugs: MDEV-7824 [Bug #68041] Zero date can be inserted in strict no-zero mode through a default value MDEV-8373 Zero date can be inserted in strict no-zero mode through CREATE TABLE AS SELECT timestamp_field into a new shared file mysql-test/include/type_temporal_zero_default.inc Does git citool support partial commit in such case? I.e. if I want to add a part of a new file in the first commit, and then add the rest of the new file in the second commit?

...

...

...

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'"

Do you agree with a new error?

yes, okay.

Regards, Sergei