Hi, Jacob! On Mar 28, [email protected] 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 [email protected] _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
