maria-developers team mailing list archive

[Jacob Mathew <jacob.mathew@xxxxxxxxxxx>]

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.

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.

> 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.

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


On Thu, Mar 30, 2017 at 9:42 AM, Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> Hi, Jacob!
>
> On Mar 28, jacob.mathew@xxxxxxxxxxx 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@xxxxxxxxxxx
>