← Back to team overview

maria-developers team mailing list archive

Re: b4a223e3381: MDEV-10355 Weird error message upon CREATE TABLE with DEFAULT

 

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@xxxxxxxxxxx


On Fri, Mar 31, 2017 at 3:02 AM, Sergei Golubchik <serg@xxxxxxxxxxx> 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@xxxxxxxxxxx
>

Follow ups

References