Re: [Maria-developers] Please review: MDEV-7824 [Bug #68041] Zero date can be inserted in strict no-zero mode through a default value
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 diff --git a/mysql-test/include/type_temporal_zero_default.inc b/mysql-test/include/type_temporal_zero_default.inc new file mode 100644 index 000..500d25e --- /dev/null +++ b/mysql-test/include/type_temporal_zero_default.inc @@ -0,0 +1,75 @@ +--echo # +--echo # MDEV-7824 [Bug #68041] Zero date can be inserted in strict no-zero mode through a default value +--echo # + +# Testing direct INSERT + +SET sql_mode=DEFAULT; +eval CREATE TABLE t1 (a $type DEFAULT $defval); +SET sql_mode=TRADITIONAL; +--error ER_TRUNCATED_WRONG_VALUE +eval INSERT INTO t1 VALUES ($defval); +--error ER_INVALID_DEFAULT_VALUE_FOR_FIELD +INSERT INTO t1 VALUES (); +--error ER_INVALID_DEFAULT_VALUE_FOR_FIELD +INSERT INTO t1 VALUES (DEFAULT); +DROP TABLE t1; +SET sql_mode=DEFAULT; + + +# Testing INSERT .. SELECT + +eval CREATE TABLE t1 (a $type NOT NULL DEFAULT $defval, b $type NOT NULL DEFAULT $defval); +eval CREATE TABLE t2 (a $type NOT NULL DEFAULT $defval); +eval INSERT INTO t2 VALUES ($defval); +SET sql_mode=TRADITIONAL; +--error ER_INVALID_DEFAULT_VALUE_FOR_FIELD +INSERT INTO t1 (a) SELECT a FROM t2; +DROP TABLE t1, t2; +SET sql_mode=DEFAULT; + + +# Testing LOAD + +--eval CREATE TABLE t1 (a $type DEFAULT $defval, b $type DEFAULT $defval) +--eval INSERT INTO t1 VALUES (DEFAULT,DEFAULT); +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR +--eval SELECT a INTO OUTFILE '$MYSQLTEST_VARDIR/tmp/mdev-7824.txt' FROM t1 +DELETE FROM t1; +SET sql_mode=TRADITIONAL; +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR +--error ER_INVALID_DEFAULT_VALUE_FOR_FIELD +--eval LOAD DATA INFILE '$MYSQLTEST_VARDIR/tmp/mdev-7824.txt' INTO TABLE t1 (a) +--remove_file $MYSQLTEST_VARDIR/tmp/mdev-7824.txt +DROP TABLE t1; +SET sql_mode=DEFAULT; + +# Testing ALTER when an old field default becomes invalid +# Return an error, even if there is no STRICT_XXX_TABLES set +--eval CREATE TABLE t1 (a $type DEFAULT $defval); +SET sql_mode='NO_ZERO_DATE'; +--error ER_INVALID_DEFAULT +ALTER TABLE t1 ADD b INT NOT NULL; +DROP TABLE t1; +SET sql_mode=DEFAULT; + + +--echo # +--echo # End of MDEV-7824 [Bug #68041] Zero date can be inserted in strict no-zero mode through a default value +--echo # + +--echo # +--echo # MDEV-8373 Zero date can be inserted in strict no-zero mode through CREATE TABLE AS SELECT timestamp_field +--echo # + +SET sql_mode=DEFAULT; +--eval CREATE TABLE t1 (a $type); +INSERT INTO t1 VALUES (0); +SET sql_mode='TRADITIONAL'; +--error ER_TRUNCATED_WRONG_VALUE
Re: [Maria-developers] Please review: MDEV-7824 [Bug #68041] Zero date can be inserted in strict no-zero mode through a default value
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
+ StringBufferMAX_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()!=
Re: [Maria-developers] Please review: MDEV-7824 [Bug #68041] Zero date can be inserted in strict no-zero mode through a default value
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
+ StringBufferMAX_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?
+
Re: [Maria-developers] Please review: MDEV-7824 [Bug #68041] Zero date can be inserted in strict no-zero mode through a default value
Hi Sergei,
Thanks for review! See my comments inline:
On 05/07/2015 06:57 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Mar 25, Alexander Barkov wrote:
Hi Sergei,
Please review a patch for mdev-7824.
It's based on a MySQL patch for
http://bugs.mysql.com/bug.php?id=68041
and is a blocker for:
MDEV-3929 Add full support for auto-initialized/updated timestamp and
datetime
That looks good.
But I don't like it that you verify defaults for every row.
It's not very cheap. And neither defaults nor sql_mode can change in the
duration of one statement. It should be enough to test only once.
This is a good idea. I copied the patch from MySQL. So it will be nice
to have this implemented in MariaDB in a faster way.
I'm not sure, though, how to do that with a minimal overhead. A couple
of bool's in TABLE, like
bool all_default_are_checked;
bool write_set_defaults_are_checked;
that are set to false at the beginning on INSERT,INSERT...SELECT,LOAD.
And used like that
bool TABLE::restore_record_and_validate_unset_fields(THD *thd, bool all)
{
restore_record(this, s-default_values);
if (all ? all_default_are_checked : write_set_defaults_are_checked)
return false;
if (all) all_default_are_checked= true;
else write_set_defaults_are_checked= true;
return validate_default_values_of_unset_fields(thd);
}
and in mysql_insert():
if (fields.elements || !value_count)
{
if (table-restore_record_and_validate_unset_fields(thd, !value_count))
Sorry, I did't understand your idea about having two separate
flags for all and write_set.
Is it for the cases when one mixes empty and non-empty parenthesized
value lists, like these:
insert into (a,b) t1 values (),(10,20);
insert into (a,b) t1 values (10,20),();
???
Note, this currently does not work and return this error:
ERROR 1136 (21S01): Column count doesn't match value count at row 2
Looks like a bug. I guess this could work.
If so, would you like me to report and fix it before mdev-7824?
Thanks.
Regards,
Sergei
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Please review: MDEV-7824 [Bug #68041] Zero date can be inserted in strict no-zero mode through a default value
Hi, Alexander!
On Mar 25, Alexander Barkov wrote:
Hi Sergei,
Please review a patch for mdev-7824.
It's based on a MySQL patch for
http://bugs.mysql.com/bug.php?id=68041
and is a blocker for:
MDEV-3929 Add full support for auto-initialized/updated timestamp and
datetime
That looks good.
But I don't like it that you verify defaults for every row.
It's not very cheap. And neither defaults nor sql_mode can change in the
duration of one statement. It should be enough to test only once.
I'm not sure, though, how to do that with a minimal overhead. A couple
of bool's in TABLE, like
bool all_default_are_checked;
bool write_set_defaults_are_checked;
that are set to false at the beginning on INSERT,INSERT...SELECT,LOAD.
And used like that
bool TABLE::restore_record_and_validate_unset_fields(THD *thd, bool all)
{
restore_record(this, s-default_values);
if (all ? all_default_are_checked : write_set_defaults_are_checked)
return false;
if (all) all_default_are_checked= true;
else write_set_defaults_are_checked= true;
return validate_default_values_of_unset_fields(thd);
}
and in mysql_insert():
if (fields.elements || !value_count)
{
if (table-restore_record_and_validate_unset_fields(thd, !value_count))
Regards,
Sergei
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
