maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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