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 <serg@mariadb.org> wrote:
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