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) [email protected] On Fri, Mar 31, 2017 at 3:02 AM, Sergei Golubchik <[email protected]> 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 [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
