Re: [Maria-developers] b4a223e3381: MDEV-10355 Weird error message upon CREATE TABLE with DEFAULT
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
Hi Sergei,
2. Why the test for FUNC_ITEM? I don't see any logical reason why skipping of "<cache>" should only apply to functions.
... 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(')'); } ==================================
The test for FUNC_ITEM is because SHOW CREATE TABLE needs to print the function name, not the currently cached value. The cached value is the constant value to which the function last evaluated. During SHOW CREATE TABLE, the value is always cached and query_type does not have the QT_NO_DATA_EXPANSION bit set. So, if the Item_cache::print() function was to be implemented as you suggest, then the output from SHOW CREATE TABLE for the test case would be incorrect: MariaDB [test]> show create table t1; +-------+--------------------------------------------------- ------------------------------------------------------------ --------------------------------------------------------------+ | Table | Create Table | +-------+--------------------------------------------------- ------------------------------------------------------------ --------------------------------------------------------------+ | t1 | CREATE TABLE `t1` ( `col` int(11) DEFAULT (1 like (*'2017-03-30 13:48:02'* between '2000-01-01 00:00:00' and '2012-12-12 00:00:00')) ) ENGINE=InnoDB DEFAULT CHARSET=latin1 | +-------+--------------------------------------------------- ------------------------------------------------------------ --------------------------------------------------------------+ 1 row in set (0.00 sec) The reason I differentiated functions in the totality of my changes is that functions need to be reevaluated. True constant values don't need to be reevaluated. So in the case of the BETWEEN in the test case, the function now() argument needs to be reevaluated, but its other 2 constant arguments may not need to be reevaluated because they appear to be true constants. I need to give some more thought to a few more cases, such as inserts from multiple time zones, before I can say with certainty whether or not they should be treated as true constants. Note that all 3 of these BETWEEN arguments are VCOL_NON_DETERMINISTIC. VCOL_NOT_STRICTLY_DETERMINISTIC, VCOL_NON_DETERMINISTIC and VCOL_SESSION_FUNC were introduced in my repository clone during a merge into my bb-10.2-MDEV-10355 branch a few days ago after I initially started working on this over a month ago before switching to 10.0 and 10.1. So I unfortunately didn't consider them in my changes. Right now, I miss Perforce's time lapse view. I need to get the same information from git. The bug fix in fix_session_vcol_expr() and the merge of the code in clear_cached_function_value() into cleanup() works fine!
... and there's no need to pass the type inside anymore.
and please watch the spacing around parentheses, try to use the same coding
The error message takes 3 variables. The vcol type is 1 of those 3 variables and is therefore necessary. After pulling the latest from 10.2, I don't see any newer way of getting the vcol type (not the field type) without using the parameter I passed in. Another way to do this would be to add the vcol type to the Virtual_column_info class. style as the rest of the code
Indentation. please, try to use the same coding style as the rest of the code whitespace around parentheses and indentation
My coding style has always employed additional whitespace for greater readability. If uniformity of the code is very important here, then I can adjust my coding style.
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.
I'm not sure I understand this comment. There are other tests that use the
timestamp variable and now() the same way as this test. So they would also
potentially fail on debian for the same reason.
Thanks,
Jacob
Jacob B. Mathew
Senior Software Engineer
MariaDB Corporation
+1 408 655 8999 <(408)%20655-8999> (mobile)
jacob.b.mathew (Skype)
jacob.mathew@MariaDB.com
On Thu, Mar 30, 2017 at 9:42 AM, Sergei Golubchik
Hi, Jacob!
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
On Mar 28, jacob.mathew@mariadb.com wrote: 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
Hi, Jacob! On Mar 30, Jacob Mathew wrote:
Hi Sergei,
2. Why the test for FUNC_ITEM? I don't see any logical reason why skipping of "<cache>" should only apply to functions.
... 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(')'); } ==================================
The test for FUNC_ITEM is because SHOW CREATE TABLE needs to print the function name, not the currently cached value.
Not really. Try - in your version of the function - to simply remove the check for FUNC_TYPE: if ( example && ( query_type & QT_ITEM_IDENT_SKIP_TABLE_NAMES ) ) // Caller is show-create-table you will see that everything works correctly. Perhaps even better than before, because constants are also printed as is, not converted.
The cached value is the constant value to which the function last evaluated. During SHOW CREATE TABLE, the value is always cached and query_type does not have the QT_NO_DATA_EXPANSION bit set. So, if the Item_cache::print() function was to be implemented as you suggest, then the output from SHOW CREATE TABLE for the test case would be incorrect:
That's what I wrote too:
And Virtual_column_info::print() should also specify QT_NO_DATA_EXPANSION.
Note that all 3 of these BETWEEN arguments are VCOL_NON_DETERMINISTIC.
What do you mean? Two constants are VCOL_NON_DETERMINISTIC?
VCOL_NOT_STRICTLY_DETERMINISTIC, VCOL_NON_DETERMINISTIC and VCOL_SESSION_FUNC were introduced in my repository clone during a merge into my bb-10.2-MDEV-10355 branch a few days ago after I initially started working on this over a month ago before switching to 10.0 and 10.1. So I unfortunately didn't consider them in my changes. Right now, I miss Perforce's time lapse view. I need to get the same information from git.
Feel free to ask me or Vicentiu on slack if you need any help with that.
The bug fix in fix_session_vcol_expr() and the merge of the code in clear_cached_function_value() into cleanup() works fine!
... and there's no need to pass the type inside anymore.
The error message takes 3 variables. The vcol type is 1 of those 3 variables and is therefore necessary. After pulling the latest from 10.2, I don't see any newer way of getting the vcol type (not the field type) without using the parameter I passed in. Another way to do this would be to add the vcol type to the Virtual_column_info class.
I mean, that this error can only happen if frm is corrupted. We do all validity checks before frm is created, and if the check after frm is opened produces different results, it means that the frm is corrupted. I think there's no need to try particularly hard to produce a very good and detailed error message for this very unlikely case. And the frm is corrupted anyway, my_error("FRM is corrupted") would work just as well. But this is up to you. If you want to pass type around and avoid question marks in the error message - let's do it.
and please watch the spacing around parentheses, try to use the same coding style as the rest of the code Indentation. please, try to use the same coding style as the rest of the code whitespace around parentheses and indentation
My coding style has always employed additional whitespace for greater readability. If uniformity of the code is very important here, then I can adjust my coding style.
Yes, please. In big projects it is important to have reasonably uniform code style. In particular this applies to spacing, indentation, curly braces. Multi-line comments use /* ... */, not //. But InnoDB has its own coding style. And Spider, probably, does too. One has to adjust to whatever code base he's working on (it was a pain until I configured my editor to use InnoDB coding style for everything inside storage/innodb/*).
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.
I'm not sure I understand this comment. There are other tests that use the timestamp variable and now() the same way as this test. So they would also potentially fail on debian for the same reason.
Debian Reproducible Builds, it's a project where they build debian packages in the enviromnent with some uncommon locale, system time seriously off (I saw +10 years once) and other strange changes. And then they expect the binary to be bytewise identical to the normally built binary. In particular Connect engine failed this, because it had __TIME__ and __DATE__ macros and we had to remove them. They never complained about tests, perhaps they don't run tests at all. So that was just a consideration that it might be safer not to rely on the current year. But it is not a strong reason. You can keep your current test, if you'd like. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei,
After doing more testing with the recursive call to
Item::cleanup_processor(), I have found problems with column default values
that reference another column as an argument to a date/time function. At
the point when fix_session_vcol_expr() is called,
Item::check_vcol_func_processor() has already been called recursively and
Item_field::check_vcol_func_processor() has set Name_resolution_context
*context= 0. This causes a crash when the recursive call to
Item::cleanup_processor() accesses members of context. I think I will need
to spend a lot more time learning this area to get this working. So, I am
currently leaning toward using a separate recursive call to
Item::clean_cached_value() in fix_session_vcol_expr() and leaving its
non-recursive call to cleanup() as is.
Is there a reason why the Virtual_column_info class doesn't include the
vcol type? Is there a reason why the Virtual_column_info class shouldn't
include the vcol type?
Thanks,
Jacob
Jacob B. Mathew
Senior Software Engineer
MariaDB Corporation
+1 408 655 8999 (mobile)
jacob.b.mathew (Skype)
jacob.mathew@MariaDB.com
On Fri, Mar 31, 2017 at 3:02 AM, Sergei Golubchik
Hi, Jacob!
On Mar 30, Jacob Mathew wrote:
Hi Sergei,
2. Why the test for FUNC_ITEM? I don't see any logical reason why skipping of "<cache>" should only apply to functions.
... 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(')'); } ==================================
The test for FUNC_ITEM is because SHOW CREATE TABLE needs to print the function name, not the currently cached value.
Not really. Try - in your version of the function - to simply remove the check for FUNC_TYPE:
if ( example && ( query_type & QT_ITEM_IDENT_SKIP_TABLE_NAMES ) ) // Caller is show-create-table
you will see that everything works correctly. Perhaps even better than before, because constants are also printed as is, not converted.
The cached value is the constant value to which the function last evaluated. During SHOW CREATE TABLE, the value is always cached and query_type does not have the QT_NO_DATA_EXPANSION bit set. So, if the Item_cache::print() function was to be implemented as you suggest, then the output from SHOW CREATE TABLE for the test case would be incorrect:
That's what I wrote too:
And Virtual_column_info::print() should also specify QT_NO_DATA_EXPANSION.
Note that all 3 of these BETWEEN arguments are VCOL_NON_DETERMINISTIC.
What do you mean? Two constants are VCOL_NON_DETERMINISTIC?
VCOL_NOT_STRICTLY_DETERMINISTIC, VCOL_NON_DETERMINISTIC and VCOL_SESSION_FUNC were introduced in my repository clone during a merge into my bb-10.2-MDEV-10355 branch a few days ago after I initially started working on this over a month ago before switching to 10.0 and 10.1. So I unfortunately didn't consider them in my changes. Right now, I miss Perforce's time lapse view. I need to get the same information from git.
Feel free to ask me or Vicentiu on slack if you need any help with that.
The bug fix in fix_session_vcol_expr() and the merge of the code in clear_cached_function_value() into cleanup() works fine!
... and there's no need to pass the type inside anymore.
The error message takes 3 variables. The vcol type is 1 of those 3 variables and is therefore necessary. After pulling the latest from 10.2, I don't see any newer way of getting the vcol type (not the field type) without using the parameter I passed in. Another way to do this would be to add the vcol type to the Virtual_column_info class.
I mean, that this error can only happen if frm is corrupted. We do all validity checks before frm is created, and if the check after frm is opened produces different results, it means that the frm is corrupted.
I think there's no need to try particularly hard to produce a very good and detailed error message for this very unlikely case. And the frm is corrupted anyway, my_error("FRM is corrupted") would work just as well.
But this is up to you. If you want to pass type around and avoid question marks in the error message - let's do it.
and please watch the spacing around parentheses, try to use the same coding style as the rest of the code Indentation. please, try to use the same coding style as the rest of the code whitespace around parentheses and indentation
My coding style has always employed additional whitespace for greater readability. If uniformity of the code is very important here, then I can adjust my coding style.
Yes, please. In big projects it is important to have reasonably uniform code style. In particular this applies to spacing, indentation, curly braces. Multi-line comments use /* ... */, not //.
But InnoDB has its own coding style. And Spider, probably, does too. One has to adjust to whatever code base he's working on (it was a pain until I configured my editor to use InnoDB coding style for everything inside storage/innodb/*).
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.
I'm not sure I understand this comment. There are other tests that use the timestamp variable and now() the same way as this test. So they would also potentially fail on debian for the same reason.
Debian Reproducible Builds, it's a project where they build debian packages in the enviromnent with some uncommon locale, system time seriously off (I saw +10 years once) and other strange changes. And then they expect the binary to be bytewise identical to the normally built binary. In particular Connect engine failed this, because it had __TIME__ and __DATE__ macros and we had to remove them.
They never complained about tests, perhaps they don't run tests at all. So that was just a consideration that it might be safer not to rely on the current year. But it is not a strong reason. You can keep your current test, if you'd like.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Jacob! On Apr 06, Jacob Mathew wrote:
Hi Sergei,
After doing more testing with the recursive call to Item::cleanup_processor(), I have found problems with column default values that reference another column as an argument to a date/time function. At the point when fix_session_vcol_expr() is called, Item::check_vcol_func_processor() has already been called recursively and Item_field::check_vcol_func_processor() has set Name_resolution_context *context= 0. This causes a crash when the recursive call to Item::cleanup_processor() accesses members of context.
Exactly. There is no need to run fix_fields for actual fields, for Item_field. See, I've written in the review - vcol->expr->cleanup(); + vcol->expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0); Note there is no "cleanup_excluding_fields_processor" in the code. There is cleanup_processor(), and it doesn't work because it tries to re-fix fields. And there is cleanup_excluding_const_fields_processor(), it doesn't work either, because it only excludes const fields, and still re-fixes other fields. That's why I wrote that you need "cleanup_excluding_const_fields_processor" which is a processor that will exclude *all* fields, const or not.
I think I will need to spend a lot more time learning this area to get this working. So, I am currently leaning toward using a separate recursive call to Item::clean_cached_value() in fix_session_vcol_expr() and leaving its non-recursive call to cleanup() as is.
But the cleanup must be recursive! Unrelated to cached items, I had it in my review - there is a test case with no cached items, that shows a bug with non-recursive cleanup. Basically the result is correct for a virtual column that uses current_user(), but not correct when the virtual column uses concat(current_user()).
Is there a reason why the Virtual_column_info class doesn't include the vcol type? Is there a reason why the Virtual_column_info class shouldn't include the vcol type?
I think it could include the type. At least I cannot think of any reason why it shouldn't. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Jacob Mathew -
Sergei Golubchik