Hi, Jacob!

On Mar 28, jacob.mathew@mariadb.com 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@mariadb.org