Hi, Jacob! On Mar 28, jacob.mathew@mariadb.com wrote:
revision-id: b4a223e338152c8764c980aeae59fb927eea1b0b (mariadb-10.2.4-98-gb4a223e3381) parent(s): 3f7455c03008b2428fa9b364b1add4c36834ff71 author: Jacob Mathew committer: Jacob Mathew timestamp: 2017-03-28 17:41:56 -0700 message:
MDEV-10355 Weird error message upon CREATE TABLE with DEFAULT
Fixed handling of default values with cached temporal functions so that the CREATE TABLE statement now succeeds. Added clearing of cached function values to trigger function reevaluation when updating default values and when updating virtual column values. Fixed the error message. Added quoting of date/time values in cases when this was omitted. Added a test case. Updated test result files that include date/time values that were previously unquoted.
diff --git a/mysql-test/r/func_default_between_temporal.result b/mysql-test/r/func_default_between_temporal.result new file mode 100644 index 00000000000..db9656f4d37 --- /dev/null +++ b/mysql-test/r/func_default_between_temporal.result @@ -0,0 +1,15 @@ +CREATE OR REPLACE TABLE t1 ( col INT DEFAULT ( 1 LIKE ( NOW() BETWEEN '2000-01-01' AND '2012-12-12' ) ) ); +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `col` int(11) DEFAULT (1 like (current_timestamp() between '2000-01-01 00:00:00' and '2012-12-12 00:00:00')) +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +SET timestamp = UNIX_TIMESTAMP( '2004-04-04' ); +INSERT INTO t1 VALUES( DEFAULT ); +SET timestamp = DEFAULT;
better use another constant here, don't rely on the current timestamp being between 2000 and 2012. I know, sounds crazy. But debian's reproducible builds (*) do everything with the system time seriously off. (*) https://wiki.debian.org/ReproducibleBuilds
+INSERT INTO t1 VALUES( DEFAULT ); +SELECT * FROM t1; +col +1 +0 +DROP TABLE t1; diff --git a/mysql-test/t/func_default_between_temporal.test b/mysql-test/t/func_default_between_temporal.test
please, rename the test somehow or put it into default.test. "func_default" means it's about a DEFAULT() function, but your test is not.
new file mode 100644 index 00000000000..6cbd5a342c6 --- /dev/null +++ b/mysql-test/t/func_default_between_temporal.test @@ -0,0 +1,11 @@
Start the test with a header, like # # MDEV-10355 Weird error message upon CREATE TABLE with DEFAULT #
+CREATE OR REPLACE TABLE t1 ( col INT DEFAULT ( 1 LIKE ( NOW() BETWEEN '2000-01-01' AND '2012-12-12' ) ) ); +SHOW CREATE TABLE t1; + +SET timestamp = UNIX_TIMESTAMP( '2004-04-04' ); +INSERT INTO t1 VALUES( DEFAULT ); +SET timestamp = DEFAULT; +INSERT INTO t1 VALUES( DEFAULT ); + +SELECT * FROM t1; + +DROP TABLE t1; diff --git a/sql/item.cc b/sql/item.cc index c34d27fa63b..ee3e43b60ad 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -616,8 +616,9 @@ void Item::print_value(String *str) str->append("NULL"); else { - switch (result_type()) { + switch (cmp_type()) { case STRING_RESULT: + case TIME_RESULT:
okay, that fixes quoting. good.
append_unescaped(str, ptr->ptr(), ptr->length()); break; case DECIMAL_RESULT: @@ -9433,17 +9433,26 @@ void Item_cache::store(Item *item)
void Item_cache::print(String *str, enum_query_type query_type) { - if (value_cached) + if ( example && ( example->type() == FUNC_ITEM ) && // There is a cached function item + ( query_type & QT_ITEM_IDENT_SKIP_TABLE_NAMES ) ) // Caller is show-create-table
1. better test for QT_ITEM_CACHE_WRAPPER_SKIP_DETAILS 2. Why the test for FUNC_ITEM? I don't see any logical reason why skipping of "<cache>" should only apply to functions. and please watch the spacing around parentheses, try to use the same coding style as the rest of the code
{ - print_value(str); - return; + // Instead of "cache" or the cached value, print the function name + example->print( str, query_type ); } - str->append(STRING_WITH_LEN("<cache>(")); - if (example) - example->print(str, query_type); else - Item::print(str, query_type); - str->append(')'); + { + if (value_cached)
Indentation. please, try to use the same coding style as the rest of the code
+ { + print_value(str); + return; + }
It's not very logical that the value is not printed if one doesn't want "<cache>". I think this method could be like this: ================================== void Item_cache::print(String *str, enum_query_type query_type) { if (value_cached && !(query_type & QT_NO_DATA_EXPANSION)) { print_value(str); return; } if (!(query_type & QT_ITEM_CACHE_WRAPPER_SKIP_DETAILS)) str->append(STRING_WITH_LEN("<cache>(")); if (example) example->print(str, query_type); else Item::print(str, query_type); if (!(query_type & QT_ITEM_CACHE_WRAPPER_SKIP_DETAILS)) str->append(')'); } ================================== That is, print the value unless QT_NO_DATA_EXPANSION is specified. Print "<cache>" unless QT_ITEM_CACHE_WRAPPER_SKIP_DETAILS is specified. And Virtual_column_info::print() should also specify QT_NO_DATA_EXPANSION.
+ str->append(STRING_WITH_LEN("<cache>(")); + if (example) + example->print(str, query_type); + else + Item::print(str, query_type); + str->append(')'); + } }
/** diff --git a/sql/item.h b/sql/item.h index 67640ce5f4d..222f8580cb4 100644 --- a/sql/item.h +++ b/sql/item.h @@ -1653,6 +1653,7 @@ class Item: public Value_source, { return mark_unsupported_function(full_name(), arg, VCOL_IMPOSSIBLE); } + virtual bool clear_cached_function_value(void *arg) { return 0; } virtual bool check_field_expression_processor(void *arg) { return 0; } virtual bool check_func_default_processor(void *arg) { return 0; } /* @@ -5450,8 +5451,21 @@ class Item_cache: public Item_basic_constant, } bool check_vcol_func_processor(void *arg) { + if ( example ) + { + return example->check_vcol_func_processor( arg );
whitespace around parentheses and indentation
+ } return mark_unsupported_function("cache", arg, VCOL_IMPOSSIBLE); } + bool clear_cached_function_value( void *arg ) + { + if ( example && example->type() == Item::FUNC_ITEM ) + { + // Clear the cached function value to trigger reevaluation + clear(); + } + return false; + }
Why did you need a dedicated clear_cached_function_value() processor? There's Item::cleanup() that should be called from fix_session_vcol_expr(). Oh, okay, I see two issues here. First, item->cleanup() does not recurse into function arguments. That's my bug, I should've used something like - vcol->expr->cleanup(); + vcol->expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0); It has to be fixed, it's a genuine bug. A test case could be create table t1 (n varchar(100), cu varchar(100) default current_user(), uc varchar(100) default concat(current_user())); create definer=foo@localhost view v1 as select * from t1; insert v1 (n) values ('v1'); select * from t1; This is a slightly modified test from default_session.test file. Second, it's incorrect for Item_cache to use simply example->check_vcol_func_processor(arg); because caching requires re-fixing, even if the underlying example item doesn't. So, this should be something like bool check_vcol_func_processor(void *arg) { if ( example ) { Item::vcol_func_processor_result *res= (Item::vcol_func_processor_result*)arg; example->check_vcol_func_processor(arg); if (res->errors & VCOL_NOT_STRICTLY_DETERMINISTIC) res->errors|= VCOL_SESSION_FUNC; return false; } return mark_unsupported_function("cache", arg, VCOL_IMPOSSIBLE); } this sets VCOL_SESSION_FUNC flag, which will require item to be re-fixed for every statement. But doesn't set it for true constants. With these two fixes Item_cache::cleanup() { clear(); } should works just fine.
/** Check if saved item has a non-NULL value. Will cache value of saved item if not already done. diff --git a/sql/table.cc b/sql/table.cc index 3a08d1e49ea..9ae035e41aa 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -2802,9 +2802,9 @@ static bool fix_and_check_vcol_expr(THD *thd, TABLE *table,
int error= func_expr->walk(&Item::check_vcol_func_processor, 0, &res); if (error || (res.errors & VCOL_IMPOSSIBLE)) - { // this can only happen if the frm was corrupted + {
this comment is still true, don't delete it. and there's no need to pass the type inside anymore.
my_error(ER_VIRTUAL_COLUMN_FUNCTION_IS_NOT_ALLOWED, MYF(0), res.name, - "???", "?????"); + vcol_type_name((enum_vcol_info_type) vcol_type), vcol->name.str); DBUG_RETURN(1); } vcol->flags= res.errors; @@ -7374,6 +7374,10 @@ int TABLE::update_virtual_fields(handler *h, enum_vcol_update_mode update_mode)
if (update) { + // Clear any cached function values in the virtual field expression + // to trigger their reevaluation + vcol_info->expr->walk( &Item::clear_cached_function_value, 0, 0 ); // Always returns 0
There's no need to do that for every update_virtual_fields(), that is, no need to do it for every row, only needs to be done once per statement. And fix_session_vcol_expr() will do it for you if you implement Item_cache::cleanup() and set VCOL_SESSION_FUNC and fix fix_session_vcol_expr() to cleanup recursively.
+ int field_error __attribute__((unused)) = 0; /* Compute the actual value of the virtual fields */ if (vcol_info->expr->save_in_field(vf, 0))
Regards, Sergei Chief Architect MariaDB and security@mariadb.org